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_UnionArgs_Payload_t elements dword and hword not used, union not necessary #288

Open
3 tasks done
skliper opened this issue Jul 21, 2022 · 1 comment · Fixed by #341 · May be fixed by #400
Open
3 tasks done

CF_UnionArgs_Payload_t elements dword and hword not used, union not necessary #288

skliper opened this issue Jul 21, 2022 · 1 comment · Fixed by #341 · May be fixed by #400

Comments

@skliper
Copy link
Contributor

skliper commented Jul 21, 2022

Checklist (Please check before submitting)

  • I reviewed the Contributing Guide.
  • I reviewed the CF README.md file to see if the feature is in the major future work.
  • I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
dword and hword are not used in the CF_UnionArgs_Payload_t union, only byte:

CF/fsw/src/cf_msg.h

Lines 159 to 169 in 0774746

/* unionargs -
*
* A lot of commands have a single byte arg, and the old application had 3 spare bytes after that.
*
* So this structure gives the flexibility of a single byte arg, 2 halfwords, or a dword, all in one declaration. */
typedef union CF_UnionArgs_Payload
{
uint32 dword;
uint16 hword[2];
uint8 byte[4];
} CF_UnionArgs_Payload_t;

Describe the solution you'd like
Remove unnecessary union

Describe alternatives you've considered
CF_UnionArgsCmd_t is somewhat out of family in that it specifies unnecessary payload elements for some of the commands. The only common benefit seems to be byte 0 which is channel, supporting all channels generically with 255. Consider untangling all the layers of abstraction and go back to simple command processing.

Additional context
None

Requester Info
Jacob Hageman - NASA/GSFC

skliper added a commit to skliper/CF that referenced this issue Jul 27, 2022
dzbaker added a commit that referenced this issue Mar 13, 2023
…ayload_t-union

Fix #288, Remove unnecessary CF_UnionArgs_Payload_t union
chillfig pushed a commit to chillfig/CF that referenced this issue Jul 11, 2023
@chillfig
Copy link
Contributor

Initial PR solving this issue is reverted by #395. Reopening to be considered for Equuleus-rc2.

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