Fix #2174, Move CRC types and convert to enum #2192

thnkslprpt · 2022-10-31T10:44:59Z

Checklist

I reviewed the Contributing Guide.
I signed and emailed the appropriate Contributor License Agreement to GSFC-SoftwareRelease@mail.nasa.gov and copied cfs-program@lists.nasa.gov.

Describe the contribution
Fixes #2174
The macro definitions of the CRC types (8, 16, and 32-bit CRC polynomials) have been moved to cfe_es_api_typedefs.h and turned into a typedef'd enum.
The previous #define's have been left in with deprecation directives added around them, due to dependencies of the FM app.

Testing performed
GitHub CI actions (incl. Build + Run, Unit Tests etc.) all passing successfully.

Expected behavior changes
Essentially no change to logic.

Note - Two new issues will need to be opened if/when this is merged:
FM: directly references the types that have been changed here.
CS: refers to (in comments only) CFE_MISSION_ES_CRC_16

Contributor Info
Avi Weiss @thnkslprpt

modules/es/fsw/src/cfe_es_api.c

jphickey

A couple modifications requested here.

Note - this is not just to be pedantic, there is a reason behind both the naming convention as well as putting the things at the right scope and in the right file and named the right way, because eventually some of these headers will become generated, so the names and content needs to match to allow that transition. That was really the reason this change was needed in the first place.

docs/cFE Application Developers Guide.md

modules/cfe_testcase/src/es_misc_test.c

modules/core_api/fsw/inc/cfe_es_api_typedefs.h

jphickey

Getting there... but we need to revert back the rest of the CFE_MISSION_ES_DEFAULT_CRC references. I'd back this changeset off to changing only what directly refers to the enum labels (that is, the implementation of the CRC function itself, and the headers that define the labels).

In addition to keeping this change smaller and simpler, not changing other refs also builds some confidence - if the various places that this function is invoked inside CFE core modules (particularly including unit and functional tests) don't have to change, we can be relatively sure that calls from external apps outside CFE core modules probably don't have to change either.

cmake/sample_defs/sample_mission_cfg.h

modules/core_api/fsw/inc/cfe_es_api_typedefs.h

modules/tbl/fsw/src/cfe_tbl_task_cmds.c

modules/es/ut-coverage/es_UT.c

modules/es/fsw/src/cfe_es_task.c

modules/es/fsw/src/cfe_es_api.c

@@ -1571,7 +1571,7 @@
 * See description in header file for argument/return detail
 *
 *-----------------------------------------------------------------*/
-uint32 CFE_ES_CalculateCRC(const void *DataPtr, size_t DataLength, uint32 InputCRC, uint32 TypeCRC)
+uint32 CFE_ES_CalculateCRC(const void *DataPtr, size_t DataLength, uint32 InputCRC, CFE_ES_CrcType_Enum_t TypeCRC)


modules/es/fsw/src/cfe_es_api.c

@@ -1571,7 +1571,7 @@
 * See description in header file for argument/return detail
 *
 *-----------------------------------------------------------------*/
-uint32 CFE_ES_CalculateCRC(const void *DataPtr, size_t DataLength, uint32 InputCRC, uint32 TypeCRC)
+uint32 CFE_ES_CalculateCRC(const void *DataPtr, size_t DataLength, uint32 InputCRC, CFE_ES_CrcType_Enum_t TypeCRC)


jphickey

Real close now, just a couple minor details still remaining

modules/cfe_testcase/src/es_misc_test.c

modules/core_api/fsw/inc/cfe_es_api_typedefs.h

jphickey

Reviewed again, but I've come to 2 realizations about this:

Unfortunately, the FM app refers to the other (non-implemented) CRC types using the existing names. That means this change breaks the build of FM, which is a problem. To get around that we would have to provide both the old and new names (which can be #define'd) and follow the deprecation procedure.
We should probably back out the "TypeCRC" local variable in the test cases, too. Reason is that it makes cross referencing the test cases more difficult. That is, in the log it would previously show the call along with the symbolic name of the last parameter, and we can indicate in our test reports which test case(s) covered those functions. They are still being covered, of course, but the log now just shows "TypeCRC" for all the calls, so its not clear which call is covering which option, which complicates the reporting/verification that we need to do.

I might be able to update both of those things myself, because this is something I would like to get merged for other reasons.

thnkslprpt · 2022-11-30T02:24:41Z

@jphickey

Thanks Joe. Now I understand when deprecation is required.

I had a go at the changes you described.
Happy to leave these changes to you though if it will be too much back-and-forth. I know you guys are (very) busy. Of if you like to keep educating me ;-) I'd be happy with that as well. Just let me know mate.
Cheers

jphickey

Perfect, thanks for doing all those changes. This looks good to me now, will review with the CFS CCB.

modules/core_api/ut-stubs/src/cfe_es_stubs.c

@@ -77,14 +77,14 @@
 * Generated stub function for CFE_ES_CalculateCRC()
 * ----------------------------------------------------
 */
-uint32 CFE_ES_CalculateCRC(const void *DataPtr, size_t DataLength, uint32 InputCRC, uint32 TypeCRC)
+uint32 CFE_ES_CalculateCRC(const void *DataPtr, size_t DataLength, uint32 InputCRC, CFE_ES_CrcType_Enum_t TypeCRC)


modules/core_api/ut-stubs/src/cfe_es_stubs.c

@@ -77,14 +77,14 @@
 * Generated stub function for CFE_ES_CalculateCRC()
 * ----------------------------------------------------
 */
-uint32 CFE_ES_CalculateCRC(const void *DataPtr, size_t DataLength, uint32 InputCRC, uint32 TypeCRC)
+uint32 CFE_ES_CalculateCRC(const void *DataPtr, size_t DataLength, uint32 InputCRC, CFE_ES_CrcType_Enum_t TypeCRC)


thnkslprpt · 2022-12-01T00:38:28Z

OK no worries @jphickey.
Squashed the commits and added a couple touch-ups (synced up the function prototypes/declarations).

FM we already know will need updates given that it directly references the old CRC types/names.

Are the MM and CS apps which call CFE_ES_CalculateCRC() fine to leave as-is given that for the TypeCRC parameter they either:
a) call it with integers (which won't break anything) or,
b) use CFE_MISSION_ES_DEFAULT_CRC (which we're not removing)

*Combines:* cFE v7.0.0-rc4+dev242 t0_lab v2.5.0-rc4+dev35 **Includes:** *cFE* - nasa/cFE#2231 - nasa/cFE#2229 - nasa/cFE#2192 *to_lab* - nasa/to_lab#142 Co-authored by: Daniel Knutsen <dmknutsen@users.noreply.github.com> Co-authored by: Justin Figueroa <chillfig@users.noreply.github.com> Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com> Co-authored by: Avi Weiss: <thnkslprpt@users.noreply.github.com>

*Combines:* cFE v7.0.0-rc4+dev242 to_lab v2.5.0-rc4+dev35 **Includes:** *cFE* - nasa/cFE#2231 - nasa/cFE#2229 - nasa/cFE#2192 *to_lab* - nasa/to_lab#142 Co-authored by: Daniel Knutsen <dmknutsen@users.noreply.github.com> Co-authored by: Justin Figueroa <chillfig@users.noreply.github.com> Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com> Co-authored by: Avi Weiss: <thnkslprpt@users.noreply.github.com>

github-advanced-security bot found potential problems Oct 31, 2022

View reviewed changes

modules/es/fsw/src/cfe_es_api.c Fixed Show fixed Hide fixed

modules/es/fsw/src/cfe_es_api.c Fixed Show fixed Hide fixed

thnkslprpt force-pushed the fix-2174-move-crc-types-into-enum branch from 0538aa6 to 90437e0 Compare November 2, 2022 10:27

dzbaker requested a review from jphickey November 3, 2022 18:45

jphickey requested changes Nov 3, 2022

View reviewed changes

docs/cFE Application Developers Guide.md Show resolved Hide resolved

modules/cfe_testcase/src/es_misc_test.c Outdated Show resolved Hide resolved

modules/core_api/fsw/inc/cfe_es_api_typedefs.h Show resolved Hide resolved

thnkslprpt requested a review from jphickey November 3, 2022 22:48

jphickey requested changes Nov 4, 2022

View reviewed changes

thnkslprpt force-pushed the fix-2174-move-crc-types-into-enum branch 2 times, most recently from d0b0850 to bb81cf8 Compare November 5, 2022 06:38

github-advanced-security bot found potential problems Nov 5, 2022

View reviewed changes

thnkslprpt force-pushed the fix-2174-move-crc-types-into-enum branch from bb81cf8 to 561957f Compare November 5, 2022 06:43

thnkslprpt requested a review from jphickey November 5, 2022 06:47

jphickey requested changes Nov 14, 2022

View reviewed changes

modules/cfe_testcase/src/es_misc_test.c Outdated Show resolved Hide resolved

modules/core_api/fsw/inc/cfe_es_api_typedefs.h Show resolved Hide resolved

thnkslprpt force-pushed the fix-2174-move-crc-types-into-enum branch from 561957f to dd8c8f0 Compare November 14, 2022 23:20

thnkslprpt requested a review from jphickey November 14, 2022 23:23

jphickey requested changes Nov 29, 2022

View reviewed changes

thnkslprpt force-pushed the fix-2174-move-crc-types-into-enum branch from 6f5b744 to 95dd8dd Compare November 30, 2022 02:18

jphickey approved these changes Nov 30, 2022

View reviewed changes

jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Nov 30, 2022

Fix nasa#2174, Move CRC types and convert to enum

ed4722b

thnkslprpt force-pushed the fix-2174-move-crc-types-into-enum branch from 95dd8dd to ed4722b Compare December 1, 2022 00:30

github-advanced-security bot found potential problems Dec 1, 2022

View reviewed changes

dzbaker added this to the Equuleus milestone Dec 15, 2022

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 Dec 15, 2022

dzbaker mentioned this pull request Dec 20, 2022

IC: Caelum-rc4+dev33 nasa/cFS#634

Merged

2 tasks

dzbaker merged commit d666214 into nasa:main Dec 20, 2022

thnkslprpt deleted the fix-2174-move-crc-types-into-enum branch December 20, 2022 21:19

dmknutsen modified the milestones: Equuleus, Draco Jan 18, 2023

dmknutsen added the draco-rc4 label Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2174, Move CRC types and convert to enum #2192

Fix #2174, Move CRC types and convert to enum #2192

thnkslprpt commented Oct 31, 2022 •

edited

jphickey left a comment

jphickey left a comment

jphickey left a comment

jphickey left a comment

thnkslprpt commented Nov 30, 2022 •

edited

jphickey left a comment

thnkslprpt commented Dec 1, 2022 •

edited

Fix #2174, Move CRC types and convert to enum #2192

Fix #2174, Move CRC types and convert to enum #2192

Conversation

thnkslprpt commented Oct 31, 2022 • edited

jphickey left a comment

Choose a reason for hiding this comment

jphickey left a comment

Choose a reason for hiding this comment

jphickey left a comment

Choose a reason for hiding this comment

jphickey left a comment

Choose a reason for hiding this comment

thnkslprpt commented Nov 30, 2022 • edited

jphickey left a comment

Choose a reason for hiding this comment

thnkslprpt commented Dec 1, 2022 • edited

thnkslprpt commented Oct 31, 2022 •

edited

thnkslprpt commented Nov 30, 2022 •

edited

thnkslprpt commented Dec 1, 2022 •

edited