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

Header version/implementation selection logic update #796

Closed
skliper opened this issue Aug 7, 2020 · 7 comments · Fixed by #880 or #912
Closed

Header version/implementation selection logic update #796

skliper opened this issue Aug 7, 2020 · 7 comments · Fixed by #880 or #912
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Aug 7, 2020

Is your feature request related to a problem? Please describe.
#726 separates the header implementation logic and paves the way to use source selection for msgid implementation and CCSDS extended header use. It partially uses new cmake flags to implement, but still relies on MESSAGE_FORMAT_IS_CCSDS_VER_2 being set correctly (but does not do it by default).

Describe the solution you'd like
Remove dependencies on MESSAGE_FORMAT_IS_CCSDS_VER_2 (see proposal below)

Describe alternatives you've considered
Consider changing cfe configs to cmake options (like the osconfig update), and use the cmake options to set the define if still required. Or could just set the version 2 define if either cmake option (from #726) is set.

Additional context
#726

Requester Info
Jacob Hageman - NASA/GSFC

@skliper
Copy link
Contributor Author

skliper commented Aug 12, 2020

Proposal:

  • cfe_sb_extern_typedefs.h:CFE_SB_MsgId_Atom_t always uint32 (always big enough to hold either msgid v1 or msgid v2
  • cfe_sb_init.c:Remove CFE_SB_InitMsgMap version reporting (gets reported at prep)
  • cfe_sb_verify.h: Error if VER_2 is defined (since it won't do anything and will change behavior underneath user if they don't use the new method)
  • cfe_sb_msg_id_util.c: clean documentation (now documented in msg implementation)
  • sb_UT.c: Remove explict checksum assert
  • ut_support.c: get actual command code from the structure, not a byte offset
  • cfe_msg_defaults.h: CCSDS Version in header... not really standards conforming anyways to change this number, just default to 0?

@skliper
Copy link
Contributor Author

skliper commented Aug 12, 2020

VER_2 define is also used to set the MID_BASE and MID_GLOB values, which has it's own problems and ticket for discussion at #732.

@CDKnightNASA
Copy link
Contributor

CDKnightNASA commented Aug 13, 2020

Proposal:

  • cfe_sb_extern_typedefs.h:CFE_SB_MsgId_Atom_t always uint32 (always big enough to hold either msgid v1 or msgid v2

why _Atom_? Would there be a CFE_SB_MsgId_t or CFE_SB_MsgId_Molecule_t? Or are operations on it atomic? Kidding aside, I say skip the addl 5 chars. I dare you to count how many times CFE_SB_MsgId_t is currently used in our code.

@jphickey
Copy link
Contributor

There actually is a reason - this typedef is the "internal value" and not the one passed around by apps. The value used in the external API is CFE_SB_MsgId_t and it is defined separately, and it needs a distinct name.

The term "Atom" originates from the circa 3-4 years ago when the GHAPS project was developing the EDS toolchain and it looked like the CFE infrastructure would move potentially toward an EDS-based solution. This tool puts an "_Atom_t" suffix on all the fundamental data types (integers, etc) that are generated from EDS definitions.

All hardcoded data types in the "*_extern_typedefs.h" files were separated from the normal API/msg header file in anticipation of transitioning to a generated header file in some future version.

I'm not necessarily tied to the "Atom" term - as it does imply atomicness but that is not necessarily so - but I do think these basic/fundamental data types used for interoperability should share a common suffix that is different than just _t to help differentiate from other local-only typedefs.

@CDKnightNASA
Copy link
Contributor

There actually is a reason - this typedef is the "internal value" and not the one passed around by apps. The value used in the external API is CFE_SB_MsgId_t and it is defined separately, and it needs a distinct name.

how about CFE_SB_MsgId_Internal_t (or some abbrev.?)

@jphickey
Copy link
Contributor

Not an internal/external thing -- it is a question of if we want to name typedefs of basic fundamental values (e.g. int, float) differently than compound values (struct, union, etc).

The EDS tool (currently) generates typedefs with the following pattern:
basic fundamental types -> _Atom_t
enums -> _Enum_t
strings -> _String_t
array -> _Array_t
everything else (incl structs) ->_t (so the names would end up being compatible with existing code).

While I do like having some indicator in the typedef name to indicate its nature, I'm not bound to it. Could be replaced by another layer of typedef or updating the tool to output typedefs without the extra suffix.

It was a CCB topic of conversation years ago when these *_extern_typedefs.h files were forked off from the others, and at the time the CCB decided to keep the suffixes, but this could always be changed again.

As it stands, it was only types/structs that were anticipated being part of the inter-processor communication (message, tables) that ever had this convention applied anyway - types for internal use were never made to match this pattern.

@CDKnightNASA
Copy link
Contributor

Not an internal/external thing...

Thanks Joe, makes sense.

<joke>Or I suggest we have a base64 encoding of a bitmap that maps into a semantic definition of types.

CFE_aF3#_t -> SB, Internal, Atom, Message, ID...</joke>

@skliper skliper added this to the 7.0.0 milestone Sep 10, 2020
skliper added a commit to skliper/cFE that referenced this issue Sep 10, 2020
- Removes MESSAGE_FORMAT_IS_CCSDS_VER2 and all references
- Now replaced by MISSION_MSGID_V2 and MISSION_INCLUDE_CCSDS_HEADER
  cmake variables
- Base MIDs localized to cpu1_msgids.h and improved documentation
  indicating example nature of implementation, note issue nasa#732
  may make this obsolete
- Updated cfe_sb.dox for message module concept
- MsgId base type now always uint32 (reduces logic differences)
- Removed system log report of version used, in build and obvious
  from packet sizes
- Cleaned extra documentation from cfe_sb_msg_id_util.c
- Removed verification limits on CFE_PLATFORM_SB_MAX_MSG_IDS
- Removed UT_GetActualPktLenField and UT_GetActualCmdCodeField
  that depended on the define, shouldn't directly access message
  in a unit test since it's implementation dependent
- Default CCSDS version default now always 0 (per the standard)
  but mission configurable
skliper added a commit to skliper/cFE that referenced this issue Sep 10, 2020
- Removes MESSAGE_FORMAT_IS_CCSDS_VER2 and all references
- Now replaced by MISSION_MSGID_V2 and MISSION_INCLUDE_CCSDS_HEADER
  cmake variables
- Base MIDs localized to cpu1_msgids.h and improved documentation
  indicating example nature of implementation, note issue nasa#732
  may make this obsolete
- Updated cfe_sb.dox for message module concept
- MsgId base type now always uint32 (reduces logic differences)
- Removed system log report of version used, in build and obvious
  from packet sizes
- Cleaned extra documentation from cfe_sb_msg_id_util.c
- Removed verification limits on CFE_PLATFORM_SB_MAX_MSG_IDS
- Removed UT_GetActualPktLenField and UT_GetActualCmdCodeField
  that depended on the define, shouldn't directly access message
  in a unit test since it's implementation dependent
- Default CCSDS version default now always 0 (per the standard)
  but mission configurable
yammajamma added a commit that referenced this issue Sep 24, 2020
Fix #796, Remove dependency on CCSDS version define
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants