Remove local-endian SID macros, and unnecessary abstraction of mask/shift

[skliper]

Is your feature request related to a problem? Please describe. The following macros aren't clearly documented as to use. They only work on a local endian StreamID (like what comes from CCSDS_RD_SID).

https://github.com/nasa/cFE/blob/62252d11409f337d3dea2732739e068987363985/fsw/cfe-core/src/inc/ccsds.h#L437-L454

The CCSDS_RD_BITS/WR_BITS isn't CCSDS related, and is just a mask/shift. More straight forward to just use mask/shift. See conversation on https://github.com/skliper/cFE/commit/a8c24ea4370d99b097f91a2c3a8dd76d9202dc74#commitcomment-38382611

Describe the solution you'd like Remove these since they just add to confusion. Just use the CCSDS_RD_SID/APID/SHDR/TYPE/VERS macros directly on the header.

Describe alternatives you've considered Could deprecate, but no known uses.

Additional context Conversation stemmed from #568

Requester Info Jacob Hageman - NASA/GSFC

EDIT fixed code blob EDIT fixed my initial issue title and updated description per @jphickey clarification of intended use

[skliper]

@yashi ping

[jphickey]

Those macros aren't accessing bits in a header, they are accessing bits in a MsgID (this is from when there was a lot of conflation between the MsgId and StreamId values).

It is supposed to be used on the output of CCSDS_RD_SID and this is the one that actually reads from a CCSDS header.

I do concur they should be removed just because they are confusing but they aren't really broken if used as intended.

[skliper]

Those macros aren't accessing bits in a header, they are accessing bits in a MsgID (this is from when there was a lot of conflation between the MsgId and StreamId values).

But the comment specifically says for extracting fields from a stream ID, where it sounds like it's not the CCSDS defined stream id (with a defined bit order), but a local-endian stream ID.

ApId_local_endian = CCSDS_SID_APID(CCSDS_RD_SID(CCSDS_PriHdr)) was the intended use?

[skliper]

Worth a note it also confusing relative to the CCSDS_PriHdr_t definition, where StreamId is endian agnostic. Obviously different uses, but overloaded naming.

Broken = CCSDS_SID_APID(CCSDS_PriHdr.StreamId) would be what I'd think this would be used for without knowing better.

[jphickey]

CCSDS does not define a StreamID - it defines an APID.

Stream ID is an old CFE term that used to be (sloppily) interchanged with MsgId. As part of the extended header efforts, some effort was made to actually attach a definition to these terms and use them more consistently.

For CFE, the term StreamId was defined as the first two bytes of a CCSDS primary header, interpreted as a 16-bit word. (notably this includes the 3-bit version identifier). StreamId is accessed through the supplied macro, which corrects for the byte ordering.

Broken = CCSDS_SID_APID(CCSDS_PriHdr.StreamId) would be what I'd think this would be used for without knowing better.

Yeah, this would be an incorrect use of the macro, but I wouldn't call it "broken" -- more like user error. I think that wouldn't even compile. The "correct" use would be to get the SID through the macro:

StreamID = CCSDS_RD_SID(CCSDS_PriHdr);
Apid = CCSDS_SID_APID(StreamID);

[jphickey]

And please note I'm not necessarily opposed to removing this -- so long as it isn't widely used, its probably easier to deprecate than document, as there is a good chance of misuse. And code shouldn't really be assuming certain framing bits anyway... any use of these macros probably means the code doesn't play nice with extended headers.

I'd actually like to see the StreamID term go away ....

[skliper]

Sorry, I should have said ccsds.h defined StreamId. Not Space Packet Protocol CCSDS 133.0-B-1 Blue Book defined.

Says it extracts from a stream id, and the only defined stream id I saw in ccsds.h is CCSDS_PriHdr.StreamId. And where is StreamId as defined by cFE documented as different from CCSDS_PriHdr.StreamId? This is broken in my book, as illustrated by the user confusion and as you say it would break if you did pass in the only StreamID defined in the ccsds.h file.

Either way, I think we are agreeing to remove, so I will. The documentation and intended use is at best extremely misleading.

I'd actually like to see the StreamID term go away ....

How would you prefer to redefine CCSDS_PriHdr then?

[jphickey]

There is a comment here that probably should be a little more visible, like somewhere in the developer documentation:

https://github.com/nasa/cFE/blob/62252d11409f337d3dea2732739e068987363985/fsw/cfe-core/src/sb/cfe_sb_msg_id_util.c#L53-L75

This comment doesn't specifically say that StreamID native endian though. But the emphasis is that it is a historical term/value that we shouldn't really use going forward because it doesn't account for the possibility of extended header fields.

I'd actually like to see the StreamID term go away ....

How would you prefer to redefine CCSDS_PriHdr then?

Might not need to change it in the structure definition - by "go away" I was more thinking in terms of what is exposed in the API or used in apps. Unfortunately the C language doesn't really allow the ccsds.h header file to become private/hidden, because the intended public SB API is defined in terms of these structs, mainly for size (e.g. CFE_SB_Msg_t, CFE_SB_CMD_HDR_SIZE, and so on).

As long as apps play by the rules and do not directly access any fields within the header and only use the SB API to inquire about framing data, its OK.

Actually - one improvement might be to split ccsds.h into two separate files - one which only defines the structs such that cfe_sb.h will get the right values and sizes for its types. (This depends on the size of the CCSDS_SpacePacket_t, CCSDS_CommandPacket_t, CCSDS_TelemetryPacket_t structs).

The rest of the file, including the macros, could be moved into a private file that only SB can use.

What I'm most concerned about is that it's still possible for code to do something like this:

uint16 StreamID = CCSDS_RD_SID(*MsgPtr);

And then proceed to use that StreamID value in places where the SB API has a CFE_SB_MsgId_t. In versions of CFE before 6.6 this was fairly common to intermingle these values, as the code didn't really distinguish between StreamID and MsgID and the difference wasn't well defined. I believe there are still references to the term Stream in some of the lab apps, too, when actually referring to a MsgId.

It is possible that there is still some lingering code, probably in older apps, that has latent bugs due to this type of thing.

This was the point of #245 but hiding the macros that allow one to read streamID would help too.

[skliper]

Yeah, bothered me to have CCSDS macros exposed if apps aren't supposed to be using them.

[yashi]

CCSDS_SID_* can and should be removed since

• We don't want to use "Stream ID" anymore.
• It's not even CCSDS related.
• It is confusing even though it has CCSDSSID prefix, which is different from CCSDSRD.
• It's not widely used.

This is what I think after reading a few issues around. And the functionality of the macros should be provided by the accessors for CFE_SB_MsgId_t. So this issue can be tracked or merged to #245, I assume.

But just for sake of discussion, I'll add my comment here.

The main problem about this issue is that all functions / macros are not properly layered. Leaking ccsds.h is one indication of it.

Because we use CCSDS as the physical layer packet format, we have ccsds.h. And that's fine if it's local only to SB. Because, it is just an implementation detail we have in SB. However, both SC and SCH has raw packet defined in their tables and the implementation of CFE_MAKE_BIG16() uses ccsds.h, which is unfortunate reveal of underlying implementation even though what is does is swap 16 bit values.

(I don't have any good idea for those tables with raw packets..)

One way to do is, as describe in #245, to hide raw packet under SB's structure, and make only accessors available to outside of SB. One (old) implementation technique is to have, say, sb-msg-impl-ccsds.c and sb-msg-impl-newformat.c. Make them compile time choice. Keep one and true API with unified sb-msg.h. And hide ccsds.h or ccsds.c local to sb-msg-impl-ccsds.c. This can be achieved by improved include directory management. ie. do not let other app to include app/src/.

Another is to have one more namespace for MsgId. Because we are using C, we don't really have namespace. But we can use names to group functions. As you know we have CFE_SB_ prefix for SB. Make yet another "sub-module" under this SB module namespace, like:

• CFE_SB_Pipe_Create()
• CFE_SB_Msg_Subscribe()
• CFE_SB_Msg_Send()
• CFE_SB_MsgId_GetStreamId() / if you want to keep it /

All functions should take the respected structure (or pointer to it) as the first argument. So that users know which one to use. This will introduce a new set of functions but it's doable since it won't conflict the current code (and easy to provide compatibility macros for old users). I believe that if we have proper APIs for accessing MsgId (and other sub-modules) we don't really need to hide the internal representation. It's differ in implementations anyway. Users will know what to do.

For hiding, one trick is to name the members of struct with the prefix __private_. It'd ring a bell for user.

Another trick is to use pointers to opaque structures. That is, keep the declaration of structure in .c and just use pointers to it in APIs. This way, SB is the only code it knows internal representation. All API must be accessed with a pointer to it.

I first assumed cFE don't really like dynamic allocation but SB is already using CFE_ES_GetPoolBuf(). So, it might be an option. It's gonna be a huge API break though.

my two cents,

[skliper]

All good ideas, and we've had discussions internally that are closely related. Major SB/MsgId rework is tentatively targeted for the next release cycle and we'll be coordinating with at least two major stakeholders (we are near the end of this cycle). We certainly welcome inputs/contributions, and @jwilmot is likely the one to coordinate with.

[jphickey]

Indeed, part of the issue is to keep things backward compatible with existing usage patterns. I like data hiding using a pointer to opaque object but many existing patterns in apps use at least the sizeof() operator on some of this stuff and/or locally instantiate these headers (as part of a bigger stuct typically) so we must expose the CCSDS structure definitions for this reason even though app code should never refer to these fields directly.

I still think it would improve the situation if we split ccsds.h and separated the struct definitions (public) from the access macros (private) and only keep the macros that are actually relevant/useful. The only problem is we can't be sure if an app is using some of these so there is a risk of breakage, so that change would have to go through the deprecation process somehow.

[skliper]

Suggestion - ccsds.h:

  #ifndef OMIT_DEPRECATED
  #include ccsds_private.h
  #endif

Internals should reference ccsds_private.h and in major release remove the include.

[jphickey]

We actually have a cfe-core/src/inc/private subdirectory intended for stuff that needs to be referenced from the public API but shouldn't be used directly by apps. Seems like a good fit to wrap the inclusion of the separate macro file in OMIT_DEPRECATED.

[yashi]

Do you have

• a list of supported compilers?
• a pointer to deprecation process?

[skliper]

list of supported compilers?

Depends on your definition of "support". CI currently uses gcc 7.4.0. We periodically test RTEMS 4.11 and VxWorks 6.9 builds with their cross compilers (see the cfe/cmake/sample_defs/toolchain* files) and eventually plan on adding these to our CI with build verification scripts but that is still on the TODO list. We certainly welcome submittal of issues or pull requests for consideration that extend or improve support, but it's not an active priority for our core members.

pointer to deprecation process?

Just submitted https://github.com/nasa/cFS/issues/67, open for discussion.

[jphickey]

All of the compilers that we actively test with on a regular basis (including VxWorks and RTEMS) are gcc-based. There are some clang users out there and the intent is the code should work but we don't directly support that. There are also some known issues with POSIX systems that are not glibc based (MacOS, BSD, etc). Technically not a compiler issue, but a C library dependency.