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.

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

Fix #1527, Implement common command-handler return pattern across cFE #2302

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thnkslprpt
Copy link
Contributor

Checklist

Describe the contribution

  • Fixes Implement single pattern for command handler returns #1527
    • Major changes are to convert ES, TIME and SB to the pattern already used by TBL and EVS - namely, to increment the command counters centrally in the command processing function (in XX_dispatch.c), rather than individually inside each command.
      • this results in a reduction of locations where CommandCounter's are incremented from 41 to 5, and ErrorCounter's from 71 to 11
    • Introduced the CFE_STATUS_COMMAND_FAILURE macro which can be used by command functions to signal a general failure to execute their command. This macro can also be used by other cFS components and apps for command execution failures.

Summary of module-specific changes:
ES

  • move incrementing of Command/Error counters from the individual commands into CFE_ES_TaskPipe()
    • there were 4 locations (in CFE_ES_QueryAllCmd() and CFE_ES_QueryAllTasksCmd()) where CFE_SUCCESS was returned during a failure/error event after the ErrorCounter was incremented. These now return CFE_STATUS_COMMAND_FAILURE which improves the clarity of these functions.

TIME

  • move incrementing of Command/Error counters from the individual commands into CFE_TIME_TaskPipe()
  • CFE_TIME_SetDelayImpl(), CFE_TIME_AdjustImpl() and CFE_TIME_1HzAdjImpl() were changed from void return type to CFE_Status_t in order to carry through the result of their respective add/subtract versions over to CFE_TIME_TaskPipe() and increment the required counters

SB

  • move incrementing of Command/Error counters from the individual commands into CFE_SB_ProcessCmdPipePkt()
  • removed the CFE_SB_IncrCmdCtr() helper function as it is no longer required with these changes - its functionality has been folded into CFE_SB_ProcessCmdPipePkt()

TBL

  • removed CFE_TBL_MESSAGE_ERROR (essentially replaced by CFE_STATUS_COMMAND_FAILURE)
  • removed the CFE_TBL_CmdProcRet_t enumeration
    • The constants in this enum were already mapped to CFE macros anyway. The only real change here is that CFE_TBL_INC_ERR_CTR, which was previously mapped to CFE_TBL_MESSAGE_ERROR, has now been replaced by the new CFE_STATUS_COMMAND_FAILURE macro which was introduced in this PR.
      • Note: I haven't added any deprecation for the removal of CFE_TBL_MESSAGE_ERROR or CFE_TBL_CmdProcRet_t - if this is required in this case, just let me know.
  • CFE_TBL_NoopCmd() erroneously noted a possible error return in doxygen comments - this has been removed as the function always returns CFE_SUCCESS.

Note: #2264 which converts the remaining int32 return types to CFE_Status_t (along with most of the local status/return variables) is complementary to this PR.

Testing performed
GitHub CI actions all passing successfully.

Local testing with full cFS suite confirms net coverage unchanged.
Prior to changes:

  lines......: 98.1% (13074 of 13326 lines)
  functions..: 97.0% (1041 of 1073 functions)
  branches...: 97.1% (5870 of 6047 branches)

After changes:

  lines......: 98.1% (13056 of 13308 lines)
  functions..: 97.0% (1040 of 1072 functions)
  branches...: 97.1% (5886 of 6063 branches)

Expected behavior changes
Behavior largely unchanged, other than the modifications listed above.

cFE command-handling code is now simpler, more consistent and easier to maintain.

System(s) tested on
Debian GNU/Linux 11 (bullseye)
Current main branch of cFS bundle.

Contributor Info
Avi Weiss @thnkslprpt

@@ -703,8 +694,11 @@
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
void CFE_TIME_SetDelayImpl(const CFE_TIME_TimeCmd_Payload_t *CommandPtr, CFE_TIME_AdjustDirection_Enum_t Direction)
CFE_Status_t CFE_TIME_SetDelayImpl(const CFE_TIME_TimeCmd_Payload_t *CommandPtr,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
@@ -675,17 +675,17 @@
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_TBL_CmdProcRet_t CFE_TBL_DumpToFile(const char *DumpFilename, const char *TableName, const void *DumpDataAddr,
size_t TblSizeInBytes)
CFE_Status_t CFE_TBL_DumpToFile(const char *DumpFilename, const char *TableName, const void *DumpDataAddr,

Check notice

Code scanning / CodeQL

Function too long Note

CFE_TBL_DumpToFile has too many lines (117, while 60 are allowed).
@@ -675,17 +675,17 @@
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_TBL_CmdProcRet_t CFE_TBL_DumpToFile(const char *DumpFilename, const char *TableName, const void *DumpDataAddr,
size_t TblSizeInBytes)
CFE_Status_t CFE_TBL_DumpToFile(const char *DumpFilename, const char *TableName, const void *DumpDataAddr,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
@@ -964,8 +956,11 @@
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
void CFE_TIME_AdjustImpl(const CFE_TIME_TimeCmd_Payload_t *CommandPtr, CFE_TIME_AdjustDirection_Enum_t Direction)
CFE_Status_t CFE_TIME_AdjustImpl(const CFE_TIME_TimeCmd_Payload_t *CommandPtr,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
@@ -1036,9 +1036,11 @@
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
void CFE_TIME_1HzAdjImpl(const CFE_TIME_OneHzAdjustmentCmd_Payload_t *CommandPtr,
CFE_TIME_AdjustDirection_Enum_t Direction)
CFE_Status_t CFE_TIME_1HzAdjImpl(const CFE_TIME_OneHzAdjustmentCmd_Payload_t *CommandPtr,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
@thnkslprpt thnkslprpt force-pushed the fix-1527-use-consistent-command-handler-return-pattern branch from 0466c81 to 312a1d0 Compare December 14, 2023 04:14
@thnkslprpt thnkslprpt force-pushed the fix-1527-use-consistent-command-handler-return-pattern branch from 312a1d0 to 7eab9a0 Compare January 25, 2024 22:22
@thnkslprpt thnkslprpt force-pushed the fix-1527-use-consistent-command-handler-return-pattern branch from 7eab9a0 to b9557b4 Compare March 24, 2024 08:31
@thnkslprpt thnkslprpt force-pushed the fix-1527-use-consistent-command-handler-return-pattern branch from b9557b4 to 0c41692 Compare July 9, 2024 19:48
@thnkslprpt thnkslprpt force-pushed the fix-1527-use-consistent-command-handler-return-pattern branch from 0c41692 to 3b8404a Compare July 9, 2024 20:02
@@ -68,8 +68,8 @@
uint8 Bytes[UT_TBL_LOAD_BUFFER_SIZE];
} UT_TBL_LoadBuffer;

void * Tbl1Ptr = NULL;
void * Tbl2Ptr = NULL;
void *Tbl1Ptr = NULL;

Check notice

Code scanning / CodeQL

Global could be static Note

The global variable Tbl1Ptr is not accessed outside of tbl_UT.c and could be made static.
void * Tbl1Ptr = NULL;
void * Tbl2Ptr = NULL;
void *Tbl1Ptr = NULL;
void *Tbl2Ptr = NULL;

Check notice

Code scanning / CodeQL

Variable scope too large Note

The variable Tbl2Ptr is only accessed in
Test_CFE_TBL_GetAddresses
and should be scoped accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement single pattern for command handler returns
1 participant