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 #245, Opaque CFE_SB_MsgId_t values #592

Closed
wants to merge 1 commit into from

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Apr 8, 2020

Describe the contribution

Ensures that all CFE core apps outside of SB itself treat the CFE_SB_MsgId_t type as an opaque/abstract value and do not assume it is an integer.

This pull request is primarily a concept/preview of this idea in action for CCB discussion. It does work though and could be merged as-is although I expect some change requests.

Currently the type safe enforcement is enabled based on a compile definition CFE_SB_OPAQUE_MSGIDS. This could be moved into the "omit deprecated" switch instead rather than a new switch.

The type-safe enforcement switches CFE_SB_MsgId_t from being a simple integer value to a structure wrapper, so it cannot be implicitly exchanged with other integers, nor can it be involved in any arithmetic calculation. However, this is a breaking API change due to the way most apps specify their msgid values as integers via #define macros.

Fixes #245

Testing performed
Build CFE for SIMULATION=native (64 bit linux)
Execute all unit tests
Sanity check CFE by booting and sending commands with cmdUtil
In particular check all CFE SB routing info, map info commands and compare file output before and after change to make sure data is the same.

Expected behavior changes
No externally-visible changes (i.e. CMD/TLM, files, tables, etc).
API changes when building with enforcing mode:

  • All CFE SB API operations that accept or return a CFE_SB_MsgId_t (Subscribe, etc) must be supplied with an instance of CFE_SB_MsgId_t only, not an integer. Passing or assigning to a bare integer triggers a compiler error.
  • Macros/conversions are provided to bridge the gap, and allow printing as integer for syslog/events as well as converting from an integer
  • No longer can switch() on a MsgId -- must use CFE_SB_MsgId_Equal and a nested if.
  • This also exposes CFE_SB_IsMsgIdValid() in the public API.

System(s) tested on

  • Ubuntu 18.04 LTS 64 bit

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

Treat the CFE_SB_MsgId_t type as an opaque/abstract value and do not
assume it is an integer.

This change offers two modes of operation, where CFE_SB_MsgId_t is
defined as a simple integer and is backward compatible, or defined
as a type safe structure.  In type safe mode, passing an integer to
an API requiring a CFE_SB_MsgId_t value will result in an error.
The macros and conversion functions can be used with either mode,
allowing a transition for applications.
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 8, 2020
@skliper
Copy link
Contributor

skliper commented Apr 8, 2020

Please separate exposing CFE_SB_IsValidMsgId changes into a different pull request. It's fixing #263 and a priority for this round of certification.

@skliper
Copy link
Contributor

skliper commented Apr 8, 2020

@jwilmot @acudmore - Requesting a full design review of the entire concept (much bigger than this change) w/ stakeholder involvement, defined requirements, schedule, and committed resources.

@astrogeco
Copy link
Contributor

astrogeco commented Apr 8, 2020

CCB 20200408 - Push to later date, maybe Fall 2020 or later? Requires a longer design discussion involving more stakeholders. Split into different, smaller pieces.

@skliper
Copy link
Contributor

skliper commented Apr 8, 2020

Just to document - I'm ok with switching cFE to use the appropriate wrappers/macros (where possible and it makes sense) that already exist in the code. I'd prefer CFE_SB_MSGID_LITERAL or other approaches get discussed at a design level first (and consider phasing vs just going directly to a more permanent solution.)

@astrogeco astrogeco added CCB - 20200408 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 8, 2020
Copy link
Contributor

@astrogeco astrogeco left a comment

Choose a reason for hiding this comment

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

Marking based on CCB 20200408

@skliper
Copy link
Contributor

skliper commented Apr 14, 2020

How do we want to handle this one? Close for now and resubmit when the design has been reviewed and approved by our stakeholders? My sense is the design will evolve as part of the review, so keeping this pull request open doesn't seem all that beneficial.

@jphickey
Copy link
Contributor Author

The pull request can be closed, but the issue ticket should remain open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type safety and improved handling of CFE_SB_MsgId_t values
3 participants