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 #297, base offsets for CF msgids #298

Closed
wants to merge 1 commit into from

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Aug 2, 2022

Checklist (Please check before submitting)

Describe the contribution
Define the CF msgids as an offset from the CFE_PLATFORM_CMD_MID_BASE or CFE_PLATFORM_TLM_MID_BASE, which helps simplify configuration.

Users should add MSG offsets to the global mission config header.

Fixes #297

Testing performed
Build and run CF app

Expected behavior changes
MsgIds for CF can be more easily customized by setting MSG offsets in the global cfe_mission_cfg.h header file, the same way other CFE framework module msgids are set. User does not need to modify this file in place to do so.

System(s) tested on
Ubuntu 22.04

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

Define the CF msgids as an offset from the CFE_PLATFORM_CMD_MID_BASE or
CFE_PLATFORM_TLM_MID_BASE, which helps simplify configuration.

Users should add MSG offsets to the global mission config header.
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Aug 2, 2022
Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

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

Not quite convinced a change in one file in one app is appropriate/sufficient to address the general problem of platform/mission configuration overrides. II guess it fixes the stated issue but I'm not a huge fan of the one-off nature. Could just use the generate_config_includefile to solve the immediate problem (and implement across all actively maintained apps) and address the transition to topic ids as a more thorough/future change.

If we do accept this concept, maybe XXX_MID_OFFSET, or XXX_TID (Topic ID?) or OMID (Offset MID)? I do like eventually switching to the topic concept and not need to recompile a the app just for a MID change, so I'm in favor of using names that lead us down that path if we are going to make updates like this.

@skliper
Copy link
Contributor

skliper commented Aug 2, 2022

Side note, whichever way we go I think we should still provide the override capability for whichever direction we choose. There's enough different MID management schemes that I don't think forcing one approach is the best fit. Within some toolchains it may be easier to have one full MID list and mask them to get the topic or whatever (just making up use cases...) Which I guess is still possible here but adds layers of abstraction when just one or two would be enough, and we don't need to require users to use this file.

@jphickey
Copy link
Contributor Author

jphickey commented Aug 3, 2022

Yeah I fully concur that this is not a real complete solution at all - its just a baby step, in one app. But this is the way that CFE itself does it, and IMO its easier to manage because offsets can be defined globally.

With the way this is done, its still evaluated at compile time, not run time - so changing a MID still requires rebuilding the app. I'd definitely prefer a solution where the MID determination was done more at runtime than compile time to avoid having to rebuild apps to update this stuff, but this PR doesn't do that.

I'm perfectly fine if this is deferred/not merged, I can just patch my own build in the meantime - but the problem is that every user of CF likely has to also patch their build too, and its not the easiest thing to manage in a deployment.

@dzbaker
Copy link
Contributor

dzbaker commented Aug 3, 2022

CCB 3 Aug 2022: Deferring for now.

@skliper
Copy link
Contributor

skliper commented Aug 3, 2022

CCB:20220802 - Deferring this as a point fix in preference for more global fixes to header file customization and topic ids.

@skliper skliper added CCB:Ignore and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Aug 5, 2022
@skliper
Copy link
Contributor

skliper commented Aug 31, 2022

In theory you can just override as documented in nasa/cFS#403.

@jphickey
Copy link
Contributor Author

Superseded by #423

@jphickey jphickey closed this Dec 13, 2023
@jphickey jphickey deleted the fix-297-msgid-offset branch January 16, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use generic "cf_msgids.h" file that uses offsets from base MID
3 participants