Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

jphickey
Copy link
Contributor

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.

@jphickey jphickey force-pushed the fix-2564-macrolists branch from 6be1fc9 to e749c8c Compare May 21, 2024 14:41
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL-coding-standard found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@skliper
Copy link
Contributor

skliper commented May 21, 2024

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 jphickey force-pushed the fix-2564-macrolists branch from e749c8c to 7f935fc Compare May 21, 2024 19:24
@jphickey
Copy link
Contributor Author

With respect to @skliper comments:

  • Complexity isn't really that much. I realize this looks like a big changeset but a lot of it has to do with enhancing the config module itself, which is a generally useful feature (I think) to be able to have a single config item be a list. Beyond that, it moves some prototypes, making separate headers, etc to make everything conform to the current conventions.
  • The "mission-prebuild" step already existed and was intended for this sort of purpose (generating files to be used during build)
  • Although it could be done with something like having a user-supplied .c file -- the problem I see its that the relationship is not obvious. All user config was typically done in header files, not source files. What I don't like is an undocumented relationship where a change to one file requires a (manual) change to another file to keep them in "sync" or else you get undefined results.

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)

@jphickey jphickey force-pushed the fix-2564-macrolists branch from 7f935fc to b9a5621 Compare May 21, 2024 20:57
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.
@jphickey jphickey force-pushed the fix-2564-macrolists branch from b9a5621 to af12ac1 Compare May 21, 2024 21:00
@skliper
Copy link
Contributor

skliper commented May 21, 2024

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
Copy link
Contributor Author

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.

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label May 22, 2024
@skliper
Copy link
Contributor

skliper commented May 29, 2024

@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.

@dzbaker dzbaker added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Aug 1, 2024
dzbaker added a commit to nasa/cFS that referenced this pull request Aug 2, 2024
*Combines:*

cFE equuleus-rc1+dev183

**Includes:**

*cFE*
- nasa/cFE#2579
- nasa/cFE#2565
- nasa/cFE#2584
- nasa/cFE#2585
- nasa/cFE#2586
- nasa/cFE#2326

Co-authored by: Tvisha Andharia <[email protected]>
Co-authored by: Anh Van <[email protected]>
Co-authored by: Joseph Hickey <[email protected]>
Co-authored by: Avi Weiss <[email protected]>
@dzbaker dzbaker merged commit 7885b8d into nasa:main Aug 2, 2024
22 checks passed
dzbaker added a commit to nasa/cFS that referenced this pull request Aug 2, 2024
*Combines:*

cFE equuleus-rc1+dev183

**Includes:**

*cFE*
- nasa/cFE#2579
- nasa/cFE#2565
- nasa/cFE#2584
- nasa/cFE#2585
- nasa/cFE#2586
- nasa/cFE#2326

Co-authored by: Tvisha Andharia <[email protected]>
Co-authored by: Anh Van <[email protected]>
Co-authored by: Joseph Hickey <[email protected]>
Co-authored by: Avi Weiss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance config module to support lists/arrays generated at compile time
3 participants