Fix #2449, crc calculation refactor

Move the current CRC-16 algorithm to a separate source file and better structure the code to support future CRC algorithm alternatives. Improve documentation to better indicate what the current algorithm is and what to expect going forward. Also corrects for issues in the CRC functional test: - Use the standard check input and compare against the standard check value for CRC-16/ARC. - Change cases from MIR to a normal test case - given a specific algorithm with specific input, the return value should be the same.
nasa · Sep 29, 2023 · e437420 · e437420
1 parent c1aa16a
commit e437420
Show file tree

Hide file tree

Showing 10 changed files with 288 additions and 104 deletions.
diff --git a/docs/cFE Application Developers Guide.md b/docs/cFE Application Developers Guide.md
@@ -1375,22 +1375,30 @@ uint32 CFE_ES_CalculateCRC(const void *DataPtr, size_t DataLength, uint32 InputC
 where DataPtr points to the first byte of an array of bytes that are to have
 the CRC calculated on, DataLength specifies the number of sequential bytes to
 include in the calculation, InputCRC is the initial value of the CRC and
-TypeCRC identifies which of the standard CRC polynomials to be used.  Currently,
-there are the following types available:
+TypeCRC identifies which of the standard CRC polynomials to be used.
 
-```
-CFE_ES_CrcType_CRC_8 – an 8-bit additive checksum calculation that returns a 32-bit value
-CFE_ES_CrcType_CRC_16 – a 16-bit additive checksum calculation that returns a 32-bit value
-CFE_ES_CrcType_CRC_32 – a 32-bit additive checksum calculation that returns a 32-bit value
-CFE_MISSION_ES_DEFAULT_CRC – the mission specified default CRC calculation (currently
-                             this is set to CFE_ES_CrcType_CRC_16 in sample_mission_cfg.h)
-```
+The set of available CRC algorithms for the TypeCRC parameter depends on the version
+of CFE and compile-time options selected.  Historically, CFE only has
+implemented a single algorithm which is CRC-16/ARC:
+
+ - Polynomial: 0x8005
+ - Initial Value: 0x0000
+ - Reflect In/Out: true
+ - Final XOR: 0x0000
+ - Check Value: 0xbb3d
+
+See the definition of the `CFE_ES_CrcType_Enum_t` type for complete documentation of all
+available values for this paramater and details of the algorithm used for each.  Note that
+this enumeration may be extended in future versions of CFE, as mission needs evolve.
+
+For application compatibility, the `CFE_MISSION_ES_DEFAULT_CRC` macro is defined as part of
+the CFE ES API configuration, which maps to the recommended CRC algorithm for applications
+to use.  By default this is set to CRC-16/ARC, but it can be configured based on mission
+preference and the set of available CRC algorithms.
 
 Unless there is a specific interface with a specified CRC calculation, applications
-must use the CFE_MISSION_ES_DEFAULT_CRC type.
+should use the `CFE_MISSION_ES_DEFAULT_CRC` value for TypeCRC when invoking this API.
 
-Currently only CFE_ES_CrcType_CRC_16 is supported. CFE_ES_CrcType_CRC_8 and CFE_ES_CrcType_CRC_32 are yet
-to be implemented.
 
 ## 5.11 File System Functions
 

diff --git a/modules/cfe_testcase/src/es_misc_test.c b/modules/cfe_testcase/src/es_misc_test.c
@@ -31,29 +31,43 @@
 
 void TestCalculateCRC(void)
 {
-    const char *Data     = "Random Stuff";
-    uint32      inputCrc = 345353;
-    uint32      Result;
+    static const char CRC_CHECK_INPUT[] = "123456789";
+    static const char CRC_OTHER_INPUT[] = "Random Stuff";
+
+    uint32 inputCrc = 345353;
 
     UtPrintf("Testing: CFE_ES_CalculateCRC");
 
-    /* CRC is implementation specific, functional just checks that a result is produced and reports */
-    UtAssert_VOIDCALL(Result = CFE_ES_CalculateCRC(Data, sizeof(Data), 0, CFE_MISSION_ES_DEFAULT_CRC));
-    UtAssert_MIR("Confirm mission default CRC of \"%s\" is %lu", Data, (unsigned long)Result);
+    /* The CFE_ES_CrcType_NONE algorithm always returns 0. */
+    UtAssert_UINT32_EQ(CFE_ES_CalculateCRC(CRC_CHECK_INPUT, sizeof(CRC_CHECK_INPUT) - 1, 1, CFE_ES_CrcType_NONE), 0);
+
+    /* Calling with an invalid type value should get aliased to NONE. */
+    UtAssert_UINT32_EQ(CFE_ES_CalculateCRC(CRC_CHECK_INPUT, sizeof(CRC_CHECK_INPUT) - 1, 1, CFE_ES_CrcType_MAX + 1), 0);
+
+    /* The CFE_ES_CrcType_16_ARC algorithm uses an input value of 0 and the check value result is 0xbb3d */
+    UtAssert_UINT32_EQ(CFE_ES_CalculateCRC(CRC_CHECK_INPUT, sizeof(CRC_CHECK_INPUT) - 1, 0, CFE_ES_CrcType_16_ARC),
+                       0xBB3D);
 
-    UtAssert_VOIDCALL(Result = CFE_ES_CalculateCRC(Data, sizeof(Data), inputCrc, CFE_ES_CrcType_CRC_16));
-    UtAssert_MIR("Confirm CRC16 of \"%s\" with input CRC of %lu is %lu", Data, (unsigned long)inputCrc,
-                 (unsigned long)Result);
+    /*
+     * In this version of CFE, only CRC-16/ARC is defined, so the default must be the same.  In a future version of CFE,
+     * if more than one option is available, this can go back to an "MIR" case, and the user must compare the result
+     * to the check value for what they have actully configured as the default CRC.
+     */
+    UtAssert_UINT32_EQ(CFE_ES_CalculateCRC(CRC_CHECK_INPUT, sizeof(CRC_CHECK_INPUT) - 1, 0, CFE_MISSION_ES_DEFAULT_CRC),
+                       0xBB3D);
 
-    UtAssert_VOIDCALL(Result = CFE_ES_CalculateCRC(Data, sizeof(Data), 0, CFE_ES_CrcType_CRC_8));
-    UtAssert_MIR("Confirm CRC8 of \"%s\" is %lu", Data, (unsigned long)Result);
+    /* Compute a secondary string - the reference value obtained using 3rd party tool implementing same algorithm */
+    UtAssert_UINT32_EQ(CFE_ES_CalculateCRC(CRC_OTHER_INPUT, sizeof(CRC_OTHER_INPUT) - 1, 0, CFE_ES_CrcType_16_ARC),
+                       0x11E3);
 
-    UtAssert_VOIDCALL(Result = CFE_ES_CalculateCRC(Data, sizeof(Data), 0, CFE_ES_CrcType_CRC_32));
-    UtAssert_MIR("Confirm CRC32 of \"%s\" is %lu", Data, (unsigned long)Result);
+    /* Test of nonzero initial value, this is used for checking CRC in across non-contiguous chunks or across multiple
+     * cycles */
+    UtAssert_UINT32_EQ(CFE_ES_CalculateCRC(CRC_CHECK_INPUT, sizeof(CRC_CHECK_INPUT) - 1, 345353, CFE_ES_CrcType_16_ARC),
+                       0xE493);
 
     /* NULL input or 0 size returns input crc */
-    UtAssert_UINT32_EQ(CFE_ES_CalculateCRC(NULL, sizeof(Data), inputCrc, CFE_ES_CrcType_CRC_16), inputCrc);
-    UtAssert_UINT32_EQ(CFE_ES_CalculateCRC(Data, 0, inputCrc, CFE_ES_CrcType_CRC_16), inputCrc);
+    UtAssert_UINT32_EQ(CFE_ES_CalculateCRC(NULL, sizeof(CRC_OTHER_INPUT), inputCrc, CFE_ES_CrcType_16_ARC), inputCrc);
+    UtAssert_UINT32_EQ(CFE_ES_CalculateCRC(CRC_OTHER_INPUT, 0, inputCrc, CFE_ES_CrcType_16_ARC), inputCrc);
 }
 
 void TestWriteToSysLog(void)

diff --git a/modules/core_api/fsw/inc/cfe_es.h b/modules/core_api/fsw/inc/cfe_es.h
@@ -998,14 +998,7 @@ CFE_Status_t CFE_ES_WriteToSysLog(const char *SpecStringPtr, ...) OS_PRINTF(1, 2
 **                          allows the user to calculate the CRC of non-contiguous blocks as
 **                          a single value.  Nominally, the user should set this value to zero.
 **
-** \param[in]   TypeCRC     One of the following CRC algorithm selections:
-**                          \arg \c CFE_ES_CrcType_CRC_8 -  (Not currently implemented)
-**                          \arg \c CFE_ES_CrcType_CRC_16 - CRC-16/ARC <BR>
-**                                  Polynomial: 0x8005 <BR>
-**                                  Initialization: 0x0000 <BR>
-**                                  Reflect Input/Output: true <BR>
-**                                  XorOut: 0x0000
-**                          \arg \c CFE_ES_CrcType_CRC_32 - (not currently implemented)
+** \param[in]   TypeCRC     One of the following CRC algorithm selections defined in CFE_ES_CrcType_Enum_t
 **
 ** \return The result of the CRC calculation on the specified memory block.
 **         If the TypeCRC is unimplemented will return 0.

diff --git a/modules/core_api/fsw/inc/cfe_es_api_typedefs.h b/modules/core_api/fsw/inc/cfe_es_api_typedefs.h
@@ -86,13 +86,53 @@ typedef void *CFE_ES_StackPointer_t; /* aka osal_stackptr_t in proposed OSAL cha
 /**
  * \brief Checksum/CRC algorithm identifiers
  *
- * Currently only CFE_ES_CrcType_CRC_16 is supported.
+ * Currently only CFE_ES_CrcType_16_ARC is supported.
+ *
+ * All defined CRC algorithms should include a check value, which is the accepted
+ * result of computing the CRC across the fixed string "123456789"
  */
 typedef enum CFE_ES_CrcType_Enum
 {
-    CFE_ES_CrcType_CRC_8  = 1, /**< \brief CRC ( 8 bit additive - returns 32 bit total) (Not currently implemented) */
-    CFE_ES_CrcType_CRC_16 = 2, /**< \brief CRC (16 bit additive - returns 32 bit total) */
-    CFE_ES_CrcType_CRC_32 = 3  /**< \brief CRC (32 bit additive - returns 32 bit total) (Not currently implemented) */
+    /**
+     * \brief Reserved placeholder value
+     *
+     * No computation is performed, always returns 0 as a result.
+     */
+    CFE_ES_CrcType_NONE = 0,
+
+    /**
+     * \brief Implementation of CRC-16/ARC
+     *
+     * \par
+     *   Polynomial: 0x8005 <br>
+     *   Initialization: 0x0000 <br>
+     *   Reflect Input/Output: true <br>
+     *   Check value: 0xbb3d <br>
+     *   XorOut: 0x0000 <br>
+     */
+    CFE_ES_CrcType_16_ARC = 1,
+
+    /**
+     * Placeholder for end of normal enumeration list
+     * This should reflect the number of algorithms defined.
+     */
+    CFE_ES_CrcType_MAX = 2,
+
+    /*
+     * Backward compatibility values.
+     * For compatibility with apps, these simplified symbols need to be defined,
+     * and they also must map to nonzero values.
+     */
+
+    /** CRC-16 historically implied CRC-16/ARC */
+    CFE_ES_CrcType_CRC_16 = CFE_ES_CrcType_16_ARC,
+
+    /**< CRC-8 historically defined but not implemented, value must not be 0 */
+    CFE_ES_CrcType_CRC_8 = CFE_ES_CrcType_MAX + 1,
+
+    /**< CRC-32 historically defined but not implemented, value must not be 0 */
+    CFE_ES_CrcType_CRC_32 = CFE_ES_CrcType_MAX + 2,
+
 } CFE_ES_CrcType_Enum_t;
 
 /**

diff --git a/modules/es/CMakeLists.txt b/modules/es/CMakeLists.txt
@@ -13,6 +13,7 @@ set(es_SOURCES
     fsw/src/cfe_es_backgroundtask.c
     fsw/src/cfe_es_cds.c
     fsw/src/cfe_es_cds_mempool.c
+    fsw/src/cfe_es_crc.c
     fsw/src/cfe_es_dispatch.c
     fsw/src/cfe_es_erlog.c
     fsw/src/cfe_es_generic_pool.c

diff --git a/modules/es/config/default_cfe_es_interface_cfg.h b/modules/es/config/default_cfe_es_interface_cfg.h
@@ -112,10 +112,10 @@
 **      Table Image data integrity values.
 **
 **  \par Limits
-**      Currently only CFE_ES_CrcType_CRC_16 is supported (see brief in CFE_ES_CrcType_Enum
+**      Currently only CFE_ES_CrcType_16_ARC is supported (see brief in CFE_ES_CrcType_Enum
 **      definition in cfe_es_api_typedefs.h)
 */
-#define CFE_MISSION_ES_DEFAULT_CRC CFE_ES_CrcType_CRC_16
+#define CFE_MISSION_ES_DEFAULT_CRC CFE_ES_CrcType_16_ARC
 
 /**
 **  \cfeescfg Maximum Length of Full CDS Name in messages
@@ -142,9 +142,9 @@
 
 /** \name Checksum/CRC algorithm identifiers */
 
-#define CFE_MISSION_ES_CRC_8  CFE_ES_CrcType_CRC_8  /* 1 */
-#define CFE_MISSION_ES_CRC_16 CFE_ES_CrcType_CRC_16 /* 2 */
-#define CFE_MISSION_ES_CRC_32 CFE_ES_CrcType_CRC_32 /* 3 */
+#define CFE_MISSION_ES_CRC_8  CFE_ES_CrcType_CRC_8
+#define CFE_MISSION_ES_CRC_16 CFE_ES_CrcType_CRC_16
+#define CFE_MISSION_ES_CRC_32 CFE_ES_CrcType_CRC_32
 
 #endif
 

diff --git a/modules/es/fsw/src/cfe_es_api.c b/modules/es/fsw/src/cfe_es_api.c
@@ -1573,72 +1573,22 @@ CFE_Status_t CFE_ES_WriteToSysLog(const char *SpecStringPtr, ...)
  *-----------------------------------------------------------------*/
 uint32 CFE_ES_CalculateCRC(const void *DataPtr, size_t DataLength, uint32 InputCRC, CFE_ES_CrcType_Enum_t TypeCRC)
 {
-    uint32       i;
-    int16        Index;
-    int16        Crc = 0;
-    const uint8 *BufPtr;
-    uint8        ByteValue;
-
-    static const uint16 CrcTable[256] = {
-
-        0x0000, 0xC0C1, 0xC181, 0x0140, 0xC301, 0x03C0, 0x0280, 0xC241, 0xC601, 0x06C0, 0x0780, 0xC741, 0x0500, 0xC5C1,
-        0xC481, 0x0440, 0xCC01, 0x0CC0, 0x0D80, 0xCD41, 0x0F00, 0xCFC1, 0xCE81, 0x0E40, 0x0A00, 0xCAC1, 0xCB81, 0x0B40,
-        0xC901, 0x09C0, 0x0880, 0xC841, 0xD801, 0x18C0, 0x1980, 0xD941, 0x1B00, 0xDBC1, 0xDA81, 0x1A40, 0x1E00, 0xDEC1,
-        0xDF81, 0x1F40, 0xDD01, 0x1DC0, 0x1C80, 0xDC41, 0x1400, 0xD4C1, 0xD581, 0x1540, 0xD701, 0x17C0, 0x1680, 0xD641,
-        0xD201, 0x12C0, 0x1380, 0xD341, 0x1100, 0xD1C1, 0xD081, 0x1040, 0xF001, 0x30C0, 0x3180, 0xF141, 0x3300, 0xF3C1,
-        0xF281, 0x3240, 0x3600, 0xF6C1, 0xF781, 0x3740, 0xF501, 0x35C0, 0x3480, 0xF441, 0x3C00, 0xFCC1, 0xFD81, 0x3D40,
-        0xFF01, 0x3FC0, 0x3E80, 0xFE41, 0xFA01, 0x3AC0, 0x3B80, 0xFB41, 0x3900, 0xF9C1, 0xF881, 0x3840, 0x2800, 0xE8C1,
-        0xE981, 0x2940, 0xEB01, 0x2BC0, 0x2A80, 0xEA41, 0xEE01, 0x2EC0, 0x2F80, 0xEF41, 0x2D00, 0xEDC1, 0xEC81, 0x2C40,
-        0xE401, 0x24C0, 0x2580, 0xE541, 0x2700, 0xE7C1, 0xE681, 0x2640, 0x2200, 0xE2C1, 0xE381, 0x2340, 0xE101, 0x21C0,
-        0x2080, 0xE041, 0xA001, 0x60C0, 0x6180, 0xA141, 0x6300, 0xA3C1, 0xA281, 0x6240, 0x6600, 0xA6C1, 0xA781, 0x6740,
-        0xA501, 0x65C0, 0x6480, 0xA441, 0x6C00, 0xACC1, 0xAD81, 0x6D40, 0xAF01, 0x6FC0, 0x6E80, 0xAE41, 0xAA01, 0x6AC0,
-        0x6B80, 0xAB41, 0x6900, 0xA9C1, 0xA881, 0x6840, 0x7800, 0xB8C1, 0xB981, 0x7940, 0xBB01, 0x7BC0, 0x7A80, 0xBA41,
-        0xBE01, 0x7EC0, 0x7F80, 0xBF41, 0x7D00, 0xBDC1, 0xBC81, 0x7C40, 0xB401, 0x74C0, 0x7580, 0xB541, 0x7700, 0xB7C1,
-        0xB681, 0x7640, 0x7200, 0xB2C1, 0xB381, 0x7340, 0xB101, 0x71C0, 0x7080, 0xB041, 0x5000, 0x90C1, 0x9181, 0x5140,
-        0x9301, 0x53C0, 0x5280, 0x9241, 0x9601, 0x56C0, 0x5780, 0x9741, 0x5500, 0x95C1, 0x9481, 0x5440, 0x9C01, 0x5CC0,
-        0x5D80, 0x9D41, 0x5F00, 0x9FC1, 0x9E81, 0x5E40, 0x5A00, 0x9AC1, 0x9B81, 0x5B40, 0x9901, 0x59C0, 0x5880, 0x9841,
-        0x8801, 0x48C0, 0x4980, 0x8941, 0x4B00, 0x8BC1, 0x8A81, 0x4A40, 0x4E00, 0x8EC1, 0x8F81, 0x4F40, 0x8D01, 0x4DC0,
-        0x4C80, 0x8C41, 0x4400, 0x84C1, 0x8581, 0x4540, 0x8701, 0x47C0, 0x4680, 0x8641, 0x8201, 0x42C0, 0x4380, 0x8341,
-        0x4100, 0x81C1, 0x8081, 0x4040
-
-    };
+    const CFE_ES_ComputeCRC_Params_t *CrcParams;
+    uint32                            CrcResult;
 
+    /* Protect against bad pointer input - historical behavior is to just return the input */
     if (DataPtr == NULL || DataLength == 0)
     {
-        return InputCRC;
+        CrcResult = InputCRC;
     }
-
-    switch (TypeCRC)
+    else
     {
-        case CFE_ES_CrcType_CRC_32:
-            CFE_ES_WriteToSysLog("%s: Calculate CRC32 not Implemented\n", __func__);
-            break;
-
-        case CFE_ES_CrcType_CRC_16:
-            Crc    = (int16)(0xFFFF & InputCRC);
-            BufPtr = (const uint8 *)DataPtr;
-
-            for (i = 0; i < DataLength; i++, BufPtr++)
-            {
-                /*
-                 * It is assumed that the supplied buffer is in a
-                 * directly-accessible memory space that does not
-                 * require special logic to access
-                 */
-                ByteValue = *BufPtr;
-                Index     = ((Crc ^ ByteValue) & 0x00FF);
-                Crc       = ((Crc >> 8) & 0x00FF) ^ CrcTable[Index];
-            }
-            break;
-
-        case CFE_ES_CrcType_CRC_8:
-            CFE_ES_WriteToSysLog("%s: Calculate CRC8 not Implemented\n", __func__);
-            break;
-
-        default:
-            break;
+        /* This always returns a valid object, even if it is not implemented */
+        CrcParams = CFE_ES_ComputeCRC_GetParams(TypeCRC);
+        CrcResult = CrcParams->Algorithm(DataPtr, DataLength, InputCRC);
     }
-    return Crc;
+
+    return CrcResult;
 }
 
 /*----------------------------------------------------------------