Fix #666, alignment of CMD/TLM message definitions #678
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The "CFE_SB_CmdHdr_t" and "CFE_SB_TlmHdr_t" types were not defined such that they would have compatible alignment with (and thereby allow safe casting to/from) a CFE_SB_Msg_t type. This changes the definition to be a union so that the types are aligned correctly.
Update the CFE_ES_ShellTlm_t, CFE_TIME_ToneSignalCmd_t, and CFE_TIME_FakeToneCmd_t to use the CFE_SB_TlmHdr_t/CFE_SB_CmdHdr_t types to define the buffer, rather than a uint8 array. This should not change the size, as it was already defined using sizeof() this structure, but it will make it aligned correctly which resolves the compiler warning. Note that all CMD/TLM should really be defined this way, but this only selectively changes the places that were actually generating a compiler warning about this. There is a risk that padding will be added, but this change should not change the padding or size of messages in 32-bit builds.
skliper
approved these changes
May 6, 2020
|
Also note this PR only fixes warnings within the FSW code. I'm still seeing several alignment cast warnings in the unit test code on my 32-bit test platform. Will submit a separate issue about that. |
skliper
added
the
CCB:Ready
|
Note that this also fixes #313 as it changes the CFE_ES_ShellTlm_t definition to be aligned. Even though this is on its way to being deprecated/optional this was generating a warning and causing the build to fail. |
|
Submitted #679 for the issues in unit test code. |
skliper
removed
the
CCB:Ready
|
CCB 20200506 - APPROVED |
astrogeco
added
CCB:Approved
|
@jphickey fyi made a mistake and merged to master directly. I |
astrogeco
added a commit
that referenced
this pull request
May 8, 2020
Fix #666, alignment of CMD/TLM message definitions
This was referenced May 8, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe the contribution
Define the SB message headers such that they are aligned appropriately for casting to/from a
CFE_SB_Msg_ttype. Note this should not affect the size of these structures on a 32-bit machine with the default configuration, as they are already multiples ofsizeof(uint32)....Then change only the CMD/TLM definitions which were generating warnings on the 32-bit build to use this correct definition, rather than a
uint8[]array to reserve space for the header. Again in these cases this should not change the size or padding because it already was multiples ofsizeof(uint32). This just makes it so the compiler will appropriately align the overall buffer.Fix #666
Testing performed
Build for 64 + 32 bit platforms with strict alignment settings and confirm that the warnings are gone. Confirm all tests pass.
Expected behavior changes
No impact to behavior.
System(s) tested on
Ubuntu 20.04
MIPS yocto/poky embedded Linux (32-bit big endian processor with strict alignment requirements)
RTEMS on QEMU (i686-rtems4.11)
Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.