Version 2 MsgId construction doesn't match description, overloads bits

[skliper]

Describe the bug Version 2 code takes the full APID (0x7FF mask), or's in a bit for cmd/tlm (0x80 mask) then or's in the Subsystem ID shifted by 8

That means if a user defines an APID of 0x80 for a telemetry message (which is valid per CCSDS), the system will report it as type cmd if it gets the type from the msgid. It's also a collision between 0x7 bits from the Subsystem ID and the 0x700 bits of APID.

Basically logic doesn't mirror: CFE_SB_SetMsgId of 0x7FF -> APID = 0x7F, Type = Cmd, SubsystemID = 7 CFE_SB_GetMsgId from APID=0x7FF, Type = Tlm, SubsytemID = 0 -> MsgId = 0x7FF

To Reproduce N/A - code inspection

Expected behavior Get/Set should mirror (SetMsgId should not overload bits)

Code snips https://github.com/nasa/cFE/blob/d217ca39f1de74695f0d4e0eb427f90fbe8b1156/fsw/cfe-core/src/sb/cfe_sb_msg_id_util.c#L118-L152 https://github.com/nasa/cFE/blob/d217ca39f1de74695f0d4e0eb427f90fbe8b1156/fsw/cfe-core/src/sb/cfe_sb_msg_id_util.c#L155-L187 https://github.com/nasa/cFE/blob/d217ca39f1de74695f0d4e0eb427f90fbe8b1156/fsw/cfe-core/src/sb/cfe_sb_msg_id_util.h#L64-L75

System observed on: N/A

Additional context Uncovered as part of #711 work

Reporter Info Jacob Hageman - NASA/GSFC

[jphickey]

I noticed the same thing earlier.... recommendation is to keep the MsgId bits the same between v1 and v2. No reason to move them, really.

It makes for weird packing but one can take the full 11 bits of APID into positions [0-10], then pack the CMD/TLM bit into bit position 12. Then 4 bits of subsystem ID go into bits 11, [13-15]. This keeps the CMD/TLM bit in the same place as v1. Example in PR #716.

[jphickey]

If we move the CMD/TLM bit, then it risks breaking apps that treat it as an integer and look at bits directly. It also makes all of the hardcoded example MID values all apps (CI_LAB, TO_LAB, SAMPLE_APP) incompatible with v2 deployment.

[skliper]

I'm just going to fix the overload as part of #726. The PR will also make it easier for missions to reassign the MsgId bits however they want, so there is no need to change the framework to guess at what mission requirements are.

[jphickey]

Well I was under the impression that one of goals of this whole effort was to remove the need for something like MESSAGE_FORMAT_IS_CCSDS_VER_2 mission config option. This means either CFE_SB_MsgId_t has to be consistently defined (header type agnostic) or that the definition is actually part of the modular library. I don't think the latter is appropriate as it is supposed to be an SB abstraction. But it could work either way. The main issue I had was with moving bits around based on MESSAGE_FORMAT_IS_CCSDS_VER_2 isn't really scalable to more options.

[skliper]

Right, I'd rather deprecate the V2 msgid from within the framework and just support the original definition by default. Missions can redefine as needed (and update all the hardcoded values and groundsystem to match). I'm not convinced we should remove the CCSDS extended header inclusion option (but update to use source selection via CMake eventually), since it's standards based and likely useful to support out of the box.

[jphickey]

It just has to be clearly defined as to what entity "owns" the CFE_SB_MsgId_t type definition. The choices are CFE_SB itself, or the MSG library which is implementing the headers (potentially user-supplied).

My recommendation is to keep it owned by CFE_SB itself because many of its API calls use it and the whole point of its existence is to provide an abstract, implementation-independent definition of a software bus endpoint. If the definition has to change based on the header implementation then it's not really an abstraction and there is not much point to its existence.

[skliper]

I moved it to MSG since it depends on specific bits in the header. Basically anything directly accessing bits conceptually is MSG. I'd think of it as the layer that DOES change based on header implementation such that SB doesn't have to.

[jphickey]

OK - but that means that every function that queries/manipulates a CFE_SB_MsgId_t value also needs to move to the MSG library with it....