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.

Sign up for GitHub

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

Jump to bottom

Fix #488, Remove compiler added padding in tlm packets #662

Closed
wants to merge 1 commit into from
Closed

Fix #488, Remove compiler added padding in tlm packets #662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions fsw/cfe-core/src/inc/ccsds.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,12 @@ typedef struct {

uint8 Time[CCSDS_TIME_SIZE];

#if ( CCSDS_TIME_SIZE == 6 )
uint8 tempPad1[4];
#elif ( CCSDS_TIME_SIZE == 8 )
uint8 tempPad1[2];
#endif
Copy link
Contributor

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.

Copy link
Contributor

@CDKnightNASA CDKnightNASA May 26, 2020 •
edited
Loading

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;


} CCSDS_TlmSecHdr_t;

/*----- CCSDS Endian Flag in the APID Qualifier Field. -----*/
Expand Down
5 changes: 3 additions & 2 deletions fsw/cfe-core/src/inc/cfe_es_msg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,8 +1412,8 @@ typedef struct
**/
typedef struct
{
char Application[CFE_MISSION_MAX_API_LEN]; /**< \brief - RESERVED - should be all zeroes */
CFE_ES_MemHandle_t PoolHandle; /**< \brief Handle of Pool whose statistics are to be telemetered */
char Application[CFE_MISSION_MAX_API_LEN]; /**< \brief Application Name */
CFE_ES_MemHandle_t PoolHandle; /**< \brief Handle of Pool whose statistics are to be telemetered */

} CFE_ES_SendMemPoolStatsCmd_Payload_t;

Expand Down Expand Up @@ -1469,6 +1469,7 @@ typedef struct
CFE_ES_MemHandle_t PoolHandle; /**< \cfetlmmnemonic \ES_POOLHANDLE
\brief Handle of memory pool whose stats are being telemetered */
CFE_ES_MemPoolStats_t PoolStats; /**< \brief For more info, see #CFE_ES_MemPoolStats_t */
uint32 AlignPad; /**< \brief Spare word to maintain alignment */
} CFE_ES_PoolStatsTlm_Payload_t;

typedef struct
Expand Down
6 changes: 3 additions & 3 deletions fsw/cfe-core/src/inc/cfe_tbl_msg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ typedef struct
*/
uint8 NumFreeSharedBufs; /**< \cfetlmmnemonic \TBL_NUMFREESHRBUF
\brief Number of free Shared Working Buffers */
uint8 ByteAlignPad1; /**< \cfetlmmnemonic \TBL_BYTEALIGNPAD1
uint8 ByteAlignPad1[3]; /**< \cfetlmmnemonic \TBL_BYTEALIGNPAD1
\brief Spare byte to ensure longword alignment */
CFE_ES_MemHandle_t MemPoolHandle; /**< \cfetlmmnemonic \TBL_MEMPOOLHANDLE
\brief Handle to TBL's memory pool */
Expand Down Expand Up @@ -794,7 +794,7 @@ typedef struct
\brief Flag indicating an inactive buffer is ready to be copied */
bool DumpOnly; /**< \cfetlmmnemonic \TBL_DUMPONLY
\brief Flag indicating Table is NOT to be loaded */
bool DoubleBuffered; /**< \cfetlmmnemonic \TBL_DBLBUFFERED
bool DoubleBuffered; /**< \cfetlmmnemonic \TBL_DBLBUFFERED
\brief Flag indicating Table has a dedicated inactive buffer */
char Name[CFE_MISSION_TBL_MAX_FULL_NAME_LEN];/**< \cfetlmmnemonic \TBL_NAME
\brief Processor specific table name */
Expand All @@ -804,7 +804,7 @@ typedef struct
\brief Name of owning application */
bool Critical; /**< \cfetlmmnemonic \TBL_CRITICAL
\brief Indicates whether table is Critical or not */
uint8 ByteAlign4; /**< \cfetlmmnemonic \TBL_SPARE4
uint8 ByteAlign4[7]; /**< \cfetlmmnemonic \TBL_SPARE4
\brief Spare byte to maintain byte alignment */
} CFE_TBL_TblRegPacket_Payload_t;

Expand Down