Use CRC rather than simple checksum for EEPROM (#2517)

Use CRC rather than simple checksum for EEPROM

src/main/common/maths.c

@@ -350,6 +350,17 @@ uint16_t crc16_ccitt(uint16_t crc, unsigned char a)
     return crc;
 }
 
+uint16_t crc16_ccitt_update(uint16_t crc, const void *data, uint32_t length)
+{
+    const uint8_t *p = (const uint8_t *)data;
+    const uint8_t *pend = p + length;
+
+    for (; p != pend; p++) {
+        crc = crc16_ccitt(crc, *p);
+    }
+    return crc;
+}
+
 uint8_t crc8_dvb_s2(uint8_t crc, unsigned char a)
 {
     crc ^= a;

src/main/common/maths.h

@@ -140,5 +140,5 @@ static inline float constrainf(float amt, float low, float high)
         return amt;
 }
 uint16_t crc16_ccitt(uint16_t crc, unsigned char a);
+uint16_t crc16_ccitt_update(uint16_t crc, const void *data, uint32_t length);
 uint8_t crc8_dvb_s2(uint8_t crc, unsigned char a);
-

src/main/config/config_eeprom.c

@@ -35,15 +35,6 @@
 
 #include "fc/config.h"
 
-// declare a dummy PG, since scanEEPROM assumes there is at least one PG
-// !!TODO remove once first PG has been created out of masterConfg
-typedef struct dummpConfig_s {
-    uint8_t dummy;
-} dummyConfig_t;
-PG_DECLARE(dummyConfig_t, dummyConfig);
-#define PG_DUMMY_CONFIG 1
-PG_REGISTER(dummyConfig_t, dummyConfig, PG_DUMMY_CONFIG, 0);
-
 extern uint8_t __config_start;   // configured via linker script when building binaries.
 extern uint8_t __config_end;
 
@@ -58,10 +49,13 @@ typedef enum {
 } configRecordFlags_e;
 
 #define CR_CLASSIFICATION_MASK  (0x3)
+#define CRC_START_VALUE         0xFFFF
+#define CRC_CHECK_VALUE         0x1D0F  // pre-calculated value of CRC that includes the CRC itself
 
 // Header for the saved copy.
 typedef struct {
     uint8_t eepromConfigVersion;
+    uint8_t magic_be;           // magic number, should be 0xBE
     char boardIdentifier[sizeof(TARGET_BOARD_IDENTIFIER)];
 } PG_PACKED configHeader_t;
 
@@ -97,42 +91,33 @@ void initEEPROM(void)
     BUILD_BUG_ON(offsetof(packingTest_t, word) != 1);
     BUILD_BUG_ON(sizeof(packingTest_t) != 5);
 
-    BUILD_BUG_ON(sizeof(configHeader_t) != 1 + sizeof(TARGET_BOARD_IDENTIFIER));
+    BUILD_BUG_ON(sizeof(configHeader_t) != 2 + sizeof(TARGET_BOARD_IDENTIFIER));
     BUILD_BUG_ON(sizeof(configFooter_t) != 2);
     BUILD_BUG_ON(sizeof(configRecord_t) != 6);
 }
 
-static uint8_t updateChecksum(uint8_t chk, const void *data, uint32_t length)
-{
-    const uint8_t *p = (const uint8_t *)data;
-    const uint8_t *pend = p + length;
-
-    for (; p != pend; p++) {
-        chk ^= *p;
-    }
-    return chk;
-}
-
 // Scan the EEPROM config. Returns true if the config is valid.
 bool isEEPROMContentValid(void)
 {
-    uint8_t chk = 0;
     const uint8_t *p = &__config_start;
     const configHeader_t *header = (const configHeader_t *)p;
 
     if (header->eepromConfigVersion != EEPROM_CONF_VERSION) {
         return false;
     }
-
+    if (header->magic_be != 0xBE) {
+        return false;
+    }
     if (strncasecmp(header->boardIdentifier, TARGET_BOARD_IDENTIFIER, sizeof(TARGET_BOARD_IDENTIFIER))) {
         return false;
     }
 
-    chk = updateChecksum(chk, header, sizeof(*header));
+    uint16_t crc = CRC_START_VALUE;
+    crc = crc16_ccitt_update(crc, header, sizeof(*header));
     p += sizeof(*header);
 #ifndef USE_PARAMETER_GROUPS
     // include the transitional masterConfig record
-    chk = updateChecksum(chk, p, sizeof(masterConfig));
+    crc = crc16_ccitt_update(crc, p, sizeof(masterConfig));
     p += sizeof(masterConfig);
 #endif
 
@@ -149,19 +134,24 @@ bool isEEPROMContentValid(void)
             return false;
         }
 
-        chk = updateChecksum(chk, p, record->size);
+        crc = crc16_ccitt_update(crc, p, record->size);
 
         p += record->size;
     }
 
     const configFooter_t *footer = (const configFooter_t *)p;
-    chk = updateChecksum(chk, footer, sizeof(*footer));
+    crc = crc16_ccitt_update(crc, footer, sizeof(*footer));
     p += sizeof(*footer);
-    chk = ~chk;
+
-    const uint8_t checkSum = *p;
+    // include stored CRC in the CRC calculation
-    p += sizeof(checkSum);
+    const uint16_t *storedCrc = (const uint16_t *)p;
+    crc = crc16_ccitt_update(crc, storedCrc, sizeof(*storedCrc));
+    p += sizeof(storedCrc);
+
     eepromConfigSize = p - &__config_start;
-    return chk == checkSum;
+
+    // CRC has the property that if the CRC itself is included in the calculation the resulting CRC will have constant value
+    return crc == CRC_CHECK_VALUE;
 }
 
 uint16_t getEEPROMConfigSize(void)
@@ -235,19 +225,20 @@ static bool writeSettingsToEEPROM(void)
     config_streamer_init(&streamer);
 
     config_streamer_start(&streamer, (uintptr_t)&__config_start, &__config_end - &__config_start);
-    uint8_t chk = 0;
 
     configHeader_t header = {
         .eepromConfigVersion =  EEPROM_CONF_VERSION,
+        .magic_be =             0xBE,
         .boardIdentifier =      TARGET_BOARD_IDENTIFIER,
     };
 
     config_streamer_write(&streamer, (uint8_t *)&header, sizeof(header));
-    chk = updateChecksum(chk, (uint8_t *)&header, sizeof(header));
+    uint16_t crc = CRC_START_VALUE;
+    crc = crc16_ccitt_update(crc, (uint8_t *)&header, sizeof(header));
 #ifndef USE_PARAMETER_GROUPS
     // write the transitional masterConfig record
     config_streamer_write(&streamer, (uint8_t *)&masterConfig, sizeof(masterConfig));
-    chk = updateChecksum(chk, (uint8_t *)&masterConfig, sizeof(masterConfig));
+    crc = crc16_ccitt_update(crc, (uint8_t *)&masterConfig, sizeof(masterConfig));
 #endif
     PG_FOREACH(reg) {
         const uint16_t regSize = pgSize(reg);
@@ -262,9 +253,9 @@ static bool writeSettingsToEEPROM(void)
             // write the only instance
             record.flags |= CR_CLASSICATION_SYSTEM;
             config_streamer_write(&streamer, (uint8_t *)&record, sizeof(record));
-            chk = updateChecksum(chk, (uint8_t *)&record, sizeof(record));
+            crc = crc16_ccitt_update(crc, (uint8_t *)&record, sizeof(record));
             config_streamer_write(&streamer, reg->address, regSize);
-            chk = updateChecksum(chk, reg->address, regSize);
+            crc = crc16_ccitt_update(crc, reg->address, regSize);
         } else {
             // write one instance for each profile
             for (uint8_t profileIndex = 0; profileIndex < MAX_PROFILE_COUNT; profileIndex++) {
@@ -272,10 +263,10 @@ static bool writeSettingsToEEPROM(void)
 
                 record.flags |= ((profileIndex + 1) & CR_CLASSIFICATION_MASK);
                 config_streamer_write(&streamer, (uint8_t *)&record, sizeof(record));
-                chk = updateChecksum(chk, (uint8_t *)&record, sizeof(record));
+                crc = crc16_ccitt_update(crc, (uint8_t *)&record, sizeof(record));
                 const uint8_t *address = reg->address + (regSize * profileIndex);
                 config_streamer_write(&streamer, address, regSize);
-                chk = updateChecksum(chk, address, regSize);
+                crc = crc16_ccitt_update(crc, address, regSize);
             }
         }
     }
@@ -285,15 +276,15 @@ static bool writeSettingsToEEPROM(void)
     };
 
     config_streamer_write(&streamer, (uint8_t *)&footer, sizeof(footer));
-    chk = updateChecksum(chk, (uint8_t *)&footer, sizeof(footer));
+    crc = crc16_ccitt_update(crc, (uint8_t *)&footer, sizeof(footer));
 
-    // append checksum now
+    // include inverted CRC in big endian format in the CRC
-    chk = ~chk;
+    const uint16_t invertedBigEndianCrc = ~(((crc & 0xFF) << 8) | (crc >> 8));
-    config_streamer_write(&streamer, &chk, sizeof(chk));
+    config_streamer_write(&streamer, (uint8_t *)&invertedBigEndianCrc, sizeof(crc));
 
     config_streamer_flush(&streamer);
 
-    bool success = config_streamer_finish(&streamer) == 0;
+    const bool success = config_streamer_finish(&streamer) == 0;
 
     return success;
 }