Fix #711, Separate secondary header access functions

[skliper]

Describe the contribution
Fix #711 - Separates header manipulation and definitions for easier mission customization
Fix #733 - Fixes validate checksum description to match implementation
Fix #736 - Fixes get msgid logic to not override bits
Fix #597 - Removes local endian SID macros
Fix #529 - By adding an API that does support maximum msg size reporting, note also fixes limit in CFE_SB_InitMsg by the same means.
Fix #781 - Fixes set/get message id logic to not override bits

• Enables source selection and out-of-tree mission defined overrides in the msg directory.

NOTE - There are follow-on tickets to finalize and clean up after this change.

Testing performed
Ran unit tests for version 1 and version 2 header formats, all pass except sample_app due to a different bug (nasa/sample_app#87)

Expected behavior changes
This just enables override. No actual changes in behavior other than the bug fixes. Some name changes in structure fields, but apps shouldn't be directly accessing those fields to begin with.

System(s) tested on

• Hardware: cFS Dev Server
• OS: Ubuntu 18.04
• Versions: master bundle + this commit (and the other branch configured to override)

Additional context
None

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman - NASA/GSFC

[skliper]

Note - the override just allows use of the full 8 bits for function code, vs the cfe default of 7. Really just using as an example of how it could be easily customized.

[skliper]

There's still quite a bit of cleanup/refactor that could be done relative to ccsds.h and ccsds.c, and I'm sure many improvements in how I hacked in the CMake changes... but curious to hear if this is a step in a direction that we could build on. It's an attempt at a trivial implementation to enable both the immediate needs and future enhancements.

[jphickey]

It's OK as a first step - but there are some things I'd definitely want to address before merging.

Main recommendation is to avoid adding to the public API in haste -- things added to the public API are hard to revoke later so we want to take the time to do it right and really design the interface to be flexible and consistent in its return values and inputs/outputs, and to be const-correct, etc. I'd be more comfortable with keeping this as a private/internal API for the time being which can be accessed through a "backdoor" (if users really want to) while buying us time to design a public API that works, rather than just copying what SB had.

[skliper]

It's OK as a first step - but there are some things I'd definitely want to address before merging.

Excellent improvements, thanks!

[CDKnightNASA]

It's OK as a first step - but there are some things I'd definitely want to address before merging.

Excellent improvements, thanks!

Something else to consider--while 100%-1/infinity% of apps should be manipulating headers via the official methods, I have two cases (SBN, and an app I wrote years ago that has languished but should still be useful in the future called "ds_replay", which [as its name probably makes clear] takes ds-generated files and replays the messages back onto a bus) where I need to be able to push messages on to the software bus without having to go through all of the header fields.

Architecturally, these could be simply via a "private" API that SBN and ds_replay sneak through.

[skliper]

Architecturally, these could be simply via a "private" API that SBN and ds_replay sneak through.

We can certainly provide an API that just puts the unmodified message on the bus (isn't this the intent of CFE_SB_PassMsg?) Vs SendMsg which could be signing the message or adding checksum, or whatever modifications get plugged in under the hood.

[jphickey]

Architecturally, these could be simply via a "private" API that SBN and ds_replay sneak through.

We can certainly provide an API that just puts the unmodified message on the bus (isn't this the intent of CFE_SB_PassMsg?) Vs SendMsg which could be signing the message or adding checksum, or whatever modifications get plugged in under the hood.

Note that messages being passed around locally on the same CPU in the same memory space don't really need the checksum at all. It's only required for when the message is sent to some other entity via some sort of interface (SBN, CI, TO, etc).

For this I think the notion of a "subscription flag" works well -- when subscribing to a MID one can indicate whether the intent is to locally cast it to a C struct and interpret/use the data (i.e. as a consumer, the default) or whether the intent is to forward it to another entity across a network (i.e. as a broker). The former needs alignment padding to allow it to be cast/accessed directly as a C data type, but the latter would get a "packed" version that removes the padding and can also recompute the checksum. (as I noted earlier the checksum of a padded/aligned message will likely be different than an unpadded/unaligned one).

[skliper]

Ok, b5c8be6 is the next step.

Still on the docket - minor cleanup of MSG future APIs, refactor of ccsds.h to make replacement of secondary header easy, further progression towards msg being a "module"... as in have an inc_public, inc_private. There is quite a bit of possible code cleanup, not sure how much I'll get to. Trying to limit to what's required or at least only changing what bugs me the most.

Unit tests not fixed yet. Did test with a custom mission_msg and it worked well (both append and replace). Eventually recommending a transition of the rest of the core over once a good pattern is established.

Could have modules own their stubs, unit tests, EDS, etc. The directory tree is begging for a simplification (could move modules to cfe/ !)

[skliper]

Note that messages being passed around locally on the same CPU in the same memory space don't really need the checksum at all. It's only required for when the message is sent to some other entity via some sort of interface (SBN, CI, TO, etc).

I have had seen requirements to checksum at the source and verify before any use (within the same memory space). Radiation, security, etc, so I'd prefer if we don't preclude the option of checksumming before it goes on SB.

For this I think the notion of a "subscription flag" works well -- when subscribing to a MID one can indicate whether the intent is to locally cast it to a C struct and interpret/use the data (i.e. as a consumer, the default) or whether the intent is to forward it to another entity across a network (i.e. as a broker). The former needs alignment padding to allow it to be cast/accessed directly as a C data type, but the latter would get a "packed" version that removes the padding and can also recompute the checksum. (as I noted earlier the checksum of a padded/aligned message will likely be different than an unpadded/unaligned one).

I like the "subscription flag" concept. I'd like to stay flexible in terms of what format is actually on SB, since I can see benefits from each approach. If it's "packed" on the bus, it can get the checksum or signature at publish, and any subscriber can authenticate prior to use (nobody allowed to modify the packed message). Then the packing is owned under the hood of the publish, vs packing owned by each endpoint. Others may want local representation for performance reasons to avoid packing/unpacking if there is extensive local use of the message.

[skliper]

Whoops... missed files in my commit, stand by.

[skliper]

Now ready for a 2nd look if interested. This update actually allows me to do the header replace and implementation customization without additional changes... 844f8af

[jphickey]

Still have some things I'd like to work out ... for the search action I'd rather not re-implement this, really should be no different than any other module. Can we reconsider PR #720? When combined with #728 I think it will work just fine to do this job and not require re-implementing a separate search path and other mechanisms that are already there.

[skliper]

Marked as ready for an initial CCB discussion (still work in progress, but a chance for comment/review)

[jphickey]

Did a quick skim though - but still have a multitude of questions.

First and foremost it is not clear to me how this is really extensible - it changes the names from CFE_SB namespace to CFE_MSG namespace, but how does one extend this? Wouldn't the same problems with extensions exist in the new namespace, or are we back to a full-blown clone and own? (i.e. to "extend" would someone have to clone the whole CFE_MSG library and change its content? If so, then we'd have multiple code blobs with the same namespace)

I was hoping to see an example of how this approach would get rid of MESSAGE_FORMAT_IS_CCSDS_VER_2 and CFE_MISSION_SB_PACKET_TIME_FORMAT compile options. I thought one of the goals was to remove these but they are still there.... (this was one of the objectives of my original idea in #716).

[jphickey]

Also as another topic of discussion, it is clear that this approach is favoring "override" at a source file level, where the user implements an alternative version of a struct/function but calls it the same name, then uses the build system to build the override file instead of the original.

This approach works but has some drawbacks:

• One is left with multiple definitions of the same symbol/type, it isn't readily clear which one is "active". This is especially true when grepping or navigating in an IDE. If there are just a few then it might be OK but if it is used frequently it can get messy.
• It requires that the "overridable" parts are already designated at a file scope level. In other words, one has to override an entire source file. If the part that needs to be changed isn't already in a separate source file, then its back to a clone and own type of paradigm.

The alternative approach is to embrace namespaces and let the user supply the override, but have them intentionally name it differently, with a different namespace, for the item they are overriding.

Advantages:

• Can compile and link in both versions simultaneously. The "override" can even invoke functions in the original file where relevant, so it is trivial to make an extension. The linker is generally smart enough to figure out what (if any) functions are not used and prune them out, so you will not be left with dead code if it isn't called.
• No need to worry about having the parts being overridden separated into different source units. Easy to mix and match, no restrictions on what you can and cannot do.
• One name, one definition. It is very clear when a direct reference exists to a type/symbol, what the actual definition/implementation is - except in one specific layer designed for this purpose (see below).

Disadvantages:

• Needs to employ an adapter layer or translation to provide a consistent API up to apps/higher level modules, to translate the namespace to the user-supplied override. However that can be mitigated by having the build system generate some macros/typedefs to aid in the translation, such that the final interface API (be it in SB or separate MSG API) can be transparent to users.

I would recommend considering this approach as I think it is more flexible and clearer than having (potentially) many symbols in the source tree with the same names.

[skliper]

I would recommend considering this approach as I think it is more flexible and clearer than having (potentially) many symbols in the source tree with the same names.

I did consider your example and I prefer the method implemented here (probably partly because of internal bias seeing as I did write this one). Much like how source selection is done in the OSAL, just compile in the implementations you need (network vs no network for example). I was not a fan of macro's to define function names. Aren't cmake smart IDE's pretty good at working with interface libraries, other includes, and sorting out the code selections?

The actual source files are listed as part of the build, and it only builds what's selected. That's preferable from my certification perspective.

[skliper]

Squashed for review.

[jphickey]

I've already voiced my opinion on some of the earlier design decisions, but I am still concerned that this isn't really "clean" from an extensibility point of view. An extension really shouldn't be touching the internals/private components of the item it is extending. A future update to the base library can too easily break the extension.

I do think we should at least consider breaking up the "msg" module into separate modules focused on each header layer, as this will help the modularity and extensibility, allowing an override/extension to still use the same module for the layer(s) it is not changing where the source for said module is built in only one script. I don't like the idea of cherry-picking some (but not all) of the msg module sources inside other modules. I'd rather see smaller, focused modules that can be used more directly as building blocks.

That being said, I know some projects are waiting on this so if this isn't seen as a big issue, I'm otherwise OK with it.

[jphickey]

I would not recommend referring to the ${DEP} variable in these scripts - it is really a local loop variable in the parent script, it is just unfortunate that CMake doesn't have proper variable scoping so it is still set in the sub-script here. The variable name could change in a future rev - or if we want to keep it, it should be noted in a very obvious comment in the parent script if sub-scripts depend on the variable ${DEP} being set.

[skliper]

I only see one suggestion which I'm fine with (document), but it seems like you had an alternative in mind?

[jphickey]

The alternative would be to use a fixed name (msg) and not make it variable. Is there an expectation that this will be compiled under a different library name? A compromise might be to set your own variable name at the beginning, and use that variable for the rest of the script. Depends on the expected use-case for compiling the same library under a different name.

[skliper]

I'd think hard coding it would be a less desirable solution. The name should match the module name such that the system can utilize common patterns. I'd rather avoid setting up a potential for the name to not match.

[jphickey]

Have we evaluated the possibility that the single msg perhaps should actually be three:

• ccsds spacepacket (base)
• ccsds extended
• cfs cmd/tlm

I reviewed the internal "extension" library and noted that it is effectively replacing the entire msg module with a different one, but then selectively re-compiling and referring to some (but not all) source files provided by the msg module, as well as giving itself an include path to the private headers within this module.

The paradigm should be that external entities don't depend on private headers - nor should source files be compiled by a script that isn't version controlled with the said source files.

It would seem, however, that if it the "msg" module were actually three different modules, then only one would need to be replaced for modifying the secondary header, and the other two (ccsds + extended) can be used as-is.

[jphickey]

For "setter" API's, it might be worth considering passing by pointer (e.g. const CFE_MSG_ApId_t* rather than by value. A pattern of passing by value kind of assumes that each logical "field" is just a single datum, like an int or a float. If this becomes a struct or string this could get very inefficient. But if it were a pointer then it could accept any data type.

[skliper]

@tngo67 @excaliburtb - requesting feedback on suggestion to break up msg module.

Options:

• Leave as is, customization implementations can either cherry-pick (fragile) or clone and own the entire module (to avoid dependence on core internals)
• Break up implementations into building blocks, customization has the same option to cherry-pick or clone and own, but limited to just the block being customized
• Or leave for now and move forward with merging this PR, open an issue to break up at some convenient time in the future?

[tngo67]

@tngo67 @excaliburtb - requesting feedback on suggestion to break up msg module.

Options:

* Leave as is, customization implementations can either cherry-pick (fragile) or clone and own the entire module (to avoid dependence on core internals)

* Break up implementations into building blocks, customization has the same option to cherry-pick or clone and own, but limited to just the block being customized

* Or leave for now and move forward with merging this PR, open an issue to break up at some convenient time in the future?

Here's my 5-cent..
If the current implementation works, and we can get the issue resolved by v7.0 for Gateway, then I vote for option #3.
The clone & own in option #1 seems to be more in-line with the current implementation approach for OSAL & PSP, correct?
Although option #2 has more flexibility, I'm not clear yet if it adds more complexity to the cFE build and certification.

[skliper]

in-line with the current implementation approach...

I see it as the same, in that using elements in osa/src/os/shared or portable in a custom OSAL requires the same "cherry-pick" approach.

[astrogeco]

CCB 2020-08-19 APPROVED, @skliper will open new issue to break down into small pieces in the future

[excaliburtb]

I think splitting up the different forms of the ccsds header and corresponding elements is probably a case of over-abstraction. I would keep the implementation as-is and let a customer need drive more abstraction later, though they technically should have the tools they need with the current abstraction.

[jphickey]

Note that splitting the monolithic library into smaller libraries has nothing to do with abstraction level (over or under). It is just structuring the library content so one can more easily re-use the libraries in applications/extensions without cherry-picking individual source files out of them (which is not the right way to use a library).

If it works as-is for what you want to do right now that's perfectly fine and I won't stand in the way. I just wouldn't recommend it for general purpose use. For every module the stable-ish interface (if there is one) is via the public API, not the internal source file structure.