Fix #45, Refactor to implement command and utility functions in separate files

[ejtimmon]

Describe the contribution
Addresses ticket #45. Specifically adds new sample_app_cmds.c/.h and sample_app_utils.c/.h files, moves functions out of sample_app.c into the new cmds and utils files. Updates unit tests.
This change makes sample_app a better reference application - structure more closely matches other apps.

Testing performed
Steps taken to test the contribution:

1. App compiles
2. App unit tests complete successfully

Expected behavior changes
No change to sample app behavior. Minor changes to unit test behavior (more test cases).

System(s) tested on

• Hardware: PC
• OS: Ubuntu 16.04
• Versions: cFE 6.7.12

Additional context
Add any other context about the contribution here.

Third party code
N/A

Contributor Info - All information REQUIRED for consideration of pull request
Elizabeth Timmons/NASA

[skliper]

We are transitioning away from aux_source_directory use, Preference is to explicitly list sources.

[jphickey]

A few additional comments. Also second @skliper 's comment about not using aux_source_directory anymore - CMake docs recommend against this for a few reasons, we've also been bitten by this picking up more files than it should have in a few places.

[jphickey]

All the sanity-checks on the message structure including the "verify length" test should be kept out here in the command dispatcher, not inside the command handler. Conceptually, the cast to the real msg type is unsafe unless it passes this sanity test first - hence why we shouldn't cast until confirming this is true.

Otherwise we are violating basic type-safety stuff by not only casting a buffer to a type that isn't actually an instance of that type, and proceeding to dereference items within that improper cast.

[ejtimmon]

Agree with the rationale, but would prefer to fix by not casting the argument to the command function. In the other GSFC apps, all command verification, including length verification, is done in the command handling function. The cast is made in the command handling function only after checking the length.

[jphickey]

Its all about gradually migrating to patterns that allow more type safety and more compiler enforcement of this type of thing.

The point of casting here is to actually give each message/command handler a unique (that is, type safe) signature. This was an intentional pattern (discussed in CCB meetings around the CFE 6.6 development timeframe) to ensure that a function pointer to one message handler would not be the same "type" as a function pointer to another message handler, and it would trigger a compiler error if the wrong type was passed through, or if it was mistakenly cast to the wrong type, or something like that.

The other aspect of this pattern in the current dispatcher code is that keeps the cast and the size check "near" each other source-wise - that is, the "sizeof()" in the size check should be the same type as the cast in the dispatch, and if its not the same, its an obvious red flag to anyone looking at it. If they are in different files, its not nearly as obvious.

Lastly, its been the intent to encourage a generic pattern that can eventually migrate to a more intelligent dispatch based on some sort of command/data dictionary. See my related comment here: nasa/cFE#263 (comment)

The intent with the sample is to exemplify the "recommended practice" -- if other GSFC apps are not implementing it exactly this way that's OK, they don't need to change, but all the CFE core apps do follow this pattern and there is a good reasoning/basis behind it. We could always re-discuss these patterns (as I said they were from CFE 6.6 CCB discussions) but I think the reasons behind the patterns are still valid.

[jphickey]

The other thing worth considering is that typecasting is generally a "risky" operation code wise, in that it has to be done with care and verification. In that sense, when it has to be done, it is good to do it in limited/consolidated places, where one can clearly see that all the correct validation is done. I don't like to see casting "scattered" across code in many different places. If we have to typecast, it is good to keep all the typecasting in one function, letting every other function be governed by the normal type safety rules.

[ejtimmon]

Ok, if there have already been the discussions on design patterns, I’ll change the sample app back. One thing I do want to be careful of is sending mixed messages to users. I don’t want a user to look at the sample app and at another GSFC app and be left wondering why the design patterns are different, or which option is preferred.
Maybe there is guidance we can add to the app developer’s guide?

[jphickey]

Ideally sample_app should just reflect the recommended pattern for new development, certainly would not expect older/existing apps to be retrofitted, unless one was doing a significant refactoring to the app anyway and wanted to update it.

The design discussions occurred a couple releases ago, so they could certainly be revisited. Although it is different than how it was historically done in apps, there were some advantages the pattern that CFE core and sample_app implement, which I think are still valid/applicable. New development should (hopefully) follow it but there is no expectation of updating existing apps in any particular timeframe.

[ejtimmon]

@jphickey Made the requested changes.

[astrogeco]

CCB 20200506 - APPROVED, will probably conflict with #54

[astrogeco]

@ejtimmon can you resolve the conflicts?

[skliper]

@ejtimmon Recommend rebasing your changes on top of IC... merging into your branch makes merges harder. Also recommended commit message format:

Fix #111, Short description

Long description if needed

[astrogeco]

Removed from IC 202004-29 COMBINED
Put commit in new branch fix-45 and will add a new pull request for that.

[skliper]

Resubmitted via #65