(WIP) Trivial header redefine

[skliper]

Describe the contribution
Partially address #554
Just does the trivial redefine of a header

Since no source selection, requires all the elements that are referenced to still exist but allows moving, adding, etc. Once source selection is available, can redefine handling or remove fields (update API implementations).

Note, depends on apps actually using the real SB header definitions for this to work (size will fail if using CFE_SB_CMD_HDR_SIZE).

NOT FOR MERGE - this actually shows an example of redefining header, don't merge!

Testing performed
Built with redefined header, used updated cmdUtil to test that the field was added as expected.

Expected behavior changes
Just an example, but adds a field to the header

System(s) tested on

• Hardware: cFS Dev Server
• OS: Ubuntu 18.04
• Versions: Bundle + this commit, updated cmdUtil

Additional context
See #554 for context

Third party code
None

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

[skliper]

Note refactoring ccsds.h/cfe_sb.h would allow this to be a cleaner include, but not investing too much until this is reviewed.

[jphickey]

Comments noted.

[jphickey]

Why would we need a compile time switch for custom/not custom header format? Every mission needs one (and only one) defined message format. It should just be "the selected message format".

I assume "not custom" means the traditional CCSDS v1 with CFE-defined secondary headers, using the 6-byte TLM header? Does that mean every user who wants v2/extended header needs to enable this "custom" switch?

I'd rather see something where the user just specifies one specific header format, by name. If that happens to be one that CFE provides out of the box, then great. Otherwise the user will have to provide an implementation to go with their selection.

[skliper]

You want to break everyone's config if they don't define version 1? We certainly can take that approach if it's preferred. I went with minimizing impacts (it works just like default (v1), define for V2 as before, but adds an additional IS_CUSTOM).

Could also have unique typedefs for each option, and just define as a common name here. Is that what you are asking for?

[jphickey]

No it doesn't break existing config because it can be done on the CMake side. If CMake needs to do some sort of source selection based on the header format then CMake needs to know what the user wants, so it can select the files appropriately.

So it would work just like anything else - CMake has a default, which is used if the user does nothing, and if the user sets something then it uses that.

Besides that, even if it was only in the C header file, it's just a matter of putting the default value into the sample file. (but again, setting in the value via CMake is totally backward compatible provided we give a default)

[skliper]

Yes, it could certainly be done multiple ways and I was hoping for a CMake related update (per the design suggestions I provided) but I haven't seen progress on that front in line with our allotted schedule. Given that, I made these trivial changes that also work to redefine the header. This is certainly not meant as the perfect solution, it's just a trivial solution that works so I can still make schedule if a better solution is not complete in time.

[jphickey]

As the "cfe_mission_custom.h" file needs to exist regardless, couldn't it just be generated to always have the "right" content in it (i.e. the user-selected format) and not have to introduce yet another preprocessor switch?

[skliper]

No it doesn't? Did you try it? Works exactly like before (remove the file, don't define a format and it's V1, define V2 and it works also... I confirmed it. Only needs to exist if you want to redefine.

[jphickey]

My point is that you've called generate_config_includefile() function to create cfe_mission_custom.h .... but in the event that there is no config here the file will just be empty.

So why would we want to generate an empty file and then selectively ignore it? It would be simpler, cleaner to just generate the file with the right content. No #ifdef's or parallel settings required - just include the generated header file, and it has the right content.

[jphickey]

Also note that the whole generate_config_includefile stuff was originally for backward compatibility with the classic build where things were configured via header file. While that is still largely the case for CFE because it generally works, there are better methods of doing this in CMake (i.e. how we updated OSAL/osconfig.h recently). It isn't really a method I'd suggest for new development.

[skliper]

I agree 100% with adopting the source selection and CMake techniques implemented in OSAL. I have suggested that approach multiple times, but it hasn't seemed to have resonated with the group. For the previous comment - because it works with no impacts on previous configurations. There's certainly a trade to be made (it can certainly be implemented either way), I'll leave it up to @jwilmot @acudmore since the long term open source solution would help from architecture guidance.

[jphickey]

Well since we already have both (CMake config file + C headers) in use my recommendation is to favor the CMake method for new development, because it covers both areas (build + code).

That is, we can control the build and also easily generate a synchronized C header from the CMake variables to influence the actual code. Using a strictly C header approach is more limiting, as it is not really possible to go the other way, so the information isn't available to CMake for build/source selection.

[skliper]

And I continue to agree 100% on improving this to utilize CMake instead of defines in c headers. It was the solution I was hoping for and was trying to encourage.

[skliper]

I do still prefer the core framework implements the basic two formats in the code such that it's covered by cppcheck (I'm not a fan of moving the supported header definitions into sample_defs and generating, but that's a trade we can discuss), but I definitely prefer using CMake to do the configuration work.

[skliper]

My point is I don't want users to think they can redefine what version 1 and 2 means (could be implied by moving the definition to sample_defs). I'd rather control those in the core code.

[skliper]

At closer inspection, for my use case I really just need to redefine CCSDS_CmdSecHdr_t and CCSDS_TlmSecHdr_t... working on shifting this example.

Reduces scope of impact, and really these fields are not CCSDS standards based and I'd suggest really should be reconfigurable.

[skliper]

I actually really prefer more of a general solution as suggested in #724 (comment)

Recommending cpu1_*.h gets a generic generate_include_configfile treatment and becomes *.h early in the include path. Then refactoring ccsds.h could make it trivial to customize the secondary header, or full header, or whatever. This also addresses mission configuration of app header files...

[astrogeco]

@skliper when would you like to bring this to the CCB?

[skliper]

Closing this example/thread... see discussion on #728. Power users can just include from the target defs directory if they are OK with multiple files with the same name in the tree, alternatively #751 provides a mechanism to generate_config_includefile from within apps (and I assume modules).