Deprecate SB elements relative to msg module adaption

[skliper]

Is your feature request related to a problem? Please describe.
Many APIs will be updated for consistency relative to the MSG module, also element scoping improvements (SB doesn't actually care about the header, it just needs to route).

Describe the solution you'd like
Per 2020-07-28 discussion SB for once #726 is in.

Deprecating:

• CFE_SB_PKTTYPE_* -> CFE_MSG_Type_t
• CFE_SB_MsgPtr_t -> CFE_MSG_Message_t *
• CFE_SB_Msg_t -> CFE_MSG_Message_t
• CFE_SB_MsgPayloadPtr_t (use pointer to payload in structure)
• CFE_SB_InitMsg -> CFE_MSG_Init
• CFE_SB_GetTotalMsgLength -> CFE_MSG_GetSize
• CFE_SB_SetTotalMsgLength -> CFE_MSG_SetSize
• CFE_SB_GetMsgTime -> CFE_MSG_GetMsgTime (this gets rid of structure return, similar to CFE_TIME_GetTime() should not return a structure #45 issue)
• CFE_SB_SetMsgTime -> CFE_MSG_SetMsgTime
• CFE_SB_GetCmdCode -> CFE_MSG_GetFcnCode
• CFE_SB_SetCmdCode -> CFE_MSG_SetFcnCode
• CFE_SB_GetChecksum (no use case defined, what do you need it for?)
• CFE_SB_GenerateChecksum -> CFE_MSG_GenerateChecksum
• CFE_SB_ValidateChecksum -> CFE_MSG_ValidateChecksum
• CFE_SB_GetMsgId -> CFE_MSG_GetMsgId
• CFE_SB_SetMsgId -> CFE_MSG_SetMsgId
• CFE_SB_GetPktType -> CFE_MSG_GetTypeFromMsgId
• CFE_SB_SetMsgSeqCnt -> CFE_MSG_SetSequenceCount

NOT deprecating, but will note in API that these are fragile (guesses based on assumptions). Future implementation could be replaced via data dictionary sort of access or standards based header definitions (including secondary header, and flags or extra internal data to mange the real sizes):

• CFE_SB_MsgHdrSize: use actual message structure where possible
• CFE_SB_GetUserData: use actual message structure where possible
• CFE_SB_GetUserDataLength: use actual message structure where possible
• CFE_SB_SetUserDataLength - use CFE_MSG_SetSize with full message structure where possible
• CFE_SB_CMD_HDR_SIZE -> sizeof CFE_MSG_CommandHeader_t) preferrred
• CFE_SB_TLM_HDR_SIZE -> sizeof (CFE_MSG_TelemetryHeader_t) preferred

Note CFE_SB_Qos_t and CFE_SB_Default_Qos will likely be used for #926 (critical subscription)

NOT deprecating, MSG types are unaligned, SB types are now aligned:

• CFE_SB_Msg_t, CmdHdr_t, TlmHdr_t -> CFE_MSG_*

NOT deprecating CFE_SB_TimeStampMsg. See #26 that requests making this part of send.

Describe alternatives you've considered
Just need to manage these, unique deprecation flag... not all actually need to go, but reduced/simplifies unit testing

Additional context
#711, #726

Requester Info
Jacob Hageman - NASA/GSFC

[skliper]

Marked as ready to discuss, so I know where to focus unit testing (plan to not unit test deprecated elements).

[astrogeco]

CCB-2020-07-15: DISCUSSED, will schedule a splinter

[skliper]

Sounds like GetUserData could still be useful... but could be complicated... MSG doesn't actually know where what you call "user data" may start. Is it immediately after the "header", which could be primary, extended, secondary. Is there padding? MSG itself does not have the full message structure/description. Might have a better home in whatever packet layout scheme/library ends up being used (if not using structures).

[skliper]

Another comment captured from CCB discussion - definitely don't want to preclude different decryption ideas... either under the hood (decrypted internal to SB) or decrypted via a library call from the app (w/ an app-owned key) after message receipt. I don't think any deprecation mentioned above would do this, but definitely need to keep in mind.

[jphickey]

I was hesitant to bring it up in the CCB but this is definitely an area where network encoding and translations need to be considered.

• Portable CFS applications should always define their RUNTIME messages as C structures. This is what is done today.
• Those C structures must be padded and aligned appropriately for the compiler/target being used. This will be the case as long as there aren't any pragma/overrides or other tricks to defeat this alignment (don't do that).
• This way it is safe to cast the structure pointer received from the software bus to the "real" type.

So yeah, for CFS apps, they should all access to message payload and fields through the C structure. They should use sizeof() and offsetof() operators to get the size/offset as needed. I see nothing wrong with that. This paradigm also works fine if the C structures are generated by a tool - the code knows no difference.

I think encryption/decryption is orthogonal to this -- totally separate issue and it can coexist with it either way. Encryption can be done inside software bus (upon send/receive, transparent to apps) or as it goes to/from the network (where it's needed) or localized to an app, where the app defines its "message" as a binary blob, and decrypts it locally to the real data (transparent to SB). It all is possible, and is unaffected by these accessor APIs as far as I can tell.

[CDKnightNASA]

would like to be part of splinter

[jphickey]

I would also like to be part of splinter, but my only availability is today and tomorrow (7/16). Otherwise I am not available until Monday 7/27.

[iamthad]

Please invite me to the splinter as well.

[jphickey]

To extend my previous comment - in case I cannot participate in splinter - I do see the usefulness of these abstract functions when sending/receiving on a network - where you may not have the C structure available - but need to add/strip padding that was not part of the real data.

They aren't currently defined that way because CFE doesn't currently make a distinction (network data is memory data) but we know this doesn't work well when making heterogeneous systems talk to each other. With data buffers received from a network socket, it is generally not safe to directly cast as a C struct because you don't know the data is aligned properly for the current CPU, among other things.

So I do think its useful if these abstract functions like CFE_SB_MsgHdrSize() and all were defined to assist with dealing with the padding problem - adding or removing the requisite processor-dependent alignment bytes when transferring between network sockets. So in other words:

• MsgHdrSize() function returns the actual/real size of the header without padding
• sizeof(struct MsgHdr) reflects the size of the header in memory, including padding.

[skliper]

It's trivial to leave in the CFE_SB_MsgHdrSize, CFE_SB_GetUserData, CFE_SB_GetUserDataLength, CFE_SB_SetUserDataLength w/ minor updates... but I'd still recommend using truth, be it the message structure (for memory) or information from whatever mechanism used to define the layout of data (be on the network or wherever). Anything implemented now via SB is really just a guess... since it isn't using the actual message structure or layout definition. It could guess based on the secondary header flag, the version information in the header, etc but all of that is open to redefinition and interpretation of what "User Data" and "Header" means. If there's heterogeneous header definitions, then SB doesn't really know for sure where anything is, nor does it care. It just routes messages.

As implemented now MsgHdrSize can easily be wrong in various scenarios (no secondary header, but with VER_2 defined which includes extended header, or if the original message used anything other than the local definition of secondary header), which means all the other functions are just as fragile. Or arguably worse, since GetUserData just returns the memory location of the message + MsgHdrSize with no regard for possible compiler padding... and so on. I guess what I'm thinking is it's not worth trying to fix these because I think using truth is better, wherever that truth may come from.

Another way to say it is if for the use case you don't know for sure where the user data is or what the header size is (the structure isn't available and header size unknown), then right now neither does SB. At least not for sure in an abstract sense. Much more likely would need to rely on what headers are being used across the entire mission, the actual layout/definition, etc.

[jphickey]

Right - I think we are in general agreement. A CFS app has the C struct definitions of the messages it is sending/receiving, so the "truth" in a CFS app would be sizeof(), offsetof(), etc and any payload field access should be made directly via that C structure.

An entity that is sending/receiving data via a network connection and has to deal with padding should have some sort of API provided by the encoding scheme to identify the size and position of actual data elements, minus padding. Or better yet, copy the important data according to its own internal header layout to/from a network packet buffer.

So I guess I'm in favor of deprecating the CFE_SB_MsgHdrSize and CFE_SB_GetUserData and related functions. These aren't exactly ideal in their current form to support padding identification so they'd have to be redefined for that purpose anyway. Recommend to use the C structure definition and associated operators/primitives for all in-memory runtime manipulation of these messages.

[jphickey]

However - I'm still on the fence about "accessor" methods like CFE_SB_Get/SetMsgId, CFE_SB_Get/SetTotalMsgLength, etc. While I concur these really have nothing to do with SB, I'm not seeing what is really gained by forcing all apps to change to use the CFE_MSG_ equivalent instead. This will be a headache for users to update their code.

On the other hand, it is fairly simple to keep these as wrappers in CFE_SB, which not only preserves API compatibility, but also provides another point of flexibility (i.e. you could have more than one framing format and select at runtime). In other words I don't see it as a bad thing to call a common layer first, which then "dispatches" to the actual CFE_MSG implementation. And CFE_SB seems as good a place as any for such a dispatcher.

[CDKnightNASA]

Right - I think we are in general agreement. A CFS app has the C struct definitions of the messages it is sending/receiving, so the "truth" in a CFS app would be sizeof(), offsetof(), etc and any payload field access should be made directly via that C structure.

One model is that what goes into SendMsg and what comes out of RcvMsg() is a generic/general-purpose/protocol-neutral struct that can be interacted with directly without any macros or functions for accessing/setting the fields. Encryption, checksum computation/validation, padding, compression, etc., could all be handled inside of Send/Rcv and we can just say that what is in the struct in memory may have nothing in common with what goes on the bus. (But it's likely that it's exactly the same!) It would be nice, though, for users of the SB API to not have to have any concept of CCSDS headers, just MsgBuf->Timestamp is a platform-specific timestamp, MsgBuf->Id is a generic ID type (uint32, uint64, binary blob, who knows?)

For example (I hope I didn't f*ck up your list, Jake, in my browser it lets me reorder the list in the original issue text. Yikes.):

typedef enum CFE_SB_MsgType
{
    cmd, tlm, evt
} CFE_SB_MsgType_t;

typedef struct CFE_SB_Msg
{
    CFE_SB_MsgType_t Type;
    CFE_SB_CommandCode_t CC; /* could be uint16, byte array, duno */
    CFE_SB_MsgID_t ID; /* could be uint32, could be a byte array, duno */
    OS_Time_t Timestamp;
    size_t UserDataSz;
    void *UserData; /* or could be uint8 UserData[maxsz]; */
} CFE_SB_Msg_t;

• CFE_SB_PKTTYPE_* -> Msg->Type (command/telemetry/event enum)
• CFE_SB_CMD_HDR_SIZE -> remove
• CFE_SB_TLM_HDR_SIZE -> remove
• CFE_SB_MsgPtr_t -> MsgPtr_t is a generic, non-protocol-specific struct
• CFE_SB_MsgPayloadPtr_t -> a field of the Msg struct
• CFE_SB_Qos_t -> remove
• CFE_SB_Default_Qos -> remove
• CFE_SB_InitMsg -> remove [ SendMsg() takes the Msg struct and maps it to an SB-specific struct ]
• CFE_SB_MsgHdrSize -> remove
• CFE_SB_GetUserData -> Msg->UserData
• CFE_SB_Get/SetUserDataLength -> Msg->UserDataSz
• CFE_SB_Get/SetTotalMsgLength -> remove
• CFE_SB_Get/SetMsgTime -> Msg->Timestamp
• CFE_SB_TimeStampMsg -> OS_GetLocalTime(Msg->Timestamp)
• CFE_SB_Get/SetCmdCode -> Msg->CC
• CFE_SB_Get/Generate/ValidateChecksum -> performed by SendMsg/RcvMsg, not part of Msg
• CFE_SB_Get/SetMsgId -> Msg->ID
• CFE_SB_GetPktType -> Msg->Type
• CFE_SB_SetMsgSeqCnt -> generated by SendMsg(), returned by RcvMsg(), not part of Msg

A second model is that everything goes through getter/setter functions that map the generic datatypes to the protocol-specific internal types, and does such things as validation, byte swapping, packing, whatever is necessary.

Anyways, food for thought.

[skliper]

Reviewed list on 2020-07-28 and updated list.

[skliper]

@ejtimmon @jwilmot @acudmore you OK with this list?