From 4908b722b61a54b054ee6d3440ea4580d3854593 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Wed, 6 May 2020 10:42:53 -0400 Subject: [PATCH 1/2] Issue #666, Define aligned message headers 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. --- fsw/cfe-core/src/inc/cfe_sb.h | 10 ++++++++-- fsw/cfe-core/src/sb/cfe_sb_util.c | 30 +++++++++++++++--------------- fsw/cfe-core/unit-test/sb_UT.c | 2 +- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/fsw/cfe-core/src/inc/cfe_sb.h b/fsw/cfe-core/src/inc/cfe_sb.h index 522ac5725..0b515e83b 100644 --- a/fsw/cfe-core/src/inc/cfe_sb.h +++ b/fsw/cfe-core/src/inc/cfe_sb.h @@ -155,10 +155,16 @@ typedef union { }CFE_SB_Msg_t; /** \brief Generic Software Bus Command Header Type Definition */ -typedef CCSDS_CommandPacket_t CFE_SB_CmdHdr_t; +typedef union { + CCSDS_CommandPacket_t Cmd; + CFE_SB_Msg_t BaseMsg; /**< Base type (primary header) */ +} CFE_SB_CmdHdr_t; /** \brief Generic Software Bus Telemetry Header Type Definition */ -typedef CCSDS_TelemetryPacket_t CFE_SB_TlmHdr_t; +typedef union { + CCSDS_TelemetryPacket_t Tlm; + CFE_SB_Msg_t BaseMsg; /**< Base type (primary header) */ +} CFE_SB_TlmHdr_t; #define CFE_SB_CMD_HDR_SIZE (sizeof(CFE_SB_CmdHdr_t))/**< \brief Size of #CFE_SB_CmdHdr_t in bytes */ #define CFE_SB_TLM_HDR_SIZE (sizeof(CFE_SB_TlmHdr_t))/**< \brief Size of #CFE_SB_TlmHdr_t in bytes */ diff --git a/fsw/cfe-core/src/sb/cfe_sb_util.c b/fsw/cfe-core/src/sb/cfe_sb_util.c index ab49485d8..abef9d505 100644 --- a/fsw/cfe-core/src/sb/cfe_sb_util.c +++ b/fsw/cfe-core/src/sb/cfe_sb_util.c @@ -211,21 +211,21 @@ CFE_TIME_SysTime_t CFE_SB_GetMsgTime(CFE_SB_MsgPtr_t MsgPtr) #if (CFE_MISSION_SB_PACKET_TIME_FORMAT == CFE_MISSION_SB_TIME_32_16_SUBS) - memcpy(&LocalSecs32, &TlmHdrPtr->Sec.Time[0], 4); - memcpy(&LocalSubs16, &TlmHdrPtr->Sec.Time[4], 2); + memcpy(&LocalSecs32, &TlmHdrPtr->Tlm.Sec.Time[0], 4); + memcpy(&LocalSubs16, &TlmHdrPtr->Tlm.Sec.Time[4], 2); /* convert packet data into CFE_TIME_SysTime_t format */ LocalSubs32 = ((uint32) LocalSubs16) << 16; #elif (CFE_MISSION_SB_PACKET_TIME_FORMAT == CFE_MISSION_SB_TIME_32_32_SUBS) - memcpy(&LocalSecs32, &TlmHdrPtr->Sec.Time[0], 4); - memcpy(&LocalSubs32, &TlmHdrPtr->Sec.Time[4], 4); + memcpy(&LocalSecs32, &TlmHdrPtr->Tlm.Sec.Time[0], 4); + memcpy(&LocalSubs32, &TlmHdrPtr->Tlm.Sec.Time[4], 4); /* no conversion necessary -- packet format = CFE_TIME_SysTime_t format */ #elif (CFE_MISSION_SB_PACKET_TIME_FORMAT == CFE_MISSION_SB_TIME_32_32_M_20) - memcpy(&LocalSecs32, &TlmHdrPtr->Sec.Time[0], 4); - memcpy(&LocalSubs32, &TlmHdrPtr->Sec.Time[4], 4); + memcpy(&LocalSecs32, &TlmHdrPtr->Tlm.Sec.Time[0], 4); + memcpy(&LocalSubs32, &TlmHdrPtr->Tlm.Sec.Time[4], 4); /* convert packet data into CFE_TIME_SysTime_t format */ LocalSubs32 = CFE_TIME_Micro2SubSecs((LocalSubs32 >> 12)); @@ -267,23 +267,23 @@ int32 CFE_SB_SetMsgTime(CFE_SB_MsgPtr_t MsgPtr, CFE_TIME_SysTime_t NewTime) /* convert time from CFE_TIME_SysTime_t format to packet format */ LocalSubs16 = (uint16) (NewTime.Subseconds >> 16); - memcpy(&TlmHdrPtr->Sec.Time[0], &NewTime.Seconds, 4); - memcpy(&TlmHdrPtr->Sec.Time[4], &LocalSubs16, 2); + memcpy(&TlmHdrPtr->Tlm.Sec.Time[0], &NewTime.Seconds, 4); + memcpy(&TlmHdrPtr->Tlm.Sec.Time[4], &LocalSubs16, 2); Result = CFE_SUCCESS; #elif (CFE_MISSION_SB_PACKET_TIME_FORMAT == CFE_MISSION_SB_TIME_32_32_SUBS) /* no conversion necessary -- packet format = CFE_TIME_SysTime_t format */ - memcpy(&TlmHdrPtr->Sec.Time[0], &NewTime.Seconds, 4); - memcpy(&TlmHdrPtr->Sec.Time[4], &NewTime.Subseconds, 4); + memcpy(&TlmHdrPtr->Tlm.Sec.Time[0], &NewTime.Seconds, 4); + memcpy(&TlmHdrPtr->Tlm.Sec.Time[4], &NewTime.Subseconds, 4); Result = CFE_SUCCESS; #elif (CFE_MISSION_SB_PACKET_TIME_FORMAT == CFE_MISSION_SB_TIME_32_32_M_20) /* convert time from CFE_TIME_SysTime_t format to packet format */ LocalSubs32 = CFE_TIME_Sub2MicroSecs(NewTime.Subseconds) << 12; - memcpy(&TlmHdrPtr->Sec.Time[0], &NewTime.Seconds, 4); - memcpy(&TlmHdrPtr->Sec.Time[4], &LocalSubs32, 4); + memcpy(&TlmHdrPtr->Tlm.Sec.Time[0], &NewTime.Seconds, 4); + memcpy(&TlmHdrPtr->Tlm.Sec.Time[4], &LocalSubs32, 4); Result = CFE_SUCCESS; #endif @@ -319,7 +319,7 @@ uint16 CFE_SB_GetCmdCode(CFE_SB_MsgPtr_t MsgPtr) /* Cast the input pointer to a Cmd Msg pointer */ CmdHdrPtr = (CFE_SB_CmdHdr_t *)MsgPtr; - return CCSDS_RD_FC(CmdHdrPtr->Sec); + return CCSDS_RD_FC(CmdHdrPtr->Cmd.Sec); }/* end CFE_SB_GetCmdCode */ @@ -339,7 +339,7 @@ int32 CFE_SB_SetCmdCode(CFE_SB_MsgPtr_t MsgPtr, /* Cast the input pointer to a Cmd Msg pointer */ CmdHdrPtr = (CFE_SB_CmdHdr_t *)MsgPtr; - CCSDS_WR_FC(CmdHdrPtr->Sec,CmdCode); + CCSDS_WR_FC(CmdHdrPtr->Cmd.Sec,CmdCode); return CFE_SUCCESS; @@ -362,7 +362,7 @@ uint16 CFE_SB_GetChecksum(CFE_SB_MsgPtr_t MsgPtr) /* cast the input pointer to a Cmd Msg pointer */ CmdHdrPtr = (CFE_SB_CmdHdr_t *)MsgPtr; - return CCSDS_RD_CHECKSUM(CmdHdrPtr->Sec); + return CCSDS_RD_CHECKSUM(CmdHdrPtr->Cmd.Sec); }/* end CFE_SB_GetChecksum */ diff --git a/fsw/cfe-core/unit-test/sb_UT.c b/fsw/cfe-core/unit-test/sb_UT.c index b6d4a3917..bf0340701 100644 --- a/fsw/cfe-core/unit-test/sb_UT.c +++ b/fsw/cfe-core/unit-test/sb_UT.c @@ -156,7 +156,7 @@ void Test_SB_Macros(void) */ void Test_SB_CCSDSSecHdr_Macros(void) { - CFE_SB_CmdHdr_t NoParamPkt; + CCSDS_CommandPacket_t NoParamPkt; uint32 ExpRtn; uint32 ActRtn; From 5e71fe2f494b9730dfb777a4d99e25b9919ff452 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Wed, 6 May 2020 10:54:38 -0400 Subject: [PATCH 2/2] Issue #666, Change message definitions to ensure alignment 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. --- fsw/cfe-core/src/inc/cfe_es_msg.h | 2 +- fsw/cfe-core/src/inc/cfe_time_msg.h | 4 ++-- fsw/cfe-core/src/time/cfe_time_utils.h | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/fsw/cfe-core/src/inc/cfe_es_msg.h b/fsw/cfe-core/src/inc/cfe_es_msg.h index bbd67a208..ed8a8955a 100644 --- a/fsw/cfe-core/src/inc/cfe_es_msg.h +++ b/fsw/cfe-core/src/inc/cfe_es_msg.h @@ -1586,7 +1586,7 @@ typedef struct typedef struct { - uint8 TlmHeader[CFE_SB_TLM_HDR_SIZE]; /**< \brief cFE Software Bus Telemetry Message Header */ + CFE_SB_TlmHdr_t TlmHeader; /**< \brief cFE Software Bus Telemetry Message Header */ CFE_ES_ShellPacket_Payload_t Payload; }CFE_ES_ShellTlm_t; diff --git a/fsw/cfe-core/src/inc/cfe_time_msg.h b/fsw/cfe-core/src/inc/cfe_time_msg.h index a7324dc9b..d492f806b 100644 --- a/fsw/cfe-core/src/inc/cfe_time_msg.h +++ b/fsw/cfe-core/src/inc/cfe_time_msg.h @@ -878,7 +878,7 @@ typedef struct */ typedef struct { - uint8 CmdHeader[CFE_SB_CMD_HDR_SIZE]; + CFE_SB_CmdHdr_t CmdHeader; } CFE_TIME_ToneSignalCmd_t; @@ -888,7 +888,7 @@ typedef struct */ typedef struct { - uint8 CmdHeader[CFE_SB_CMD_HDR_SIZE]; + CFE_SB_CmdHdr_t CmdHeader; } CFE_TIME_FakeToneCmd_t; diff --git a/fsw/cfe-core/src/time/cfe_time_utils.h b/fsw/cfe-core/src/time/cfe_time_utils.h index 728f69a80..2f48a5d4b 100644 --- a/fsw/cfe-core/src/time/cfe_time_utils.h +++ b/fsw/cfe-core/src/time/cfe_time_utils.h @@ -277,7 +277,7 @@ typedef struct /* ** Local 1Hz wake-up command packet (not related to time at tone)... */ - CCSDS_CommandPacket_t Local1HzCmd; + CFE_SB_CmdHdr_t Local1HzCmd; /* ** Time at the tone command packets (sent by time servers)... @@ -294,7 +294,7 @@ typedef struct * "tone signal" message above. */ #if (CFE_MISSION_TIME_CFG_FAKE_TONE == true) - CCSDS_CommandPacket_t ToneSendCmd; + CFE_SB_CmdHdr_t ToneSendCmd; #endif /*