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

CFE_MSG_Message_t should really be a struct, not a union #2514

Closed
jphickey opened this issue Feb 13, 2024 · 2 comments · Fixed by #2515
Closed

CFE_MSG_Message_t should really be a struct, not a union #2514

jphickey opened this issue Feb 13, 2024 · 2 comments · Fixed by #2515

Comments

@jphickey
Copy link
Contributor

Is your feature request related to a problem? Please describe.
The cfe_msg_api_typedefs.h file defines the CFE_MSG_Message_t typedef as a union:

typedef union CFE_MSG_Message CFE_MSG_Message_t;

In reality, users should not be accessing the object directly at all. All access should be abstracted via the MSG API.

Describe the solution you'd like
At the generic API level (abstract type definition), this should really be a struct like the Command and Telemetry headers are. Users of this object should not treat it as anything other an an opaque blob.

Describe alternatives you've considered
N/A

Additional context
Historically, there were two reasons that it was originally a union:

  1. To enforce some type of alignment (e.g. long word) by including an alignment field within the union
  2. To access it as raw bytes/octets for the purpose of e.g. computing a checksum.

Item 1 is obsolete for two reasons:
a) It is already aligned when instantiated as part of a CFE_SB_Buffer_t encapsulating object within SB. There does not need to be alignment in the message type itself
b) Aligning in this way wasn't sufficient and/or didn't meet mission requirements as it constituted compiler padding if it was ever relied upon. In most use cases, structures used for inter-process communication must be specifically designed such that they do not include any padding. For example: if a header was nominally 12 bytes long but needed 64-bit alignment, the accepted approach is to explicitly include a "spare" fields (either uint8[4] or single uint32). It should not be done via this method of allowing the compiler to pad it out.

Item 2 is invalid because users (as in, apps and libraries) should never access the message as raw bytes. The checksum implementation is strictly done by the MSG module itself. Therefore, the MSG module itself (internally) should be the only thing that accesses the message in this manner. Although this could be through a union (and that would be fine) that aspect should be hidden from the public API.

Simply changing this to a struct shouldn't impact any app code, because the typedef stays the same and nothing should be relying on it being a union. (aside from unit tests and the MSG module itself).

Requester Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey
Copy link
Contributor Author

Pinging @skliper as a heads up. This is a trivial change to the header and only needed minimal updates to UT to make it work. I will be submitting a PR, but wanted to get your attention in case you know of any cases that there is a real need to have this as a union instead of a struct.

@skliper
Copy link
Contributor

skliper commented Feb 13, 2024

Concur, I agree the union was for historical/obsolete reasons.

dzbaker added a commit that referenced this issue Feb 22, 2024
Fix #2514, change CFE_MSG_Message from union to struct
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants