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

thnkslprpt · 2023-06-06T05:23:51Z

Checklist

I reviewed the Contributing Guide.
I signed and emailed the appropriate Contributor License Agreement to GSFC-SoftwareRelease@mail.nasa.gov and copied cfs-program@lists.nasa.gov.

Describe the contribution

Fixes Some minor out-of-family naming/consistency issues in CF could be updated #387
- Add event for CreatePipe() failure during initialization (and created a new matching EID: CF_CR_PIPE_ERR_EID) + clarified different EIDs for pipe creation errors during initialization of the app, and separate EID for errors during CF Channel pipe creation (CF_CR_CHANNEL_PIPE_ERR_EID).
- In CF_HkCmd(): Use CFE_SB_TimeStampMsg() instead of CFE_MSG_SetMsgTime() and use the CFE_MSG_PTR conversion macro - CF was the only app remaining to use CFE_MSG_SetMsgTime()
- Add memset at the beginning of the initialization routine to zero-out the global data structure (defensive programming, and for consistency - almost all of the cFS modules/apps do this)
- Remove check of the return value of CFE_EVS_SendEvent() reporting initialization success - this is very out-of-family for cFS and also inconsistent - CF does not check the return value of any other calls to CFE_EVS_SendEvent()
- Remove null check in CF_AppMain() after successful call to CFE_SB_ReceiveBuffer() - out-of-family with cFS and also redundant (CFE_SB_ReceiveBuffer() with a successful return guarantees the returned pointer to be non-NULL)

Minor changes:

Comment in CF_CmdAbandon_Txn() noted incorrect parameter cannot be NULL - corrected this
Rename tlm_header to TelemetryHeader
Rename run_status to RunStatus
Rename cmd_pipe to CmdPipe
Rename Software Bus command pipe message pointer variables (CFE_SB_Buffer_t types) from msg to BufPtr
Rename msg_id to MessageID
Move 'Cmd' to end of command function names (e.g. CF_CmdNoop() changed to CF_NoopCmd())
Rename CF_CmdReset() to CF_ResetCountersCmd() - more clear and specific, and in line with vast majority of cFS incl. cFE (also amended the matching type CF_ResetCmd_t to CF_ResetCountersCmd_t)
Rename CF_Init() to CF_AppInit()
Removed a couple of unnessesasary header #includes - cf_verify.h in cf_clist.c and

Note: CF does not verify length for non-command MIDs received in the app pipe (CF_WAKE_UP_MID and CF_SEND_HK_MID) - it would be worthwhile to rectify this at some point.

cFE and the other apps are generally inconsistent on this - some don't check non-command MIDs, some use a single VerifyLength function to check all MIDs arriving, some use separate VerifyLength functions for command and non-command MIDs...

Testing performed
GitHub CI actions all passing successfully (incl. Build + Run, Unit/Functional Tests etc.).

Expected behavior changes
Minor changes as noted above, no significant changes to behavior.

Aligning aberrant naming to the predominant patterns in cFS improves usability and eases future maintenance.

Contributor Info
Avi Weiss @thnkslprpt

github-advanced-security

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

chillfig

To safely remove the assignment to the status variable and maintain functional behavior, CFE_SUCCESS must be guaranteed at cf_app.c line 255. Is it?

Rationale given for removing the update of the status at that line is:

"this is very out-of-family for cFS and also inconsistent - CF does not check the return value of any other calls to CFE_EVS_SendEvent()"

However, examples of this check exist at:
https://github.com/nasa/HK/blob/406ec64b6428131230f2c9dac4f2f32452e04c7e/fsw/src/hk_app.c#L207

https://github.com/nasa/SCH/blob/dbaf51ebbcffe1177997880d0f555c7d489217f5/fsw/src/sch_app.c#L345

chillfig · 2023-06-15T18:38:38Z

fsw/src/cf_app.c

@@ -248,13 +252,8 @@ CFE_Status_t CF_Init(void)

    if (status == CFE_SUCCESS)
    {
-        status =


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

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).

skliper · 2023-06-27T20:15:39Z

For length check and command handling inconsistencies, hopefully nasa/cFE#2038 will eventually resolve this across all of cFS.

chillfig

Concur with changes and justification for check return value omission is appreciated. Although, as discussed at the 6/22/2023 cFS community board, cFS is a community-built product with many developers who worked different apps and so it may be unwise to always use justification of consistency to make all the submodules/apps of cFS behave the same.

github-advanced-security

CodeQL-coding-standard found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

thnkslprpt · 2023-12-03T02:47:04Z

Concur with changes...

@chillfig Justin - slight change since your approval due to a merge conflict resolution. There are now 2 separate EIDs for the different pipe creation error locations: CF_CR_CHANNEL_PIPE_ERR_EID and CF_CR_PIPE_ERR_EID

jphickey

I like this changeset, but it overlaps with other recent PRs. We should rebase it, but only after the other pending items.

fsw/src/cf_app.c

-    msg = NULL;
-
-    while (CFE_ES_RunLoop(&CF_AppData.run_status))
+    while (CFE_ES_RunLoop(&CF_AppData.RunStatus))


fsw/src/cf_app.c

fsw/src/cf_cmd.c

@@ -513,7 +513,7 @@
 *-----------------------------------------------------------------*/
 void 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)


fsw/src/cf_cmd.c

@@ -480,7 +480,7 @@
 *-----------------------------------------------------------------*/
 void 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)


thnkslprpt · 2023-12-06T12:05:39Z

I like this changeset, but it overlaps with other recent PRs. We should rebase it, but only after the other pending items.

No worries Joe @jphickey - I'll try to keep it up-to-date. Feel free to merge it whenever it suits.

fsw/src/cf_dispatch.c


        /* first, verify command length */
        if (len == expected_lengths[cmd])
        {
            /* if valid, process command */
            if (fns[cmd])
            {
-                fns[cmd](msg);
+                fns[cmd](BufPtr);


fsw/src/cf_cmd.c

@@ -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)


fsw/src/cf_dispatch.c

@@ -39,13 +39,13 @@
 * See description in cf_cmd.h for argument/return detail
 *
 *-----------------------------------------------------------------*/
-void CF_ProcessGroundCommand(const CFE_SB_Buffer_t *msg)
+void CF_ProcessGroundCommand(const CFE_SB_Buffer_t *BufPtr)


fsw/src/cf_cmd.c

@@ -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)


fsw/src/cf_dispatch.c

@@ -39,13 +39,13 @@
 * See description in cf_cmd.h for argument/return detail
 *
 *-----------------------------------------------------------------*/
-void CF_ProcessGroundCommand(const CFE_SB_Buffer_t *msg)
+void CF_ProcessGroundCommand(const CFE_SB_Buffer_t *BufPtr)


fsw/src/cf_dispatch.c

@@ -129,30 +129,30 @@
 * See description in cf_app.h for argument/return detail
 *
 *-----------------------------------------------------------------*/
-void CF_AppPipe(const CFE_SB_Buffer_t *msg)
+void CF_AppPipe(const CFE_SB_Buffer_t *BufPtr)


fsw/src/cf_app.c

@@ -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)


fsw/src/cf_dispatch.h

fsw/src/cf_cmd.c

@@ -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)


fsw/src/cf_cmd.c

@@ -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)


dzbaker · 2024-09-26T20:29:04Z

Hi @thnkslprpt, apologies about the delay with this PR. I'd like to get it into the next round of PR reviews. Would you be able to resolve the merge conflict/unit test workflow failure?

thnkslprpt · 2024-09-27T07:20:03Z

Hi @thnkslprpt, apologies about the delay with this PR. I'd like to get it into the next round of PR reviews. Would you be able to resolve the merge conflict/unit test workflow failure?

I think it's good to go now @dzbaker

github-advanced-security bot found potential problems Jun 6, 2023

View reviewed changes

dzbaker requested a review from chillfig June 8, 2023 18:14

chillfig approved these changes Jun 15, 2023

View reviewed changes

chillfig added the enhancement label Jun 15, 2023

chillfig self-requested a review June 15, 2023 18:38

chillfig requested changes Jun 22, 2023

View reviewed changes

thnkslprpt requested a review from chillfig June 23, 2023 00:19

chillfig approved these changes Jun 29, 2023

View reviewed changes

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch from 7aa196a to 6a7d0be Compare August 19, 2023 07:24

github-advanced-security bot found potential problems Aug 19, 2023

View reviewed changes

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch from 6a7d0be to a19fb2c Compare October 26, 2023 12:39

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch 3 times, most recently from 454f902 to 69277a8 Compare December 3, 2023 02:34

jphickey reviewed Dec 5, 2023

View reviewed changes

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch 3 times, most recently from b505df0 to 716b89d Compare December 6, 2023 08:15

github-advanced-security bot found potential problems Dec 6, 2023

View reviewed changes

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch 5 times, most recently from b3d8a4f to 2cbc05b Compare December 6, 2023 11:06

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch from 2cbc05b to f2d7e54 Compare December 10, 2023 07:57

dmknutsen added CCB:Approved and removed CCB:Approved labels Dec 14, 2023

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch from f2d7e54 to bdabfd7 Compare December 16, 2023 03:24

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch 2 times, most recently from 88fa0fe to e441c8e Compare January 14, 2024 07:24

github-advanced-security bot found potential problems Jan 14, 2024

View reviewed changes

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch from e441c8e to 9a01790 Compare January 14, 2024 07:40

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch from 9a01790 to 191c62f Compare March 24, 2024 02:19

github-advanced-security bot found potential problems Mar 24, 2024

View reviewed changes

fsw/src/cf_dispatch.h Fixed Show fixed Hide fixed

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch from 191c62f to 5b54245 Compare March 24, 2024 08:04

dzbaker added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Jul 5, 2024

dzbaker approved these changes Jul 5, 2024

View reviewed changes

github-advanced-security bot found potential problems Jul 15, 2024

View reviewed changes

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch from 4710faa to e1cc63c Compare September 27, 2024 06:57

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

f928452

thnkslprpt force-pushed the fix-387-update-minor-naming-and-consistency-issues branch from e1cc63c to f928452 Compare September 27, 2024 07:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

thnkslprpt commented Jun 6, 2023 •

edited

Loading

github-advanced-security bot left a comment

chillfig left a comment

chillfig Jun 15, 2023

thnkslprpt Jun 23, 2023 •

edited

Loading

skliper commented Jun 27, 2023

chillfig left a comment

github-advanced-security bot left a comment

thnkslprpt commented Dec 3, 2023

jphickey left a comment

thnkslprpt commented Dec 6, 2023

dzbaker commented Sep 26, 2024

thnkslprpt commented Sep 27, 2024

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

Are you sure you want to change the base?

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

Conversation

thnkslprpt commented Jun 6, 2023 • edited Loading

github-advanced-security bot left a comment

Choose a reason for hiding this comment

chillfig left a comment

Choose a reason for hiding this comment

chillfig Jun 15, 2023

Choose a reason for hiding this comment

thnkslprpt Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

skliper commented Jun 27, 2023

chillfig left a comment

Choose a reason for hiding this comment

github-advanced-security bot left a comment

Choose a reason for hiding this comment

thnkslprpt commented Dec 3, 2023

jphickey left a comment

Choose a reason for hiding this comment

thnkslprpt commented Dec 6, 2023

dzbaker commented Sep 26, 2024

thnkslprpt commented Sep 27, 2024

thnkslprpt commented Jun 6, 2023 •

edited

Loading

thnkslprpt Jun 23, 2023 •

edited

Loading