Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #2449, crc calculation refactor #2450

Merged
merged 1 commit into from Oct 12, 2023
Merged

Conversation

jphickey
Copy link
Contributor

Checklist (Please check before submitting)

Describe the contribution
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.

Fixes #2449

Testing performed
Build and run coverage test and functional test

Expected behavior changes
CRC algorithms are better documented, code better abstracted in a separate source file to more easily permit additional routines to be implemented in the future.

Functional test cases are fixed and should be comparable to 3rd party implementation of the same algorithm.

System(s) tested on
Debian

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

modules/es/fsw/src/cfe_es_crc.c Fixed Show fixed Hide fixed
return 0;
}

uint32 CFE_ES_ComputeCRC_Algo_16_ARC(const void *DataPtr, size_t DataLength, uint32 InputCRC)

Check notice

Code scanning / CodeQL-coding-standard

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
modules/es/fsw/src/cfe_es_api.c Fixed Show fixed Hide fixed
modules/es/fsw/src/cfe_es_crc.h Fixed Show fixed Hide fixed
@jphickey
Copy link
Contributor Author

Sample of functional test output after this fix.

[BEGIN] 22 Test Calculate CRC
[ INFO] es_misc_test.c:39:Testing: CFE_ES_CalculateCRC
[ PASS] 22.001 es_misc_test.c:42 - CFE_ES_CalculateCRC(CRC_CHECK_INPUT, sizeof(CRC_CHECK_INPUT) - 1, 1, CFE_ES_CrcType_NONE) (0) == 0 (0)
[ PASS] 22.002 es_misc_test.c:45 - CFE_ES_CalculateCRC(CRC_CHECK_INPUT, sizeof(CRC_CHECK_INPUT) - 1, 1, CFE_ES_CrcType_MAX + 1) (0) == 0 (0)
[ PASS] 22.003 es_misc_test.c:48 - CFE_ES_CalculateCRC(CRC_CHECK_INPUT, sizeof(CRC_CHECK_INPUT) - 1, 0, CFE_ES_CrcType_16_ARC) (47933) == 0xBB3D (47933)
[ PASS] 22.004 es_misc_test.c:55 - CFE_ES_CalculateCRC(CRC_CHECK_INPUT, sizeof(CRC_CHECK_INPUT) - 1, 0, CFE_MISSION_ES_DEFAULT_CRC) (47933) == 0xBB3D (47933)
[ PASS] 22.005 es_misc_test.c:58 - CFE_ES_CalculateCRC(CRC_OTHER_INPUT, sizeof(CRC_OTHER_INPUT) - 1, 0, CFE_ES_CrcType_16_ARC) (4579) == 0x11E3 (4579)
[ PASS] 22.006 es_misc_test.c:61 - CFE_ES_CalculateCRC(CRC_CHECK_INPUT, sizeof(CRC_CHECK_INPUT) - 1, 345353, CFE_ES_CrcType_16_ARC) (58515) == 0xE493 (58515)
[ PASS] 22.007 es_misc_test.c:64 - CFE_ES_CalculateCRC(NULL, sizeof(CRC_OTHER_INPUT), inputCrc, CFE_ES_CrcType_16_ARC) (345353) == inputCrc (345353)
[ PASS] 22.008 es_misc_test.c:65 - CFE_ES_CalculateCRC(CRC_OTHER_INPUT, 0, inputCrc, CFE_ES_CrcType_16_ARC) (345353) == inputCrc (345353)
[ INFO]                        ABORT::0     WARN::0     FLOW::0     DEBUG::0     N/A::0   
[  END] 22 Test Calculate CRC   TOTAL::8     PASS::8     FAIL::0     MIR::0     TSF::0     TTF::0   

Notably, CRC values in the range of 0x8000 - 0xFFFF are returned correctly now. Previously, they were reported as a 32-bit sign-extended value, e.g. 0xFFFFE493 (4294960275) instead of 0xE493 (58515). Although the lower 16 bits still matched, a user would have to mask those out to check it. Now, it matches properly.

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.
return Crc;
}

CFE_ES_ComputeCRC_Params_t *CFE_ES_ComputeCRC_GetParams(CFE_ES_CrcType_Enum_t CrcType)

Check notice

Code scanning / CodeQL-coding-standard

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
#include "common_types.h"
#include "cfe_es.h"

typedef uint32 (*const CFE_ES_ComputeCRC_Algo_t)(const void *DataPtr, size_t DataLength, uint32 InputCRC);

Check notice

Code scanning / CodeQL-coding-standard

Hidden pointer indirection Note

The typedef CFE_ES_ComputeCRC_Algo_t hides pointer indirection.
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Oct 5, 2023
@dzbaker dzbaker added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Oct 5, 2023
dzbaker added a commit to nasa/cFS that referenced this pull request Oct 12, 2023
*Combines:*

cFE v7.0.0-rc4+dev395
osal v6.0.0-rc4+dev239

**Includes:**

*cFE*
- nasa/cFE#2440
- nasa/cFE#2450

*osal*
- nasa/osal#1422

Co-authored by: Justin Figueroa <chillfig@users.noreply.github.com>
Co-authored by: Jacob Hageman  <skliper@users.noreply.github.com>
Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
@dzbaker dzbaker mentioned this pull request Oct 12, 2023
2 tasks
@dzbaker dzbaker merged commit ab18ec1 into nasa:main Oct 12, 2023
22 checks passed
dzbaker added a commit to nasa/cFS that referenced this pull request Oct 12, 2023
*Combines:*

cFE v7.0.0-rc4+dev395
osal v6.0.0-rc4+dev239

**Includes:**

*cFE*
- nasa/cFE#2440
- nasa/cFE#2450

*osal*
- nasa/osal#1422

Co-authored by: Justin Figueroa <chillfig@users.noreply.github.com>
Co-authored by: Jacob Hageman  <skliper@users.noreply.github.com>
Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
@jphickey jphickey deleted the fix-2449-crc-calc branch November 16, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CRC calculation does not match expected values in functional test
2 participants