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.