VW MQB: Add CRC for Airbag_01, ESP_02, ESP10, ESP33, Licht_Anf_01

[JaCzekanski]

When writing a MQB can simulator I used this project for CAN frame definitions and CRC calculations.
CRC calculation for some of the messages was missing, as they are not used by OpenPilot, but since they are defined in .dbc (and I had Cabana logs from my vehicle) I wrote a brute forcer that could satisfy the CHECKSUM algorithm.

I used 2Q0 front assist camera module for testing and when sending an invalid CRC the module would show a DTC error. Sending a valid message with increasing COUNTER and CHECKSUM marked the DTC as inactive.

[jyoung8607]

Sounds like you're working on something interesting. In case it helps you, I found out very recently there's documentation on this algorithm. See the AUTOSAR E2E Protocol Specification, specifically Profile 2. You'll still need to brute force the "data IDs".

I like the idea of opendbc having value beyond openpilot, but if we're going to add support for message CRCs that openpilot doesn't use, we need to add some tests. Really they should have tests right now; we currently get away without because everything openpilot uses gets exercised in openpilot and panda. We shouldn't add anything that has no test coverage at all.

In its current form, cleanup of the messages that use the same data ID for all counters would be fine. I debated this same style back when I wrote it. However, the message list has grown to where it should really become a std::map lookup table, and in that case it'll be much simpler to keep them expanded. This will also make it much cleaner to add support for more messages.

[JaCzekanski]

@jyoung8607 As for test coverage - I can export messages with consecutive COUNTERs from my car's Cabana log and verify all of the added (and existing) CRCs.

Regarding refactor - I made a quick draft:

const std::unordered_map<uint32_t, std::array<uint8_t, 16>> crc_mqb_constants {
    // Airbag_01
    {0x40, {0x40}},
    // ESP_21 Electronic Stability Program
    {0xFD, {0xB4, 0xEF, 0xF8, 0x49, 0x1E, 0xE5, 0xC2, 0xC0, 0x97, 0x19, 0x3C, 0xC9, 0xF1, 0x98, 0xD6, 0x61}},
    // ...
};

unsigned int volkswagen_mqb_checksum(uint32_t address, const Signal &sig, const std::vector<uint8_t> &d) {
    /* ... */
    
    uint8_t crc = 0;
    
    auto crc_const = crc_mqb_constants.find(address);
    if (crc_const != crc_mqb_constants.end()) {
        uint8_t counter = d[1] & 0x0F;
        crc ^=  crc_const->second[counter];
    }
}

Defining constants using map + array takes care of not having to repeat the value 16 times. The only thing I don't like - it's not constexpr :(

[jyoung8607]

That looks good! To make this easy for the openpilot team to review, I suggest splitting this into three PRs.

Phase 1: Add tests for the currently existing messages, make sure it runs properly in CI.
Phase 2: Refactor volkswagen_mqb_checksum() along the lines you propose, CI tests will prove it's a null effect change.
Phase 3: Come back and add your new messages and data IDs under the new code and test structure.

[JaCzekanski]

@jyoung8607 Sorry for the lack of updates - not much time to work on side projects :(
I have a patch (almost) ready for getting the MQB CRC fully unit tested - I just need to extract messages from Cabana for all the IDs that have CRCs implemented. Initially, I was gonna add a big .csv file with raw messages, but I decided that it wasn't an elegant solution so I'm gonna preprocess the logs and add the minimum set of messages directly to .py test file.

[jyoung8607]

@JaCzekanski I'm still interested in these changes, if you're still interested in working on them. If you've moved on, eventually I'll pick this up and finish it. Moving to Draft for now.

[JaCzekanski]

Hey @jyoung8607, I completely forgot about this PR as I haven't touched CAN stuff in the last months.
I had everything ready including test data (already preprocessed) and the only piece missing was the serialization/deserialization of messages with missing field descriptions.

I'll push the commit with the tests added.

[edit]
Ouch, there's been a lot of changes here since I last visited!
I'll rebase and try to convert my code to pytest so it's less burden on your side.

[JaCzekanski]

Okay, I undid refactoring attempts and added the CRC's for the fields I needed.
I refactored my code to use pytest and for some of the messages its working great.
Some of them I had to skip over because of missing fields OR _BZ/_CRC fields instead of COUNTER/CHECKSUM - this should work once packer is fixed to support those or once .dbc are updated. Also, verify_mqb_crc_custom should be removed then since that's just a hack I tried to point to proper field names.

Test data is taken from my car's log, I just took random messages, making sure to include every counter value - this should cover most of the cases.

[jyoung8607]

Looks pretty good just reading it over, though I want to play with it locally just to make sure it fails where I think it'll fail.

I'd still like to do this in the three-phase approach we talked about earlier. First PR as discussed, let's bring in the new CI test with only the existing supported messages. Second PR, refactor the lookup table just the way you proposed, and your tests will give us assurance the refactor didn't break anything. Maybe tighten up the table to a single line per message, like this:

  {0xFD, {0xB4, 0xEF, 0xF8, 0x49, 0x1E, 0xE5, 0xC2, 0xC0, 0x97, 0x19, 0x3C, 0xC9, 0xF1, 0x98, 0xD6, 0x61}},  // ESP_21

Third PR, bring in your new messages. The easiest and most correct thing is to just make those messages fully supported. You can update vw_mqb_2010.dbc for your new messages to use the supported signal names CHECKSUM and COUNTER. That will activate opendbc's processing of those messages, enable openpilot (or other opendbc consumers) to use them if needed, and make it so you don't need a separate custom test.

In the meantime I'll track down some test messages for EV_Gearshift (it's unique to the e-Golf) so we can dispose of that exception.

[jyoung8607]

@JaCzekanski here's sample messages for 0x187 EV_Gearshift:

180.611	0x187	0	0x70801500000000F0
180.711	0x187	0	0x07811500000000F0
180.811	0x187	0	0x7A821500000000F0
180.911	0x187	0	0x26831500000000F0
181.011	0x187	0	0xBE841500000000F0
181.121	0x187	0	0x5A851500000000F0
181.221	0x187	0	0xFC861500000000F0
181.311	0x187	0	0x9E871500000000F0
181.411	0x187	0	0xAF881500000000F0
181.511	0x187	0	0x35891500000000F0
181.611	0x187	0	0xC58A1500000000F0
181.711	0x187	0	0x118B1500000000F0
181.811	0x187	0	0xD08C1500000000F0
181.911	0x187	0	0xE88D1500000000F0
182.011	0x187	0	0xF58E1500000000F0
182.11	0x187	0	0x008F1500000000F0

[JaCzekanski]

The first step is done in #1235.

[JaCzekanski]

And second step is here: #1236

I'll switch this PR into a draft until the rest is merged.

[jyoung8607]

@JaCzekanski Thanks for the other two PRs, you're good to update this PR with your new messages!

[jyoung8607]

Null-effect since openpilot doesn't make active use of these messages, but validated anyway:

• CI testing with test_checksums.py, all checks pass
• Local testing with pytest selfdrive/car/tests, all checks pass
• Local testing with selfdrive/tests/process_replay/test_processes.py, no changes

[jyoung8607]

@JaCzekanski Happened to look this over again, and ESP_33 support snuck in without test data. Can you file another PR adding test data for ESP_33? Long term I'd like to see automated coverage tests catch this for us, but tests like this are the first step to establishing that coverage.

[JaCzekanski]

@jyoung8607 So sorry about that :( I had the .csv with test data on drive, but somehow I didn't add it to .py test file.

Opened new PR with fix #1242