Add core OP support for CRC validation, with extra support for Volkswagen MQB

[jyoung8607]

Fourth of five PRs to upstream the Volkswagen, Audi, SEAT, and Škoda community port. This one reduces the size and scope of the final OP PR by giving you a look at the core CRC stuff separated from the traditional vehicle port.

This code teaches OP generally about the signal name "CRC", like it knows "CHECKSUM" today. It also teaches OP about the specific algorithm used by Volkswagen MQB, which is CRC-8/AUTOSAR with a twist. It enables OP to do CRC and counter validation on all Volkswagen MQB received messages it relies on, similar to Honda and Toyota, and to set the CRC on transmitted messages.

I wrote the overarching CRC signal stuff with extension to other vehicle ports in mind. It could see use for Pedal, Hyundai, Mazda, and Nissan at a minimum. The wrapper function init_crc_lookup_tables() exists for the purpose of adding other CRC lookup tables as needed. In the beginning I had them embedded as static lookup tables, but switched to computing them at startup on informal feedback from @rbiasini.

Note: this code requires the CRC signal be named exactly "CRC", just as CHECKSUM and COUNTER work today. OP would need some nontrivial refactoring to support magic signal names that vary by message. As such, our PR commaai/opendbc#191 changes those signal names to CRC and COUNTER respectively for select Volkswagen MQB messages we rely on. I do not like this and would much prefer to keep the signal names canonical, and am willing to talk more about it if you see a reasonable path to fix it.

[rbiasini]

Nice. I would like to merge this in. 3 things:

• see comment Add core OP support for CRC validation, with extra support for Volkswagen MQB #836 (comment)
• there are some conflicts
• Rename CRC with CHECKSUM so it's compatible with the dbc file we already merged in?

As we discussed, I agree I don't like canonicalizing the dbc files. On the other hand, we can come back to this after we have a MVP VW port. Thanks!

[jyoung8607]

Thanks for the review.

Per our DM discussion I will push a commit to rename things to CHECKSUM for now, and we'll follow up with support for the correct MSG_NAME_[CRC|BZ] scheme later. I already have the conflicts fixed, and I will look at adopting the start bit generalization from your comment.

It may be a few days until I can follow up, because my CAN bus implodes when I try to run 0.6.5 for reasons I have not yet identified, and I don't have cycles to hack on it further today.

[jyoung8607]

Nice. I would like to merge this in. 3 things:

Rename CRC with CHECKSUM so it's compatible with the dbc file we already merged in?

Done per our discussion.

there are some conflicts

The PR has been brought current with OP 0.6.5-devel.

see comment
...
What about defining both counter_start_bit and checksum_start_bit as well? Basically after this line there should be no reference to car_type, except for the call to template.render.

I've refactored parser_dbc.py to be cleaner, hopefully in the way you were thinking. We can't really verify exact start bits per se, because those messages move around for at least some cars. Honda seems to do this a lot. The existing code was really just checking alignment for the most part.

The refactor doesn't check it in precisely the same way (it will allow a Honda checksum to start on bit 7, or a Honda counter to start on any odd bit) but I trust it complies with the spirit of your request. There's really no other way to do this in a manufacturer-generic way.

The refactored code does the alignment check based on endianness, and along the way it checks the DBC for correctness. It found two issues in the 2016 Accord Touring DBC, and you'll notice the Travis build for this PR fails at that point. I've submitted commaai/opendbc#193 to fix it properly.

[jyoung8607]

Two final comments:

First, my 0.6.5 merge for conflict resolution seems to have added some new-in-0.6.5 files to the PR. My git-fu is not that strong and I'm not sure of the right way to pull them out, I wonder if deleting them might make things worse? Let me know if you can fix this, or if I should withdraw and resubmit the PR from a clean 0.6.5 fork.

Second, you'll notice some extra instrumentation to print the name of the DBC when a fault occurs. In the past, depending on when and why process_dbc.py ran, you might get no information at all about which DBC failed to process, or worse, something extremely misleading if there's more than one file with problems.

Here's an example of two files broken, Honda and VW. The make is triggered by the Honda DBC change, but the process_dbc failure is actually from the VW file:

Example of a situation where you'd normally have no context at all, but the refactored code tells you what DBC it's working with now:

[rbiasini]

RE: added files: yeah, just delete them. You also want to add the opendbc changes to this PR (at least the accord fix), so tests won't fail.

Don't worry about how clean is the history of commits of the PR, I'll squash all your commits before merging. As long as the diffs are good and there are no conflicts reported, we are good.

And nice addition printing the dbc file that fails.

[jyoung8607]

RE: added files: yeah, just delete them.

Done.

You also want to add the opendbc changes to this PR (at least the accord fix),

Done.

so tests won't fail.

Looks good now.

Don't worry about how clean is the history of commits of the PR, I'll squash all your commits before merging.

Hulk squash!

As long as the diffs are good and there are no conflicts reported, we are good.

And nice addition printing the dbc file that fails.

Thank you for your support through this process!

[MikhailTik55]

Hello,i make the correction table CRC DataIDList VW MQB.
Bus dumps Gateway_MQB Running (Bremse), Powertrain (Motor), Extended are required.
I will be grateful for any information.
CRC: ID 0x117 I got a correction of 0x16 in the 0xAC program.

[jyoung8607]

@MikhailTik55 I don't quite understand what you're asking for.

To the extent I've solved the CRC pad byte for a given message, you can already find the solution in this PR. Each message requires a different solution; I have not found a general approach. I have not bothered to solve the pad byte for all messages, only the ones I needed.

I have some code that helps me with the per-message brute force process, but I'm not planning to publish it, because it's a write-only disaster full of cruft from when I was trying to find the overall algorithm, and would not be usable for anyone else.

If you have an openpilot related need to solve a certain message, I'm willing to do one or two on your behalf. If you need it for something else, or want a solution for every message, there's sufficient information in this PR to trivially write your own brute force tester.

Further questions would best be asked in #volkswagen on Comma's Discord.

[MikhailTik55]

Hello! Thank you for your interest to my message.
I adapt VAG assistants to CAN bus Runault.
The protocol of VAG MQB messages is similar to specification AUTOSAR CRC8H2F E2E Profile 2.
https://www.autosar.org/fileadmin/user_upload/standards/classic/4-3/AUTOSAR_SWS_E2ELibrary.pdf
Here’s an interesting file, have a look at it:
https://android.googlesource.com/kernel/msm/+/c1aabdf379bc2feeb0df7057ed5bad96f492133e/net/can/gw.com
In order to make right messages with correct CRC I need hex-dumps of tires Gateway MBQ, like:
GW MQB Extended
00000117 75 0C 00 40 68 00 00 FF C2 00 08
000003DC FF 01 00 00 00 01 00 00 CF 00 08
00000117 11 0D 00 40 68 00 00 FF D6 00 08
0000030C 71 FF 3F FF 63 30 00 00 D7 00 08
000003DB FF 03 00 00 80 00 00 FE E3 00 08
00000117 BD 0E 00 40 68 00 00 FF EA 00 08
00000324 37 0F 00 00 04 00 00 00 EB 00 08
000003DF FF 00 01 38 15 00 00 00 ED 00 08
00000117 D9 0F 00 40 68 00 00 FF FE 00 08