-
Notifications
You must be signed in to change notification settings - Fork 206
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 #488, Remove compiler added padding in tlm packets #662
Conversation
CCB-20200506 - Will conflict with some 6.8 changes so we'll need to handle that. Related discussion on #678 . |
uint8 tempPad1[4]; | ||
#elif ( CCSDS_TIME_SIZE == 8 ) | ||
uint8 tempPad1[2]; | ||
#endif |
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.
Padding shouldn't be added this way -- it isn't a fixed quantity. Furthermore the "ccsds.h" definition should be the source of truth -- that is, it should be the actual definition of the structure without getting into any architecture-specific padding requirements.
The other reason not to do it this way is that the opposite problem exists when extended headers is enabled. This adds 4 bytes to the primary, so now the size of TLM hdr becomes 16 and the CMD header becomes 12, so now CMDs will need padding and TLM packets will not.
The paradigm should be that ccsds.h
definitions represent the basic fundamental primitives, and cfe_sb.h
structures represent how these are actually realized for use within the software bus and applications.
So - in the related PR #678 the CFE_SB_TlmHdr_t
becomes a padded and aligned version of CCSDS_TlmSecHdr_t
One can reliably determine the actual amount of padding on the current system by checking sizeof(CFE_SB_TlmHdr_t) - sizeof(CCSDS_TlmSecHdr_t)
or equivalent...
This will work on 32 bit or 64 bit, with or without extended headers, and should continue to work even if the headers are changed in the future.
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.
or what about using a union, like:
typedef union { uint8 Time[CCSDS_TIME_SIZE], TotalSz[10] } CCSDS_TlmSecHdr_t;
Marking as duplicate and closing for now, can reinvestigate as part of future packet refactor for the padding within packets. |
Describe the contribution
Fix #488
Removes compiler added padding by padding CCSDS_TlmSecHdr_t such that the payload will always start on a 64-bit boundary. Also adjusts packet alignment such that packets with 64 bit variables are aligned correctly.
Related to nasa/cFS-GroundSystem#81
Intended to be delivered with nasa/cFS-GroundSystem#82
Testing performed
Steps taken to test the contribution:
Expected behavior changes
Compiler added padding will no longer be applied to packets.
System(s) tested on
Oracle VM VirtualBox
OS: ubuntu-19.10
Versions: cFE 6.7.14.0, OSAL 5.0.13.0, PSP 1.4.9.0
Contributor Info - All information REQUIRED for consideration of pull request
Dan Knutsen
NASA/Goddard