-
Notifications
You must be signed in to change notification settings - Fork 202
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 #2174, Move CRC types and convert to enum #2192
Fix #2174, Move CRC types and convert to enum #2192
Conversation
0538aa6
to
90437e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
d0b0850
to
bb81cf8
Compare
@@ -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) |
Check notice
Code scanning / CodeQL-coding-standard
Long function without assertion
@@ -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) |
Check notice
Code scanning / CodeQL-coding-standard
Function too long
bb81cf8
to
561957f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Real close now, just a couple minor details still remaining
561957f
to
dd8c8f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
6f5b744
to
95dd8dd
Compare
Thanks Joe. Now I understand when deprecation is required. I had a go at the changes you described. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks for doing all those changes. This looks good to me now, will review with the CFS CCB.
95dd8dd
to
ed4722b
Compare
@@ -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) |
Check notice
Code scanning / CodeQL-coding-standard
Long function without assertion
@@ -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) |
Check notice
Code scanning / CodeQL-coding-standard
Function too long
OK no worries @jphickey. 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 |
*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>
Checklist
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'denum
.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