CF icon indicating copy to clipboard operation
CF copied to clipboard

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

Open thnkslprpt opened this issue 2 years ago • 3 comments

Checklist

Describe the contribution

  • Fixes #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

thnkslprpt avatar Jun 06 '23 05:06 thnkslprpt

For length check and command handling inconsistencies, hopefully https://github.com/nasa/cFE/issues/2038 will eventually resolve this across all of cFS.

skliper avatar Jun 27 '23 20:06 skliper

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

thnkslprpt avatar Dec 03 '23 02:12 thnkslprpt

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.

thnkslprpt avatar Dec 06 '23 12:12 thnkslprpt

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?

dzbaker avatar Sep 26 '24 20:09 dzbaker

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

thnkslprpt avatar Sep 27 '24 07:09 thnkslprpt