-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Updates for Volkswagen support #191
Merged
Merged
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
d066bdd
Update ACC_02.SetSpeed to use a more accurate m/s formula instead of kmh
jyoung8607 54f09f0
Fixes to LWI vs EPS calculations for steering angles, clarify descrip…
jyoung8607 67fba05
Endianness fixes
jyoung8607 25f4954
Add ESP_15 and PSD_04/05/06
jyoung8607 2ee4902
Testing PSD interpretation
jyoung8607 c912559
Correct message info for ESP_08
jyoung8607 d7904a7
Additional authoritative message names and IDs
jyoung8607 ee4a113
OpenDBC updates
jyoung8607 2da9f80
OpenDBC updates
jyoung8607 90287b6
Merge remote-tracking branch 'CommaAI/master'
jyoung8607 99f1abb
DBC cleanup, CRC support, restore VIN_01 mux lost during Cabana editing.
jyoung8607 9fc9a6f
Add the PQ heading control assist message.
jyoung8607 49c029e
Add a pointer to the VIN_1 mux.
jyoung8607 f1be7d1
Add comment about rate-limiting on Getriebe_11 counter signal.
jyoung8607 1d85513
Rename CRC->CHECKSUM for upstream of a minimum viable VW port.
jyoung8607 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo? Also, why can't we keep the CRC/Counter signal names as they are? The suffix _CRC and _BZ seems ok. Generally, I would like to keep opendbc as close as possible to the stock dbc file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of an ugly hack. I thought I had put a comment on that signal, but it appears to have been lost at some point. I'll add it back.
The Getriebe_11 message shows up on Extended CAN at 20Hz with the counter incrementing by five instead of one. I'm pretty sure what's happening is Getriebe_11 is sent on powertrain CAN at 100Hz and the J533 gateway is rate-limiting this message to conserve bandwidth. OP complains loudly about all the missing messages; that's the only reason I know this is happening! Renaming it prevents OP from doing COUNTER management on this particular message.
I'm in total agreement with you; I didn't want to change the names. Unfortunately OP as it stands today doesn't really lend itself to the checksum/CRC and counter signals having dynamic names. It's been a couple months since I've had my head in that code so I can't call out specifics ATM.
Later today I'm going to file a PR that covers just the OP Volkswagen CRC support, because I think you'll want to look at it separately from the vehicle port. Perhaps we should hold this PR in suspense until then? I'd actually be very happy if we can arrive at a solution that keeps the DBC aligned with canonical message and signal names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COUNTERXX
: yeah, reasonable hack. With the comment added is good._CRC
: can't you just change https://github.com/commaai/openpilot/pull/836/files#diff-56069f2917a3366b3df2428fd6ac3202R34 so that it checks if_CRC
is in the signal name?I'm happy to merge this PR right away, particularly if you are ok reverting the checksum/counter name changes (except for the one message at 5hz with skips).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be possible in the higher level Python code, but I'm screwed once I get down to the CANPacker code where I can't regexp search on key-value pair maps:
https://github.com/commaai/openpilot/blob/799302b842198b2cccb83c601bbe79ad1eaa5e32/selfdrive/can/packer.cc#L80
https://github.com/commaai/openpilot/blob/799302b842198b2cccb83c601bbe79ad1eaa5e32/selfdrive/can/packer.cc#L109
If I knew the message name, I could concatenate that and "_CRC" or "_BZ", but I only have the address and not the name at that point, and it's not clear to me if I can look it up. That's about the point where my eyes started bleeding and I gave up. Hints welcome.