Skip to content

Fix #263, expose IsValidMsgID and partially address #245 Consistent use of MsgId types #602

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

Merged
jphickey merged 2 commits into from
Apr 22, 2020
Merged

Fix #263, expose IsValidMsgID and partially address #245 Consistent use of MsgId types #602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
537f9b8
Fix #263, Expose CFE_SB_IsValidMsgId() API
jphickey Apr 15, 2020
51c599c
Fix #245, Consistent use of MsgId type
jphickey Apr 13, 2020
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
31 changes: 19 additions & 12 deletions fsw/cfe-core/src/es/cfe_es_task.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,23 +251,30 @@ int32 CFE_ES_TaskInit(void)
/*
** Initialize housekeeping packet (clear user data area)
*/
CFE_SB_InitMsg(&CFE_ES_TaskData.HkPacket, CFE_ES_HK_TLM_MID, sizeof(CFE_ES_TaskData.HkPacket), true);
CFE_SB_InitMsg(&CFE_ES_TaskData.HkPacket,
CFE_SB_ValueToMsgId(CFE_ES_HK_TLM_MID),
sizeof(CFE_ES_TaskData.HkPacket), true);

/*
** Initialize shell output packet (clear user data area)
*/
CFE_SB_InitMsg(&CFE_ES_TaskData.ShellPacket, CFE_ES_SHELL_TLM_MID, sizeof(CFE_ES_TaskData.ShellPacket), true);
CFE_SB_InitMsg(&CFE_ES_TaskData.ShellPacket,
CFE_SB_ValueToMsgId(CFE_ES_SHELL_TLM_MID),
sizeof(CFE_ES_TaskData.ShellPacket), true);

/*
** Initialize single application telemetry packet
*/
CFE_SB_InitMsg(&CFE_ES_TaskData.OneAppPacket, CFE_ES_APP_TLM_MID, sizeof(CFE_ES_TaskData.OneAppPacket), true);
CFE_SB_InitMsg(&CFE_ES_TaskData.OneAppPacket,
CFE_SB_ValueToMsgId(CFE_ES_APP_TLM_MID),
sizeof(CFE_ES_TaskData.OneAppPacket), true);

/*
** Initialize memory pool statistics telemetry packet
*/
CFE_SB_InitMsg(&CFE_ES_TaskData.MemStatsPacket, CFE_ES_MEMSTATS_TLM_MID,
sizeof(CFE_ES_TaskData.MemStatsPacket), true);
CFE_SB_InitMsg(&CFE_ES_TaskData.MemStatsPacket,
CFE_SB_ValueToMsgId(CFE_ES_MEMSTATS_TLM_MID),
sizeof(CFE_ES_TaskData.MemStatsPacket), true);

/*
** Create Software Bus message pipe
Expand All @@ -282,7 +289,7 @@ int32 CFE_ES_TaskInit(void)
/*
** Subscribe to Housekeeping request commands
*/
Status = CFE_SB_SubscribeEx(CFE_ES_SEND_HK_MID, CFE_ES_TaskData.CmdPipe,
Status = CFE_SB_SubscribeEx(CFE_SB_ValueToMsgId(CFE_ES_SEND_HK_MID), CFE_ES_TaskData.CmdPipe,
CFE_SB_Default_Qos, CFE_ES_TaskData.LimitHK);
if ( Status != CFE_SUCCESS )
{
Expand All @@ -293,8 +300,8 @@ int32 CFE_ES_TaskInit(void)
/*
** Subscribe to ES task ground command packets
*/
Status = CFE_SB_SubscribeEx(CFE_ES_CMD_MID, CFE_ES_TaskData.CmdPipe,
CFE_SB_Default_Qos, CFE_ES_TaskData.LimitCmd);
Status = CFE_SB_SubscribeEx(CFE_SB_ValueToMsgId(CFE_ES_CMD_MID), CFE_ES_TaskData.CmdPipe,
CFE_SB_Default_Qos, CFE_ES_TaskData.LimitCmd);
if ( Status != CFE_SUCCESS )
{
CFE_ES_WriteToSysLog("ES:Cannot Subscribe to ES ground commands, RC = 0x%08X\n", (unsigned int)Status);
Expand Down Expand Up @@ -427,7 +434,7 @@ void CFE_ES_TaskPipe(CFE_SB_MsgPtr_t Msg)
uint16 CommandCode;

MessageID = CFE_SB_GetMsgId(Msg);
switch (MessageID)
switch (CFE_SB_MsgIdToValue(MessageID))
{
/*
** Housekeeping telemetry request
Expand Down Expand Up @@ -622,7 +629,7 @@ void CFE_ES_TaskPipe(CFE_SB_MsgPtr_t Msg)
default:
CFE_EVS_SendEvent(CFE_ES_CC1_ERR_EID, CFE_EVS_EventType_ERROR,
"Invalid ground command code: ID = 0x%X, CC = %d",
(unsigned int)MessageID, (int)CommandCode);
(unsigned int)CFE_SB_MsgIdToValue(MessageID), (int)CommandCode);
CFE_ES_TaskData.CommandErrorCounter++;
break;
}
Expand All @@ -632,7 +639,7 @@ void CFE_ES_TaskPipe(CFE_SB_MsgPtr_t Msg)

CFE_EVS_SendEvent(CFE_ES_MID_ERR_EID, CFE_EVS_EventType_ERROR,
"Invalid command pipe message ID: 0x%X",
(unsigned int)MessageID);
(unsigned int)CFE_SB_MsgIdToValue(MessageID));
CFE_ES_TaskData.CommandErrorCounter++;
break;
}
Expand Down Expand Up @@ -1692,7 +1699,7 @@ bool CFE_ES_VerifyCmdLength(CFE_SB_MsgPtr_t Msg, uint16 ExpectedLength)

CFE_EVS_SendEvent(CFE_ES_LEN_ERR_EID, CFE_EVS_EventType_ERROR,
"Invalid cmd length: ID = 0x%X, CC = %d, Exp Len = %d, Len = %d",
(unsigned int)MessageID, (int)CommandCode, (int)ExpectedLength, (int)ActualLength);
(unsigned int)CFE_SB_MsgIdToValue(MessageID), (int)CommandCode, (int)ExpectedLength, (int)ActualLength);
result = false;
CFE_ES_TaskData.CommandErrorCounter++;
}
Expand Down
20 changes: 13 additions & 7 deletions fsw/cfe-core/src/evs/cfe_evs_task.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ int32 CFE_EVS_EarlyInit ( void )
CFE_EVS_GlobalData.EVS_AppID = CFE_EVS_UNDEF_APPID;

/* Initialize housekeeping packet */
CFE_SB_InitMsg(&CFE_EVS_GlobalData.EVS_TlmPkt, CFE_EVS_HK_TLM_MID,
CFE_SB_InitMsg(&CFE_EVS_GlobalData.EVS_TlmPkt, CFE_SB_ValueToMsgId(CFE_EVS_HK_TLM_MID),
sizeof(CFE_EVS_GlobalData.EVS_TlmPkt), false);

/* Elements stored in the hk packet that have non-zero default values */
Expand Down Expand Up @@ -315,15 +315,15 @@ int32 CFE_EVS_TaskInit ( void )
}

/* Subscribe to command and telemetry requests coming in on the command pipe */
Status = CFE_SB_SubscribeEx(CFE_EVS_CMD_MID, CFE_EVS_GlobalData.EVS_CommandPipe,
Status = CFE_SB_SubscribeEx(CFE_SB_ValueToMsgId(CFE_EVS_CMD_MID), CFE_EVS_GlobalData.EVS_CommandPipe,
CFE_SB_Default_Qos, CFE_EVS_MSG_LIMIT);
if (Status != CFE_SUCCESS)
{
CFE_ES_WriteToSysLog("EVS:Subscribing to Cmds Failed:RC=0x%08X\n",(unsigned int)Status);
return Status;
}

Status = CFE_SB_SubscribeEx(CFE_EVS_SEND_HK_MID, CFE_EVS_GlobalData.EVS_CommandPipe,
Status = CFE_SB_SubscribeEx(CFE_SB_ValueToMsgId(CFE_EVS_SEND_HK_MID), CFE_EVS_GlobalData.EVS_CommandPipe,
CFE_SB_Default_Qos, CFE_EVS_MSG_LIMIT);
if (Status != CFE_SUCCESS)
{
Expand Down Expand Up @@ -354,8 +354,12 @@ int32 CFE_EVS_TaskInit ( void )
*/
void CFE_EVS_ProcessCommandPacket ( CFE_SB_MsgPtr_t EVS_MsgPtr )
{
CFE_SB_MsgId_t MessageID;

MessageID = CFE_SB_GetMsgId(EVS_MsgPtr);

/* Process all SB messages */
switch (CFE_SB_GetMsgId(EVS_MsgPtr))
switch (CFE_SB_MsgIdToValue(MessageID))
{
case CFE_EVS_CMD_MID:
/* EVS task specific command */
Expand All @@ -372,7 +376,7 @@ void CFE_EVS_ProcessCommandPacket ( CFE_SB_MsgPtr_t EVS_MsgPtr )
CFE_EVS_GlobalData.EVS_TlmPkt.Payload.CommandErrorCounter++;
EVS_SendEvent(CFE_EVS_ERR_MSGID_EID, CFE_EVS_EventType_ERROR,
"Invalid command packet, Message ID = 0x%08X",
(unsigned int)CFE_SB_GetMsgId(EVS_MsgPtr));
(unsigned int)CFE_SB_MsgIdToValue(MessageID));
break;
}

Expand Down Expand Up @@ -573,7 +577,8 @@ void CFE_EVS_ProcessGroundCommand ( CFE_SB_MsgPtr_t EVS_MsgPtr )

EVS_SendEvent(CFE_EVS_ERR_CC_EID, CFE_EVS_EventType_ERROR,
"Invalid command code -- ID = 0x%08x, CC = %d",
(unsigned int)CFE_SB_GetMsgId(EVS_MsgPtr), (int)CFE_SB_GetCmdCode(EVS_MsgPtr));
(unsigned int)CFE_SB_MsgIdToValue(CFE_SB_GetMsgId(EVS_MsgPtr)),
(int)CFE_SB_GetCmdCode(EVS_MsgPtr));
Status = CFE_STATUS_BAD_COMMAND_CODE;

break;
Expand Down Expand Up @@ -618,7 +623,8 @@ bool CFE_EVS_VerifyCmdLength(CFE_SB_MsgPtr_t Msg, uint16 ExpectedLength)

EVS_SendEvent(CFE_EVS_LEN_ERR_EID, CFE_EVS_EventType_ERROR,
"Invalid cmd length: ID = 0x%X, CC = %d, Exp Len = %d, Len = %d",
(unsigned int)MessageID, (int)CommandCode, (int)ExpectedLength, (int)ActualLength);
(unsigned int)CFE_SB_MsgIdToValue(MessageID),
(int)CommandCode, (int)ExpectedLength, (int)ActualLength);
result = false;
}

Expand Down
6 changes: 4 additions & 2 deletions fsw/cfe-core/src/evs/cfe_evs_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,8 @@ void EVS_GenerateEventTelemetry(uint32 AppID, uint16 EventID, uint16 EventType,
int ExpandedLength;

/* Initialize EVS event packets */
CFE_SB_InitMsg(&LongEventTlm, CFE_EVS_LONG_EVENT_MSG_MID, sizeof(LongEventTlm), true);
CFE_SB_InitMsg(&LongEventTlm, CFE_SB_ValueToMsgId(CFE_EVS_LONG_EVENT_MSG_MID),
sizeof(LongEventTlm), true);
LongEventTlm.Payload.PacketID.EventID = EventID;
LongEventTlm.Payload.PacketID.EventType = EventType;

Expand Down Expand Up @@ -408,7 +409,8 @@ void EVS_GenerateEventTelemetry(uint32 AppID, uint16 EventID, uint16 EventType,
*
* This goes out on a separate message ID.
*/
CFE_SB_InitMsg(&ShortEventTlm, CFE_EVS_SHORT_EVENT_MSG_MID, sizeof(ShortEventTlm), true);
CFE_SB_InitMsg(&ShortEventTlm, CFE_SB_ValueToMsgId(CFE_EVS_SHORT_EVENT_MSG_MID),
sizeof(ShortEventTlm), true);
CFE_SB_SetMsgTime((CFE_SB_Msg_t *) &ShortEventTlm, *TimeStamp);
ShortEventTlm.Payload.PacketID = LongEventTlm.Payload.PacketID;
CFE_SB_SendMsg((CFE_SB_Msg_t *) &ShortEventTlm);
Expand Down
72 changes: 67 additions & 5 deletions fsw/cfe-core/src/inc/cfe_sb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,54 @@
#define CFE_SB_SUBSCRIPTION 0 /**< \brief Subtype specifier used in #CFE_SB_SingleSubscriptionTlm_t by SBN App */
#define CFE_SB_UNSUBSCRIPTION 1 /**< \brief Subtype specified used in #CFE_SB_SingleSubscriptionTlm_t by SBN App */

#define CFE_SB_INVALID_MSG_ID 0xFFFF /**< \brief Initializer for #CFE_SB_MsgId_t values that will not match any real MsgId */
/* ------------------------------------------------------ */
/* Macro Constants for use with the CFE_SB_MsgId_t type */
/* ------------------------------------------------------ */

/**
* \brief Translation macro to convert from MsgId integer values to opaque/abstract API values
*
* This conversion exists in macro form to allow compile-time evaluation for constants, and
* should not be used directly in application code.
Copy link
Contributor

@skliper skliper Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather thse be in a separate file if it's not intended for application use, since this file is pulled into the API by doxygen.

Copy link
Contributor Author

@jphickey jphickey Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the problem is the same logic is needed by CFE_SB_MsgIdToValue/ValueToMsgId and these are inline functions within the same file, so the macro needs to be available.

By "application code' really I mean runtime -- the reason these need to exist in macro form is because one cannot use the inline function when initializing data at compile-time. I want to encourage use of the inline functions whenever possible because its more type safe, but occasionally one does need an expression that evaluates at compile-time, and its those times that the macro needs to be used instead.

Copy link
Contributor

@skliper skliper Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still could be in different file and just include, which provides a clear separation.

Copy link
Contributor Author

@jphickey jphickey Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there is a possible valid case for using it externally, where one actually needs a value that evaluates at compile time.

The preference should always be to use the inline function, but there are some places (e.g. global variable init) where they don't work.

As there are no applications using this and the decision isn't made yet as to how MsgIds will be defined in the future, I suppose I could put this into a new file in the private/ subdir if that helps, but that seems like overkill to make a new file for just this.

But No matter how we define MID's in the future, I would think we'd need something that can be evaluated at compile time....

Copy link
Contributor Author

@jphickey jphickey Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the documentation was clarified to indicate "application runtime code" rather than just "application code"?

Copy link
Contributor

@skliper skliper Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, from a naive user point of view I wouldn't know the difference. No biggie, I just don't like huge header files to begin with, especially containing things that have different uses. Wouldn't have to be in private, just prefer separation.

*
* For applications, use the CFE_SB_ValueToMsgId() inline function instead.
*
* \sa CFE_SB_ValueToMsgId()
*/
#define CFE_SB_MSGID_WRAP_VALUE(val) ((CFE_SB_MsgId_t)(val))

/**
* \brief Translation macro to convert to MsgId integer values from opaque/abstract API values
*
* This conversion exists in macro form to allow compile-time evaluation for constants, and
* should not be used directly in application code.
*
* For applications, use the CFE_SB_MsgIdToValue() inline function instead.
*
* \sa CFE_SB_MsgIdToValue()
*/
#define CFE_SB_MSGID_UNWRAP_VALUE(mid) ((CFE_SB_MsgId_Atom_t)(mid))

/**
* \brief Reserved value for CFE_SB_MsgId_t that will not match any valid MsgId
*
* This rvalue macro can be used for static/compile-time data initialization to ensure that
* the initialized value does not alias to a valid MsgId object.
*/
#define CFE_SB_MSGID_RESERVED CFE_SB_MSGID_WRAP_VALUE(-1)

/**
* \brief A literal of the CFE_SB_MsgId_t type representing an invalid ID
*
* This value should be used for runtime initialization of CFE_SB_MsgId_t values.
*
* \note This may be a compound literal in a future revision. Per C99, compound
* literals are lvalues, not rvalues, so this value should not be used in
* static/compile-time data initialization. For static data initialization
* purposes (rvalue), #CFE_SB_MSGID_RESERVED should be used instead.
* However, in the current implementation, they are equivalent.
*/
#define CFE_SB_INVALID_MSG_ID CFE_SB_MSGID_RESERVED

/*
** Macro Definitions
Expand Down Expand Up @@ -1278,7 +1325,21 @@ bool CFE_SB_ValidateChecksum(CFE_SB_MsgPtr_t MsgPtr);

/*****************************************************************************/
/**
* \brief Identifies whether a two #CFE_SB_MsgId_t values are equal
* \brief Identifies whether a given CFE_SB_MsgId_t is valid
*
* \par Description
* Implements a basic sanity check on the value provided
*
* \return Boolean message ID validity indicator
* \retval true Message ID is within the valid range
* \retval false Message ID is not within the valid range
*/
bool CFE_SB_IsValidMsgId(CFE_SB_MsgId_t MsgId);


/*****************************************************************************/
/**
* \brief Identifies whether two #CFE_SB_MsgId_t values are equal
*
* \par Description
* In cases where the #CFE_SB_MsgId_t type is not a simple integer
Expand All @@ -1296,7 +1357,7 @@ bool CFE_SB_ValidateChecksum(CFE_SB_MsgPtr_t MsgPtr);
*/
static inline bool CFE_SB_MsgId_Equal(CFE_SB_MsgId_t MsgId1, CFE_SB_MsgId_t MsgId2)
{
return (MsgId1 == MsgId2);
return CFE_SB_MSGID_UNWRAP_VALUE(MsgId1) == CFE_SB_MSGID_UNWRAP_VALUE(MsgId2);
}

/*****************************************************************************/
Expand Down Expand Up @@ -1327,7 +1388,7 @@ static inline bool CFE_SB_MsgId_Equal(CFE_SB_MsgId_t MsgId1, CFE_SB_MsgId_t MsgI
*/
static inline CFE_SB_MsgId_Atom_t CFE_SB_MsgIdToValue(CFE_SB_MsgId_t MsgId)
{
return MsgId;
return CFE_SB_MSGID_UNWRAP_VALUE(MsgId);
}

/*****************************************************************************/
Expand Down Expand Up @@ -1356,7 +1417,8 @@ static inline CFE_SB_MsgId_Atom_t CFE_SB_MsgIdToValue(CFE_SB_MsgId_t MsgId)
*/
static inline CFE_SB_MsgId_t CFE_SB_ValueToMsgId(CFE_SB_MsgId_Atom_t MsgIdValue)
{
return MsgIdValue;
CFE_SB_MsgId_t Result = CFE_SB_MSGID_WRAP_VALUE(MsgIdValue);
return Result;
}
/**@}*/

Expand Down
2 changes: 1 addition & 1 deletion fsw/cfe-core/src/inc/cfe_sb_msg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,7 @@ typedef struct{
** Structure of one element of the map information in response to #CFE_SB_SEND_MAP_INFO_CC
*/
typedef struct{
CFE_SB_MsgId_Atom_t MsgId;/**< \brief Message Id which has been subscribed to */
CFE_SB_MsgId_t MsgId;/**< \brief Message Id which has been subscribed to */
CFE_SB_MsgRouteIdx_Atom_t Index;/**< \brief Routing table index where pipe destinations are found */
}CFE_SB_MsgMapFileEntry_t;

Expand Down
Loading