Updates for Volkswagen support #191
Conversation
# Conflicts: # vw_golf_mk4.dbc # vw_mqb_2010.dbc
|
Also added the PQ35/PQ46/NMS message VIN_1, similar to VIN_01 on MQB cars, that will let us auto-identify characteristics for some cars that are hard to tell apart by fingerprint. |
| SG_ Getriebe_11_CRC : 0|8@1+ (1,0) [0|255] "" Gateway_MQB,Motor_Diesel_MQB,Motor_Hybrid_MQB,Motor_Otto_MQB | ||
| SG_ Getriebe_11_BZ : 8|4@1+ (1,0) [0|15] "" Gateway_MQB,Motor_Diesel_MQB,Motor_Hybrid_MQB,Motor_Otto_MQB | ||
| SG_ CRC : 0|8@1+ (1,0) [0|255] "" Gateway_MQB,Motor_Diesel_MQB,Motor_Hybrid_MQB,Motor_Otto_MQB | ||
| SG_ COUNTERXX : 8|4@1+ (1,0) [0|15] "" Gateway_MQB,Motor_Diesel_MQB,Motor_Hybrid_MQB,Motor_Otto_MQB |
There was a problem hiding this comment.
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.
Typo?
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.
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.
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.
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.
_CRC: can't you just change https://github.com/commaai/openpilot/pull/836/files#diff-56069f2917a3366b3df2428fd6ac3202R34 so that it checks if_CRCis in the signal name?
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:
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.
|
Just one comment. Other than that looks good! |
Per my earlier comment, see commaai/openpilot#836 for the CRC validation code. I'm more than happy to revert the CRC and COUNTER renaming if we can find a better solution on that side. |
Per my Discord talk with @rbiasini, to reduce OP core code diffs a bit, we will temporarily (ab)use CHECKSUM to store a CRC like Pedal is doing today. After the port is upstreamed, we will look at updating the core OP code to support dynamic CRC and counter signal names, and revert the DBC signal names back to canonical (MSG_NAME_CRC, MSG_NAME_BZ, etc). I will update the OP core CRC support PR shortly.
|
ok, good to merge. Per our conversation, decided to just use canonical |
* Update ACC_02.SetSpeed to use a more accurate m/s formula instead of kmh * Fixes to LWI vs EPS calculations for steering angles, clarify descriptions of each. * Endianness fixes * Add ESP_15 and PSD_04/05/06 * Testing PSD interpretation * Correct message info for ESP_08 * Additional authoritative message names and IDs * OpenDBC updates * OpenDBC updates * DBC cleanup, CRC support, restore VIN_01 mux lost during Cabana editing. * Add the PQ heading control assist message. * Add a pointer to the VIN_1 mux. * Add comment about rate-limiting on Getriebe_11 counter signal. * Rename CRC->CHECKSUM for upstream of a minimum viable VW port. Per my Discord talk with @rbiasini, to reduce OP core code diffs a bit, we will temporarily (ab)use CHECKSUM to store a CRC like Pedal is doing today. After the port is upstreamed, we will look at updating the core OP code to support dynamic CRC and counter signal names, and revert the DBC signal names back to canonical (MSG_NAME_CRC, MSG_NAME_BZ, etc). I will update the OP core CRC support PR shortly.
* Update ACC_02.SetSpeed to use a more accurate m/s formula instead of kmh * Fixes to LWI vs EPS calculations for steering angles, clarify descriptions of each. * Endianness fixes * Add ESP_15 and PSD_04/05/06 * Testing PSD interpretation * Correct message info for ESP_08 * Additional authoritative message names and IDs * OpenDBC updates * OpenDBC updates * DBC cleanup, CRC support, restore VIN_01 mux lost during Cabana editing. * Add the PQ heading control assist message. * OpenDBC updates * Add a pointer to the VIN_1 mux. * Probable EPS_1 message * Add comment about rate-limiting on Getriebe_11 counter signal. * Rename CRC->CHECKSUM for upstream of a minimum viable VW port. Per my Discord talk with @rbiasini, to reduce OP core code diffs a bit, we will temporarily (ab)use CHECKSUM to store a CRC like Pedal is doing today. After the port is upstreamed, we will look at updating the core OP code to support dynamic CRC and counter signal names, and revert the DBC signal names back to canonical (MSG_NAME_CRC, MSG_NAME_BZ, etc). I will update the OP core CRC support PR shortly. * OpenDBC updates * OpenDBC updates * Probable EPS_1 message * Updates for Volkswagen support (#191) * Update ACC_02.SetSpeed to use a more accurate m/s formula instead of kmh * Fixes to LWI vs EPS calculations for steering angles, clarify descriptions of each. * Endianness fixes * Add ESP_15 and PSD_04/05/06 * Testing PSD interpretation * Correct message info for ESP_08 * Additional authoritative message names and IDs * OpenDBC updates * OpenDBC updates * DBC cleanup, CRC support, restore VIN_01 mux lost during Cabana editing. * Add the PQ heading control assist message. * Add a pointer to the VIN_1 mux. * Add comment about rate-limiting on Getriebe_11 counter signal. * Rename CRC->CHECKSUM for upstream of a minimum viable VW port. Per my Discord talk with @rbiasini, to reduce OP core code diffs a bit, we will temporarily (ab)use CHECKSUM to store a CRC like Pedal is doing today. After the port is upstreamed, we will look at updating the core OP code to support dynamic CRC and counter signal names, and revert the DBC signal names back to canonical (MSG_NAME_CRC, MSG_NAME_BZ, etc). I will update the OP core CRC support PR shortly. * Lexus is adjustment (#192) * Add 581 for ISH gas pedal signal * Add STEER_ANGLE in 608 STEER_TORQUE_SENSOR (similar to TSS2) * Change STEER_TORQUE_EPS factor in 608 STEER_TORQUE_SENSOR to 1.0 * Re-generate lexus_is_2018 * reduce factor to 0.77 * Fix steer angle factor for toyota * OpenDBC updates * Create luxgen_s5_2014.dbc (#101) * Create luxgen_s5_2015.dbc * Update luxgen_s5_2015.dbc * Update luxgen_s5_2015.dbc * fixed to luxgen dbc file * EV_Gearshift message * EV_Gearshift message for e-Golf support without a separate gearbox. * Corrected byte order on two signals. * Corrected byte order on several homebrewed VW MQB signals.
This is the second in a series of PRs to upstream the Volkswagen, Audi, SEAT, and Škoda community port, covering all MQB platform vehicles with factory ACC.
Free bonus: the "money message" for older Volkswagen, Audi, SEAT and Škoda vehicles based on the PQ35/PQ46/NMS platform. We're engaged in discussions with some B6/B7 Passat and Octavia owners. We've identified the message for Heading Control Assist torque, the important signals therein, and worked out the checksum (credit to Bugsy for this one). We're still a ways out for community support, much less upstream, but sharing what we have so far.