nasa / cFE

The Core Flight System (cFS) Core Flight Executive (cFE)
Apache License 2.0
402 stars 198 forks source link

Fix #2564, add config tool for platform-specific settings #2565

Open jphickey opened 1 month ago

jphickey commented 1 month ago

Checklist (Please check before submitting)

Describe the contribution Improve the config module to handle platform-specific definitions from the "cfe_platform_cfg.h" file. Specifically this can generate static tables/lists for items that cannot be simply handled via the C preprocessor. Notable examples are the mem pool size lists and the allowable processor/spacecraft IDs in table services.

This utilizes a local tool within the config module to generate some small C code snippets to define the arrays based on the actual values in the CFE platform config header. This is then compiled and linked with the config module, where other modules can read the value and use the list.

This also adds a new value type to the config module for arrays/lists, which is a pair of values: a size (length of list) and a pointer to the first element of the list.

Fixes #2564

Testing performed Build and run all tests, functional and coverage, sanity check CFE

Expected behavior changes List sizes are properly configurable by the user.

System(s) tested on Debian

Contributor Info - All information REQUIRED for consideration of pull request Joseph Hickey, Vantage Systems, Inc.

skliper commented 1 month ago

Interesting approach. On my project we just wrote the .c config files directly... is "automating" it this way really worth the added complexity (but still very restricted capabilities)? Why not just define the array in a c file, drop it into _defs, and use cfe_locate_implementation_file from the module's CMake file?

In practice it has been very useful and although it's more "free-form" that means it's also more "free-form". No need to add processing levels or anything extra if say the user wants to configure via a custom structure (which we do). It's also doesn't require any knowledge of these preprocessing mechanisms or how it all really works under the hood since nothing is under the hood. Very simple to understand and very powerful with no required changes to the core, we just use cfe_locate_implementation_file from the custom app CMake.

Example snippet from a custom app config.c:

const uint8  DP_ScMIDCount = sizeof(DP_ScMIDs) / sizeof(DP_ScMIDs[0]);

/* Fill in configuration array structure */
const DP_OutputConfig_t DP_OutputConfig[FIFO_LIB_Max_Id] = {
    [FIFO_LIB_CRIS_Id]  = {.MsgLim       = 4,
                           .MIDCount     = DP_ScMIDCount,
                           .MIDList      = DP_ScMIDs,
                           .FileMID      = CF_CHAN1_OUT_MID,
                           .FileSemName  = ZEPHYR_SC_FILE_SEM_NAME,
                           .FilePipeName = "DP_ScFilePipe",
                           .LivePipeName = "DP_ScLivePipe"   },
jphickey commented 1 month ago

With respect to @skliper comments:

What I like about this solution is that it just works - the table is always supposed to be in sync (that is, there is one and only one "correct" content) - and thus is a prime candidate to be generated on the fly.

Furthermore, by putting them in the "config" module - it means the unit test can have custom values. We've always been fighting issues where the user configuration breaks unit test because the UT assumed it would be configured in the default way. This solves that problem, because the UT can supply its own tables that work with the UT test cases, and UT can use different tables for different test cases.

With this solution -- a) config remains entirely within cfe_platform_cfg.h (or its components) - no new files for the user to be concerned with b) config changes cannot break unit test or cause things to go untested in certain configs (this was the issue in TBL)

skliper commented 1 month ago

RE: @jphickey

Fair, although the approach we're using also "just works", you can easily provide alternate configurations as needed for unit or functional testing, there's no added CMake logic necessary in the core, the defines that go in the array could also just be in one location if just used to set the array values, and it's more flexible since you can have unique config structures. Although user config was typically done in header files I think we've both hit on the fact that's overly restrictive, and both approaches add a c file it's just one is autogenerated. Also it's somewhat a slippery slope w/ an autocoding approach since every additional config concept requires changes in the core vs just add whatever makes sense to a custom c file without the dependency on a layer of interpretation. Coming in fresh it always takes a while to sort out how all the autogeneration stuff works to figure out how to do what really needs to be done (or changed for whatever the use case is) whereas if it's just obvious in a c file it just seems simpler to me. Not that I'm opposed to whatever core cmake logic folks want to add and maintain, it's just that if there's a desire to support config arrays why not also support config structures?

jphickey commented 1 month ago

To clarify - for applications, in most cases I strongly advocate for doing something like an OutputConfig structure as a table that gets loaded via table services. This is already implemented, and gives the user the best flexibility. They can simply write their own .c file (or other format!) and upload that, they don't even need to recompile the code to reconfigure something. That's really the preferred solution, because it isolates the app from its config. All the mechanisms to allow simple user overrides for table files are already there, so its just a matter of having the apps utilize them.

This approach in this PR is strictly for CFE core components - basically things that need to be compiled in to the core and cannot be done via table services. (In particular, table services itself uses/needs some of the config items being proposed with this method)

Historically, CFE core services are configured via compile time headers, and if it is possible to preserve that paradigm for users, then that is preferable IMO.

skliper commented 1 month ago

@jphickey What about configuration with respect to a library (the actual case where it's used in our setup)? Similar to the cFE case where tables aren't available and needed at startup, and the flexibility of being able to initialize a structure is handy. Avoids any concerns related to configuring it from an app (races, etc). Also when there's a desire to not support run time reconfiguration due to significantly complex interactions/dependencies. The table model doesn't seem to explicitly support a load only once concept by design, and although the app could just not ever manage tables the use of a table "implies" the ability to load unless it's dump only, and dump only you can't initialize (load once) via an external table (which seems strange). At least as I understand it.