-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Honda: Gear position message refactor #2457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Something to keep in mind here and in the future: openpilot doesn't actually care about the specific gear we're in. It just needs to know whether we should engage or not. |
|
All the Bosch C appear to be gearbox 50. Even the Accord which is CVT: 4df2d9d0197e20e3/0000000e--54aa88db6f I just realized you moved dbc signals names/locations too - will check that out and report back on Bosch C. |
Are you sure that route isn't from a hybrid Accord with an e-CVT? It's possible that I didn't get this distinction quite right, it's based on messages present aligned with Wikipedia research:
If it makes sense to store the detected difference in a flag rather than trans type, I can do that. But what really matters is that we're coalescing around two messages, each of which have consistent frequencies and signals. |
…gearshift-cleanup
|
Sorry, read discord history from the driver and it looks like it was a hybrid, and I have no DMs active with CVT Accord drivers. But did confirm that a CVT CRV is using the same CVT signal you build: 2dc4489d7e1410ca/00000000--8cc91f7e24 I presume Accord would be the same. But there is an issue with Bosch C:
edit: seems you refactored the canfd pilot signal too. Guess it was fun endian stuff... I'll merge this into my combined Honda build in discord so there is early testing in case any issues arise. |
Taking longer than anticipated. vanillagorilla didn't build canfd long control safety tests. Unseen until now since safety coverage ci used to timeout on forks. Will fix (target Wednesday evening) in combined-honda before asking people to test. |
|
You don't have to worry about this one too much. It's mostly good but not quite finished or quite fully tested yet, and I'll just add the CI test routes from your new PRs to the validation notebook in commaai/openpilot#35682. |
…gearshift-cleanup
…gearshift-cleanup
…gearshift-cleanup
|
@adeebshihadeh @sshane This is large change to the most crufty part of the Honda port, and touches ALL Hondas. I'm reasonably confident in my testing, but be on the lookout in case I missed something. Expect process_replay to see a diff on CP.transmissionType, but not for actual gear positions. |
|
Looks like one of the fuzzing tests failed: https://github.com/commaai/opendbc/actions/runs/16451100901/job/46496300736 |
#2525, hopefully. I'm not fully clear on what the fuzzer was changing and it doesn't seem to happen every run. |
|
Integra CVT uses the auto message 0x191. Seems to cause no problems, but the label isn't 100% precise. ad9840558640c31d/0000011e--89f22afa86 |
The Honda gearbox message situation needed some help. Four different messages defined with an array of overlapping and conflicting names, incorrect message frequencies, different fields used from the same message in different DBCs, different signal bit widths and endianness settings, several ways to pick a message, lots of duplication. Other than that it was great.
This PR migrates the Honda port to gearbox message auto-detection with a common gearbox DBC. Virtually all cars are covered by two messages: one for traditional CVTs, and another for traditional autos, eCVTs and direct-drive EVs. The oldest ACURA_RDX is a snowflake that has to stay in its custom DBC.
Validation
Verified against several data sources using commaai/openpilot#35682:
TODO