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

Provide configurable/customizable message abstraction layer #554

Closed
skliper opened this issue Mar 12, 2020 · 11 comments · Fixed by #726
Closed

Provide configurable/customizable message abstraction layer #554

skliper opened this issue Mar 12, 2020 · 11 comments · Fixed by #726
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Mar 12, 2020

Is your feature request related to a problem? Please describe.
SB provides abstraction from CCSDS packets, yet the CCSDS_CommandPacket_t is still referenced directly by the services (and likely all the Apps that accept a command) and SB functionality itself is tightly coupled to the actual message format.

Describe the solution you'd like
Provide a "MSG" abstraction layer (like inc/cfe_msg.h and a src/msg) to implement direct access getters/setters for supported "header" fields. Allow for selection of the supported message formats, or customization via mission configuration (and adding of additional getters/setters). SB should be abstracted the same as the other services.

"header" is intentionally vague, since it should include any common fields for which getter/setter abstraction is appropriate related to cFE services and apps.

Describe alternatives you've considered
Leave as is, which requires clone and own approach for customization and extensive SB impacts.

Additional context
Suggest that since this would now support customization, we collapse the open source supported time format options down to 1 (big endian, with the 6 byte default). If missions need something else, they can easily customize.

#711 - Separate message access API's from SB
#597 - local endian SID macros, unused shift/mask macros (in ccsds.h)
#440 - Improve API consistency for functions accepting a software bus message
#416 - Investigate various VerifyCmdLength implementations and possible common utility
#172 - Unsafe macros, investigate conversion into Inline functions
#92 - CFE_SB_GetMsgTime() and CFE_SB_SetMsgTime() do not handle byte-swapping on _EL platforms

Requester Info
Jacob Hageman - NASA/GSFC

@skliper skliper changed the title CCSDS not fully abstracted Provide configurable/customizable message abstraction layer May 18, 2020
@jphickey
Copy link
Contributor

Which services are directly referencing CCSDS_CommandPacket fields? As far as I know everything should already be using the getter/setter APIs provided by SB (e.g. CFE_SB_Get/SetCmdCode, CFE_SB_Get/SetTimestamp, etc). If there are specific places that still are doing direct access to CCSDS header fields these should be fixed. AFAIK there should be an abstract API for any/all of these items already.

@skliper
Copy link
Contributor Author

skliper commented May 18, 2020

Note I'd expect a major refactor of ccsds.h, such that only public elements are exposed, move/replace the rest such that it's behind the abstraction layer.

@skliper
Copy link
Contributor Author

skliper commented May 18, 2020

Which services are directly referencing CCSDS_CommandPacket fields?

SB mainly.

@jphickey
Copy link
Contributor

Yes, SB is expected to be the one task that DOES actually access the internal fields because it is the implementation. Are we just talking about splitting these accessors into a separate module?

It is currently not clear to me what the real problem is here... One module within the CFE framework has to have the actual implementation of the getter/setter, which today is SB. What problem is splitting this into a separate module going to solve?

@jphickey
Copy link
Contributor

Or to put this another way -- what would the proposed "new" abstract getter/setter API look like, if it is not the abstraction currently offered by SB? Do we have examples of the type(s) of header fields that a project may want to add?

@skliper
Copy link
Contributor Author

skliper commented May 18, 2020

Also refactor CCSDS to only relate to the actual defined CCSDS elements in the standard... for example the CCSDS_CmdSecHdr_t is not CCSDS and that should be clear.

Are we just talking about splitting these accessors into a separate module?

That's part of it. Customization of the message format should not impact core services (as long as the minimal getter/setter APIs exist). What this allows is mission customization. I'm thinking source inclusion options to either use different elements of the open source implementation as defined or the mission can include their own custom access functions.

Other than a rename, I'd expect the current getter/setters to remain the same but at least a few need to be "fixed" since they incorrectly access/set bits (for example time is endian dependent)

@skliper
Copy link
Contributor Author

skliper commented May 18, 2020

Restated problem today - missions can not customize their header without cloning and owning and the current implementation mixes scope in ccsds.h/SB, without clear separation of what's CCSDS/custom/should be private/should be public/and what's actually necessary for SB. Improving the design would also enable independent improvements in SB and/or message formatting/packing/unpacking.

@skliper
Copy link
Contributor Author

skliper commented May 18, 2020

Historically we seem to have spent a significant amount of time trying to support all the different options, endianness, time field sizes, etc based on many different mission requirements. This allows for the open source software to support customization without requiring us to carry along the implementation for every one of these options. Reduce the built in support down to what's required for the core. Improved, clear abstraction facilitates a framework which could easily be adapted to support standards based packet definitions, etc. A good thing, right?

@CDKnightNASA
Copy link
Contributor

Obv. this may have impacts on SBN; assuming the API can read/write all fields, SBN could translate SB headers to an SBN-specific format and translate back on the other end of the connection. Currently SBN does byte swapping of secondary headers and sends primary headers without modification.

@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label May 19, 2020
@skliper
Copy link
Contributor Author

skliper commented May 20, 2020

If there's any community / @jwilmot / @acudmore interest, here's just one of the concepts being discussed:

Break up the access functions down for easier source selection

  • cfe_msg_msgid_v1 (version 1 of get/set msgid and get/set pkttype from msgid)
  • cfe_msg_msgid_v2 (version 2 of get/set msgid and get/set pkttype from msgid)
  • cfe_msg_time (reduced to 1 implementation, big endian)
  • cfe_msg_ccsds (get/set all the “primary” ccsds header elements per standard)
  • cfe_msg_ccsds_ext (get/set all the “extended” ccsds header elements per standard)
  • cfe_msg_fc
  • cfe_msg_checksum (or combine fc/checksum two into cmdsechdr)

Add a msg/CMakeLists.txt with source selection options

  • if user doesn’t define anything assume v1 (msgid_v1, time, ccsds, fc, checksum)
  • if user selects v2 (msgid_v2, time, ccsds, ccsds_ext, fc, checksum)

Then define cfe_msg.h with the supported CFE_MSG_TlmHdr_t/CFE_MSG_CmdHdr_t (use cmake to set a define for v2, ifdef to include), or if CFE_MSG_CUSTOM then don’t define it. This file would also control all the API prototypes above.

Use cases

  • User does nothing (Heritage configuration) - defaults to v1
  • User defines V2 - does v2
  • User defines CFE_MSG_CUSTOM (or whatever name makes sense) then:
    • Can either exclude msg from the CFE_CORE_MODULES list (Change so mission can override), and define their own module to include and can include whatever they want from core or implement them all from scratch using their own CMakeLists.txt file and whatever source they want to customize
    • will need to define the tlm/cmd header typedefs (from sample_defs via a mission scope header?)

Specific example - user wants a different time format:

  • mission config CFE_MSG_CUSTOM and define mission custom CFE_MSG_TlmHdr_t and CFE_MSG_CmdHdr_t
  • remove core msg and add mission owned msg_mission to CFE_CORE_MODULES (w/ out of tree path option)
  • within msg_mission, implement custom time but use standard API, use CMakeList.txt to select the rest from the core

@skliper
Copy link
Contributor Author

skliper commented Sep 11, 2020

Implemented by #726

@skliper skliper closed this as completed Sep 11, 2020
@astrogeco astrogeco added this to the 7.0.0 milestone Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants