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 #387, Update minor out-of-family naming/consistency issues in CF #388

Merged
merged 1 commit into from
Oct 3, 2024
Merged

Fix #387, Update minor out-of-family naming/consistency issues in CF #388

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
2 changes: 1 addition & 1 deletion .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ A clear and concise description of what the contribution is.
**Testing performed**
Steps taken to test the contribution:
1. Build steps '...'
1. Execution steps '...'
2. Execution steps '...'

**Expected behavior changes**
A clear and concise description of how this contribution will change behavior and level of impact.
Expand Down
2 changes: 1 addition & 1 deletion config/default_cf_internal_cfg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@
* @par Limits:
* The minimum size of this parameter is 1
* The maximum size dictated by cFE platform configuration
* parameter is CFE_SB_MAX_PIPE_DEPTH
* parameter is OS_QUEUE_MAX_DEPTH
*/
#define CF_PIPE_DEPTH (32)

Expand Down
4 changes: 2 additions & 2 deletions config/default_cf_msgstruct.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,11 @@ typedef struct CF_DisableEngineCmd
*
* For command details see #CF_RESET_CC
*/
typedef struct CF_ResetCmd
typedef struct CF_ResetCountersCmd
{
CFE_MSG_CommandHeader_t CommandHeader; /**< \brief Command header */
CF_UnionArgs_Payload_t Payload; /**< \brief Generic command arguments */
} CF_ResetCmd_t;
} CF_ResetCountersCmd_t;

/**
* \brief Freeze command structure
Expand Down
13 changes: 12 additions & 1 deletion fsw/inc/cf_events.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@
*
* Failure from create pipe call during engine channel initialization
*/
#define CF_CR_PIPE_ERR_EID (31)
#define CF_CR_CHANNEL_PIPE_ERR_EID (31)

/**
* \brief CF Channel Message Subscription Failed Event ID
Expand Down Expand Up @@ -212,6 +212,17 @@
*/
#define CF_INIT_OUTGOING_SIZE_ERR_EID (35)

/**
* \brief CF Create SB Command Pipe at Initialization Failed Event ID
*
* \par Type: ERROR
*
* \par Cause:
*
* Failure from create command pipe call during application initialization
*/
#define CF_CR_PIPE_ERR_EID (36)

/**************************************************************************
* CF_PDU event IDs - Protocol data unit
*/
Expand Down
54 changes: 23 additions & 31 deletions fsw/src/cf_app.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,23 +71,23 @@
{
CFE_EVS_SendEvent(CF_INIT_TBL_CHECK_REL_ERR_EID, CFE_EVS_EventType_ERROR,
"CF: error in CFE_TBL_ReleaseAddress (check), returned 0x%08lx", (unsigned long)status);
CF_AppData.run_status = CFE_ES_RunStatus_APP_ERROR;
CF_AppData.RunStatus = CFE_ES_RunStatus_APP_ERROR;
}

status = CFE_TBL_Manage(CF_AppData.config_handle);
if (status < CFE_SUCCESS)
{
CFE_EVS_SendEvent(CF_INIT_TBL_CHECK_MAN_ERR_EID, CFE_EVS_EventType_ERROR,
"CF: error in CFE_TBL_Manage (check), returned 0x%08lx", (unsigned long)status);
CF_AppData.run_status = CFE_ES_RunStatus_APP_ERROR;
CF_AppData.RunStatus = CFE_ES_RunStatus_APP_ERROR;
}

status = CFE_TBL_GetAddress((void *)&CF_AppData.config_table, CF_AppData.config_handle);
if (status < CFE_SUCCESS)
{
CFE_EVS_SendEvent(CF_INIT_TBL_CHECK_GA_ERR_EID, CFE_EVS_EventType_ERROR,
"CF: failed to get table address (check), returned 0x%08lx", (unsigned long)status);
CF_AppData.run_status = CFE_ES_RunStatus_APP_ERROR;
CF_AppData.RunStatus = CFE_ES_RunStatus_APP_ERROR;
}
}
}
Expand Down Expand Up @@ -186,13 +186,16 @@
* See description in cf_app.h for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_Status_t CF_Init(void)
CFE_Status_t CF_AppInit(void)
Fixed Show fixed Hide fixed

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
{
CFE_Status_t status;
const CFE_SB_MsgId_Atom_t MID_VALUES[] = {CF_CMD_MID, CF_SEND_HK_MID, CF_WAKE_UP_MID};
uint32 i;

CF_AppData.run_status = CFE_ES_RunStatus_APP_RUN;
CF_AppData.RunStatus = CFE_ES_RunStatus_APP_RUN;

/* Zero-out global data structure */
memset(&CF_AppData, 0, sizeof(CF_AppData));

CFE_MSG_Init(CFE_MSG_PTR(CF_AppData.hk.TelemetryHeader), CFE_SB_ValueToMsgId(CF_HK_TLM_MID), sizeof(CF_AppData.hk));

Expand All @@ -203,19 +206,19 @@
}
else
{
status = CFE_SB_CreatePipe(&CF_AppData.cmd_pipe, CF_PIPE_DEPTH, CF_PIPE_NAME);
status = CFE_SB_CreatePipe(&CF_AppData.CmdPipe, CF_PIPE_DEPTH, CF_PIPE_NAME);
if (status != CFE_SUCCESS)
{
CFE_ES_WriteToSysLog("CF app: error creating pipe %s, returned 0x%08lx", CF_PIPE_NAME,
(unsigned long)status);
CFE_EVS_SendEvent(CF_CR_PIPE_ERR_EID, CFE_EVS_EventType_ERROR,
"CF app: error creating pipe %s, returned 0x%08lx", CF_PIPE_NAME, (unsigned long)status);
}
}

if (status == CFE_SUCCESS)
{
for (i = 0; i < (sizeof(MID_VALUES) / sizeof(MID_VALUES[0])); ++i)
{
status = CFE_SB_Subscribe(CFE_SB_ValueToMsgId(MID_VALUES[i]), CF_AppData.cmd_pipe);
status = CFE_SB_Subscribe(CFE_SB_ValueToMsgId(MID_VALUES[i]), CF_AppData.CmdPipe);
if (status != CFE_SUCCESS)
{
CFE_ES_WriteToSysLog("CF app: failed to subscribe to MID 0x%04lx, returned 0x%08lx",
Expand All @@ -237,13 +240,8 @@

if (status == CFE_SUCCESS)
{
status =
Copy link
Contributor

Choose a reason for hiding this comment

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

@thnkslprpt Would you please expand on why the status init is removed here?

Copy link
Contributor Author

@thnkslprpt thnkslprpt Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes correct - of the 10 main apps only HK and CF currently check the return of the CFE_EVS_SendEvent() reporting successful initialization. MD assigns it to the Result variable but does not report on failure. In cFE, only the SB module checks the return of this call to CFE_EVS_SendEvent(), all of the other modules do not do so.

In general, out of several thousand invocations of CFE_EVS_SendEvent() across cFS, its return value is only checked a handful of times. In CF specifically, there are 141 calls to CFE_EVS_SendEvent() (just in flight code) and this is the only place that the return value is assigned/checked.

If an app is already confirmed to be successfully registered with EVS (as is the case here), it's almost inconceivable that the call to CFE_EVS_SendEvent() will fail.

It seems undesirable for an app to exit (through failure of the init routine, and then setting RunStatus to CFE_ES_RunStatus_APP_ERROR) just because the event/notification of successful initialization failed for some reason (again, highly improbable if already confirmed registered with EVS).

This change is not essential (it can be left as it is) it's more just for the sake of consistency (almost all apps and cFE modules just call CFE_EVS_SendEvent() to report successful initialization, without checking its return value or assigning it to their Status/Result variables).

CFE_EVS_SendEvent(CF_INIT_INF_EID, CFE_EVS_EventType_INFORMATION, "CF Initialized. Version %d.%d.%d.%d",
CF_MAJOR_VERSION, CF_MINOR_VERSION, CF_REVISION, CF_MISSION_REV);
if (status != CFE_SUCCESS)
{
CFE_ES_WriteToSysLog("CF: error sending init event, returned 0x%08lx", (unsigned long)status);
}
CFE_EVS_SendEvent(CF_INIT_INF_EID, CFE_EVS_EventType_INFORMATION, "CF Initialized. Version %d.%d.%d.%d",
CF_MAJOR_VERSION, CF_MINOR_VERSION, CF_REVISION, CF_MISSION_REV);
}

return status;
Expand All @@ -258,38 +256,32 @@
void CF_AppMain(void)
{
int32 status;
CFE_SB_Buffer_t *msg;
CFE_SB_Buffer_t *BufPtr = NULL;

CFE_ES_PerfLogEntry(CF_PERF_ID_APPMAIN);

status = CF_Init();
status = CF_AppInit();
if (status != CFE_SUCCESS)
{
CF_AppData.run_status = CFE_ES_RunStatus_APP_ERROR;
CF_AppData.RunStatus = CFE_ES_RunStatus_APP_ERROR;
}

msg = NULL;

while (CFE_ES_RunLoop(&CF_AppData.run_status))
while (CFE_ES_RunLoop(&CF_AppData.RunStatus))

Check warning

Code scanning / CodeQL

Side effect in a Boolean expression Warning

This Boolean expression is not side-effect free.
{
CFE_ES_PerfLogExit(CF_PERF_ID_APPMAIN);

status = CFE_SB_ReceiveBuffer(&msg, CF_AppData.cmd_pipe, CF_RCVMSG_TIMEOUT);
status = CFE_SB_ReceiveBuffer(&BufPtr, CF_AppData.CmdPipe, CF_RCVMSG_TIMEOUT);
CFE_ES_PerfLogEntry(CF_PERF_ID_APPMAIN);

/*
* note that CFE_SB_ReceiveBuffer() guarantees that a CFE_SUCCESS status is accompanied by
* a valid (non-NULL) output message pointer. However the unit test can force this condition.
*/
if (status == CFE_SUCCESS && msg != NULL)
if (status == CFE_SUCCESS)
{
CF_AppPipe(msg);
CF_AppPipe(BufPtr);
}
else if (status != CFE_SB_TIME_OUT && status != CFE_SB_NO_MESSAGE)
{
CFE_EVS_SendEvent(CF_INIT_MSG_RECV_ERR_EID, CFE_EVS_EventType_ERROR,
"CF: exiting due to CFE_SB_ReceiveBuffer error 0x%08lx", (unsigned long)status);
CF_AppData.run_status = CFE_ES_RunStatus_APP_ERROR;
CF_AppData.RunStatus = CFE_ES_RunStatus_APP_ERROR;
}
else
{
Expand All @@ -298,5 +290,5 @@
}

Check warning

Code scanning / CodeQL

Unbounded loop Warning

This loop does not have a fixed bound.

CFE_ES_PerfLogExit(CF_PERF_ID_APPMAIN);
CFE_ES_ExitApp(CF_AppData.run_status);
CFE_ES_ExitApp(CF_AppData.RunStatus);
}
6 changes: 3 additions & 3 deletions fsw/src/cf_app.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ typedef struct
{
CF_HkPacket_t hk;

uint32 run_status;
uint32 RunStatus;

CFE_SB_PipeId_t cmd_pipe;
CFE_SB_PipeId_t CmdPipe;

CFE_TBL_Handle_t config_handle;
CF_ConfigTable_t *config_table;
Expand Down Expand Up @@ -163,7 +163,7 @@ CFE_Status_t CF_TableInit(void);
* @retval Returns anything else on error.
*
*/
CFE_Status_t CF_Init(void);
CFE_Status_t CF_AppInit(void);

/************************************************************************/
/** @brief CF app entry point
Expand Down
2 changes: 1 addition & 1 deletion fsw/src/cf_cfdp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -953,7 +953,7 @@ CFE_Status_t CF_CFDP_InitEngine(void)
nbuf);
if (ret != CFE_SUCCESS)
{
CFE_EVS_SendEvent(CF_CR_PIPE_ERR_EID, CFE_EVS_EventType_ERROR,
CFE_EVS_SendEvent(CF_CR_CHANNEL_PIPE_ERR_EID, CFE_EVS_EventType_ERROR,
"CF: failed to create pipe %s, returned 0x%08lx", nbuf, (unsigned long)ret);
break;
}
Expand Down
1 change: 0 additions & 1 deletion fsw/src/cf_clist.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
* This file is intended to be a generic class that can be used in other apps.
*/

#include "cf_verify.h"
#include "cf_clist.h"
#include "cf_assert.h"

Expand Down
20 changes: 10 additions & 10 deletions fsw/src/cf_cmd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
* See description in cf_cmd.h for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_Status_t CF_ResetCmd(const CF_ResetCmd_t *msg)
CFE_Status_t CF_ResetCountersCmd(const CF_ResetCountersCmd_t *msg)

Check notice

Code scanning / CodeQL

Function too long Note

CF_ResetCountersCmd has too many lines (63, while 60 are allowed).

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
{
const CF_UnionArgs_Payload_t *data = &msg->Payload;
static const char * names[5] = {"all", "cmd", "fault", "up", "down"};
Expand Down Expand Up @@ -488,7 +488,7 @@
* See description in cf_cmd.h for argument/return detail
*
*-----------------------------------------------------------------*/
void CF_CmdCancel_Txn(CF_Transaction_t *txn, void *ignored)
void CF_Cancel_TxnCmd(CF_Transaction_t *txn, void *ignored)
{
CF_CFDP_CancelTransaction(txn);
}
Expand All @@ -501,7 +501,7 @@
*-----------------------------------------------------------------*/
CFE_Status_t CF_CancelCmd(const CF_CancelCmd_t *msg)
{
if (CF_TsnChanAction(&msg->Payload, "cancel", CF_CmdCancel_Txn, NULL) > 0)
if (CF_TsnChanAction(&msg->Payload, "cancel", CF_Cancel_TxnCmd, NULL) > 0)

Check warning

Code scanning / CodeQL-coding-standard

Side effect in a Boolean expression

This Boolean expression is not side-effect free.

Check warning

Code scanning / CodeQL

Side effect in a Boolean expression Warning

This Boolean expression is not side-effect free.
{
CFE_EVS_SendEvent(CF_CMD_CANCEL_INF_EID, CFE_EVS_EventType_INFORMATION,
"CF: cancel transaction successfully initiated");
Expand All @@ -523,7 +523,7 @@
* See description in cf_cmd.h for argument/return detail
*
*-----------------------------------------------------------------*/
void CF_CmdAbandon_Txn(CF_Transaction_t *txn, void *ignored)
void CF_Abandon_TxnCmd(CF_Transaction_t *txn, void *ignored)
{
CF_CFDP_ResetTransaction(txn, 0);
}
Expand All @@ -536,7 +536,7 @@
*-----------------------------------------------------------------*/
CFE_Status_t CF_AbandonCmd(const CF_AbandonCmd_t *msg)
{
if (CF_TsnChanAction(&msg->Payload, "abandon", CF_CmdAbandon_Txn, NULL) > 0)
if (CF_TsnChanAction(&msg->Payload, "abandon", CF_Abandon_TxnCmd, NULL) > 0)

Check warning

Code scanning / CodeQL-coding-standard

Side effect in a Boolean expression

This Boolean expression is not side-effect free.

Check warning

Code scanning / CodeQL

Side effect in a Boolean expression Warning

This Boolean expression is not side-effect free.
{
CFE_EVS_SendEvent(CF_CMD_ABANDON_INF_EID, CFE_EVS_EventType_INFORMATION, "CF: abandon successful");
++CF_AppData.hk.Payload.counters.cmd;
Expand Down Expand Up @@ -946,7 +946,7 @@
* See description in cf_cmd.h for argument/return detail
*
*-----------------------------------------------------------------*/
CF_ChanAction_Status_t CF_CmdValidateChunkSize(CF_ChunkSize_t val, uint8 chan_num /* ignored */)
CF_ChanAction_Status_t CF_ValidateChunkSizeCmd(CF_ChunkSize_t val, uint8 chan_num /* ignored */)
{
CF_ChanAction_Status_t ret = CF_ChanAction_Status_SUCCESS;
if (val > sizeof(CF_CFDP_PduFileDataContent_t))
Expand All @@ -962,7 +962,7 @@
* See description in cf_cmd.h for argument/return detail
*
*-----------------------------------------------------------------*/
CF_ChanAction_Status_t CF_CmdValidateMaxOutgoing(uint32 val, uint8 chan_num)
CF_ChanAction_Status_t CF_ValidateMaxOutgoingCmd(uint32 val, uint8 chan_num)
{
CF_ChanAction_Status_t ret = CF_ChanAction_Status_SUCCESS;

Expand Down Expand Up @@ -1022,7 +1022,7 @@
case CF_GetSet_ValueID_outgoing_file_chunk_size:
item.ptr = &config->outgoing_file_chunk_size;
item.size = sizeof(config->outgoing_file_chunk_size);
item.fn = CF_CmdValidateChunkSize;
item.fn = CF_ValidateChunkSizeCmd;
break;
case CF_GetSet_ValueID_ack_limit:
item.ptr = &config->chan[chan_num].ack_limit;
Expand All @@ -1039,7 +1039,7 @@
case CF_GetSet_ValueID_chan_max_outgoing_messages_per_wakeup:
item.ptr = &config->chan[chan_num].max_outgoing_messages_per_wakeup;
item.size = sizeof(config->chan[chan_num].max_outgoing_messages_per_wakeup);
item.fn = CF_CmdValidateMaxOutgoing;
item.fn = CF_ValidateMaxOutgoingCmd;
break;
default:
break;
Expand Down Expand Up @@ -1224,7 +1224,7 @@
*-----------------------------------------------------------------*/
CFE_Status_t CF_SendHkCmd(const CF_SendHkCmd_t *msg)
{
CFE_MSG_SetMsgTime(CFE_MSG_PTR(CF_AppData.hk.TelemetryHeader), CFE_TIME_GetTime());
CFE_SB_TimeStampMsg(CFE_MSG_PTR(CF_AppData.hk.TelemetryHeader));
/* return value ignored */ CFE_SB_TransmitMsg(CFE_MSG_PTR(CF_AppData.hk.TelemetryHeader), true);

/* This is also used to check tables */
Expand Down
12 changes: 6 additions & 6 deletions fsw/src/cf_cmd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ CFE_Status_t CF_NoopCmd(const CF_NoopCmd_t *msg);
*
* @param msg Pointer to command message
*/
CFE_Status_t CF_ResetCmd(const CF_ResetCmd_t *msg);
CFE_Status_t CF_ResetCountersCmd(const CF_ResetCountersCmd_t *msg);

/************************************************************************/
/** @brief Ground command to start a file transfer.
Expand Down Expand Up @@ -337,7 +337,7 @@ CFE_Status_t CF_ResumeCmd(const CF_ResumeCmd_t *msg);
* @param txn Pointer to transaction object
* @param ignored Not used by this function
*/
void CF_CmdCancel_Txn(CF_Transaction_t *txn, void *ignored);
void CF_Cancel_TxnCmd(CF_Transaction_t *txn, void *ignored);

/************************************************************************/
/** @brief Handle a cancel ground command.
Expand All @@ -355,12 +355,12 @@ CFE_Status_t CF_CancelCmd(const CF_CancelCmd_t *msg);
* This helper function is used with CF_TsnChanAction() to abandon matched transactions
*
* @par Assumptions, External Events, and Notes:
* msg must not be NULL.
* txn must not be NULL.
*
* @param txn Pointer to transaction object
* @param ignored Not used by this function
*/
void CF_CmdAbandon_Txn(CF_Transaction_t *txn, void *ignored);
void CF_Abandon_TxnCmd(CF_Transaction_t *txn, void *ignored);

/************************************************************************/
/** @brief Handle an abandon ground command.
Expand Down Expand Up @@ -524,7 +524,7 @@ CFE_Status_t CF_WriteQueueCmd(const CF_WriteQueueCmd_t *msg);
* @retval CF_ChanAction_Status_ERROR if failed (val is greater than max PDU)
*
*/
CF_ChanAction_Status_t CF_CmdValidateChunkSize(CF_ChunkSize_t val, uint8 chan_num);
CF_ChanAction_Status_t CF_ValidateChunkSizeCmd(CF_ChunkSize_t val, uint8 chan_num);

/************************************************************************/
/** @brief Checks if the value is within allowable range as outgoing packets per wakeup
Expand All @@ -540,7 +540,7 @@ CF_ChanAction_Status_t CF_CmdValidateChunkSize(CF_ChunkSize_t val, uint8 chan_nu
* @retval CF_ChanAction_Status_ERROR if failed (val is not allowed)
*
*/
CF_ChanAction_Status_t CF_CmdValidateMaxOutgoing(uint32 val, uint8 chan_num);
CF_ChanAction_Status_t CF_ValidateMaxOutgoingCmd(uint32 val, uint8 chan_num);

/************************************************************************/
/** @brief Perform a configuration get/set operation.
Expand Down
2 changes: 1 addition & 1 deletion fsw/src/cf_codec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ static const CF_Codec_BitField_t CF_CFDP_PduMd_CHECKSUM_TYPE = CF_INIT_FIELD
static const CF_Codec_BitField_t CF_CFDP_PduFileData_RECORD_CONTINUATION_STATE = CF_INIT_FIELD(2, 6);
static const CF_Codec_BitField_t CF_CFDP_PduFileData_SEGMENT_METADATA_LENGTH = CF_INIT_FIELD(6, 0);

/* NOTE: get/set will handle endianess */
/* NOTE: get/set will handle endianness */
/*
* ALSO NOTE: These store/set inline functions/macros are used with
* literal integers as well as variables. So they operate on value, where
Expand Down
Loading
Loading