Fix #142, Standardize command responses #143
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fsw/src/sc_cmds.c
Outdated
| @@ -702,12 +718,12 @@ | |||
| /* */ | |||
| /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ | |||
|
|
|||
| void SC_ManageTable(SC_TableType type, int32 ArrayIndex) | |||
| CFE_Status_t SC_ManageTable(SC_TableType type, int32 ArrayIndex) | |||
Check notice
Code scanning / CodeQL-coding-standard
Long function without assertion
fsw/src/sc_cmds.c
Outdated
| @@ -683,17 +694,22 @@ | |||
| /* */ | |||
| /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ | |||
|
|
|||
| void SC_ManageAtsTable(int32 ArrayIndex) | |||
| CFE_Status_t SC_ManageAtsTable(int32 ArrayIndex) | |||
Check notice
Code scanning / CodeQL-coding-standard
Long function without assertion
fsw/src/sc_cmds.c
Outdated
| @@ -664,17 +670,22 @@ | |||
| /* */ | |||
| /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ | |||
|
|
|||
| void SC_ManageRtsTable(int32 ArrayIndex) | |||
| CFE_Status_t SC_ManageRtsTable(int32 ArrayIndex) | |||
Check notice
Code scanning / CodeQL-coding-standard
Long function without assertion
fsw/src/sc_cmds.c
Outdated
| @@ -702,12 +718,12 @@ | |||
| /* */ | |||
| /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ | |||
|
|
|||
| void SC_ManageTable(SC_TableType type, int32 ArrayIndex) | |||
| CFE_Status_t SC_ManageTable(SC_TableType type, int32 ArrayIndex) | |||
Check notice
Code scanning / CodeQL-coding-standard
Function too long
|
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? |
thnkslprpt
force-pushed
the
fix-142-standardize-command-responses
branch
6 times, most recently
from
0bea4eb
to
6db7f5d
Compare
Good to go now @dzbaker |
fsw/inc/sc_events.h
Outdated
| * This event message is issued when a #SC_MANAGE_TABLE_CC command was received | ||
| * and executed without error | ||
| */ | ||
| #define SC_MANAGE_TABLE_INF_EID 111 |
There was a problem hiding this comment.
@thnkslprpt What was the reason for adding this EID?
There was a problem hiding this comment.
It was added in order to address the comment in the issue from Dan:
"SC_MANAGE_TABLE_CC command does not issue success events in all success cases.", but if Jose's comment below regarding flooding is an issue, I can remove the event add a comment to that effect.
fsw/src/sc_cmds.c
Outdated
| @@ -773,4 +775,9 @@ void SC_ManageTable(SC_TableType type, int32 ArrayIndex) | |||
| } | |||
| } | |||
|
|
|||
| if (Result == CFE_SUCCESS) | |||
| { | |||
| CFE_EVS_SendEvent(SC_MANAGE_TABLE_INF_EID, CFE_EVS_EventType_INFORMATION, "Table manage command."); | |||
There was a problem hiding this comment.
@jphickey Is it possible we never sent a success event here because it floods the system whenever running an RTS (CFE TABLE sends this command whenever a table needs to be managed)? We also don't increment the command success counter?
There was a problem hiding this comment.
I don't understand, could you elaborate? Specifically what's the flow that would cause flooding and what's the relation to CFE TIME?
There was a problem hiding this comment.
@skliper Oops, not CFE TIME, CFE TABLE Services. I'm not sure if it will flood the system or not, I haven't tested it. But I assumed that was the reason why we never added an event for this command. Will this get called only when a table is updated externally from SC? In that case, it might not be a problem.
There was a problem hiding this comment.
@pepepr08 Oh, yeah I see now where SC calls CFE_TBL_NotifyByMessage to register the SC_MANAGE_TABLE_CC with CFE Table services. I wish it was using a different MID so it's obvious it's not a ground command. Either way, should only get called when a table is updated using table services (worth testing to confirm though). Interestingly SC is the only common/shared app I could find that uses the NotifyByMessage API. Either way since it's really just internal TBL/SC coordination I don't think it falls under the "common ground command handling" need to send an event when it happens. I'm not aware of any other app that reports table management in an event.
There was a problem hiding this comment.
Yes correct - no other apps have an event for table handling, and it was only noted in the issue here because SC is the only app that handles tables as part of the processing of a direct command (SC_ManageTableCmd()).
So for now I'll remove the event, and add a comment noting that it is an exception.
thnkslprpt
force-pushed
the
fix-142-standardize-command-responses
branch
from
6db7f5d
to
88c6703
Compare
Checklist
Describe the contribution
SC_RESET_DEB_EID,SC_DISABLE_RTS_DEB_EID,SC_ENABLE_RTS_DEB_EID&SC_CONT_CMD_DEB_EIDtoINFORMATIONtype- Adds new event (SC_TABLE_MANAGE_INF_EID) and required logic to theSC_ManageTableCmdandSC_ManageTablefunctions in order to return a success event when no errors were encountered during processing (either from the SC functions or passed through from cFE functions)- Updates nominal manage table tests to include a check forSC_TABLE_MANAGE_INF_EIDNote: the issue requested the addition of an event for the
SC_MANAGE_TABLE_CCcommand, but given the risk of flooding, and the fact that issuing an event for table processing is out-of-family with the open-source cFS apps, it has not been included in this pull request.Testing performed
GitHub CI actions all passing successfully (incl. Build + Run, Unit/Functional Tests etc.).
Expected behavior changes
Success event will be issued if no errors are encountered during processing of the
SC_MANAGE_TABLE_CCcommand.System(s) tested on
Debian GNU/Linux 12 (bookworm)
Current main branch of cFS bundle.
Contributor Info
Avi Weiss @thnkslprpt