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

CF "cf_field.h" macro cleanup #65

Closed
jphickey opened this issue Nov 23, 2021 · 1 comment · Fixed by #182
Closed

CF "cf_field.h" macro cleanup #65

jphickey opened this issue Nov 23, 2021 · 1 comment · Fixed by #182
Milestone

Comments

@jphickey
Copy link
Contributor

This issue was imported from the GSFC issue tracking system

Imported from: [GSFCCFS-1802] CF "cf_field.h" macro cleanup
Originally submitted by: Hickey, Joseph P. (GSFC-582.0)[VANTAGE SYSTEMS INC] on Wed Nov 17 11:13:49 2021

Original Description:
The "cf_field.h" defines a macro called "inc_subfield" .... thankfully this appears to not be used anywhere. Recommend removal.

The other macros "FIELD_SET_VAL" and "FIELD_GET_VAL" have a couple recommended cleanups:

1.They are multi-line/complex macros. Recommend to convert to inline functions instead - they only operate on "uint8" base values so this should not be a problem.
2. They are then aliased to a shorthand "FSV" and "FGV", respectively. It appears the author simply didn't want to write the long names? Is there a real justification for having the same macro/function under two names? Only the shorthand is actually used in the code. Recommend to choose one name and use it.

@jphickey
Copy link
Contributor Author

jphickey commented Jan 6, 2022

After PR #137 is merged, this consolidates use of fields to only the Encode/Decode routines.

Recommendation is to further move FSV/FGV and the related field info into the codec file (removing it from cf_cfdp_pdu.h). This will more appropriately scope this info, and fix the remaining issues here. Nothing outside of cf_codec.c should care about these bit fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants