nasa / LC

The Core Flight System (cFS) Limit Checker (LC) application.
Apache License 2.0
30 stars 21 forks source link

Apps should check/verify msg BEFORE calling handler #82

Closed jphickey closed 1 year ago

jphickey commented 1 year ago

Checklist (Please check before submitting)

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 LC, which does a similar check, but done inside each handler, for example:

https://github.com/nasa/LC/blob/2f177ae83a24445d6ab6997682a2ffa71dacbd31/fsw/src/lc_cmds.c#L143-L150

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.