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

Apps should check/verify msg BEFORE calling handler #75

Closed
3 tasks done
jphickey opened this issue Mar 21, 2023 · 0 comments · Fixed by #87
Closed
3 tasks done

Apps should check/verify msg BEFORE calling handler #75

jphickey opened this issue Mar 21, 2023 · 0 comments · Fixed by #87

Comments

@jphickey
Copy link
Contributor

Checklist (Please check before submitting)

  • I reviewed the Contributing Guide.
  • I reviewed the README file to see if the feature is in the major future work.
  • I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
This remains an area with coding pattern discrepancies between CFE core and CFS apps, and also different between CFS apps to some degree as well.

The CFE core and sample app (which is supposed to be the example of "best practice") do validation on the message before calling the handler. For example:

        case SAMPLE_APP_NOOP_CC:
            if (SAMPLE_APP_VerifyCmdLength(&SBBufPtr->Msg, sizeof(SAMPLE_APP_NoopCmd_t)))
            {
                SAMPLE_APP_Noop((SAMPLE_APP_NoopCmd_t *)SBBufPtr);
            }

This is different from CS, which does a similar check, but done inside each handler, for example:

CS/fsw/src/cs_cmds.c

Lines 69 to 79 in a249bcf

void CS_ResetCmd(const CS_NoArgsCmd_t *CmdPtr)
{
/* command verification variables */
size_t ExpectedLength = sizeof(CS_NoArgsCmd_t);
/* Verify command packet length */
if (CS_VerifyCmdLength(&CmdPtr->CmdHeader.Msg, ExpectedLength))
{
CS_AppData.HkPacket.CmdCounter = 0;
CS_AppData.HkPacket.CmdErrCounter = 0;

Describe the solution you'd like
CFS Apps should follow the best practices/patterns set forth in the framework code. (there are reasons for the pattern being recommended practice)

Additional context
The pattern recommended in the framework (checking before calling, as done in sample_app) has several advantages:

  1. Each command handler function has a unique type-safe prototype, that cannot be interchanged with another handler without triggering a type mismatch compiler error.
  2. All typecasting/conversions are confined to one place, and it is nearby to the place that the verification is done - which eases maintainability because check and conversion are all in close proximity and any mismatches will be more visible.
  3. It spreads out the cyclomatic complexity. In the non-recommended pattern, there is a case where the length check fails, and the entire handler is essentially skipped. This adds to the cyclomatic complexity of every handler. In the recommended pattern, those checks are done prior to the invocation of the handler, so the handler can focus solely on its intended purpose - doing the command itself.

Requester Info
Joseph Hickey, Vantage Systems, Inc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants