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

cFE/fsw/cfe-core/src/sb/cfe_sb_msg_id_util.c

Lines 118 to 152 in d217ca3

	/*
	* Function: CFE_SB_GetMsgId - See API and header file for details
	*/
	CFE_SB_MsgId_t CFE_SB_GetMsgId(const CFE_SB_Msg_t *MsgPtr)
	{
	CFE_SB_MsgId_Atom_t MsgIdVal = 0;
	#ifndef MESSAGE_FORMAT_IS_CCSDS_VER_2
	MsgIdVal = CCSDS_RD_SID(MsgPtr->Hdr);
	#else
	uint32 SubSystemId;
	MsgIdVal = CCSDS_RD_APID(MsgPtr->Hdr); /* Primary header APID */
	if ( CCSDS_RD_TYPE(MsgPtr->Hdr) == CCSDS_CMD)
	MsgIdVal = MsgIdVal | CFE_SB_CMD_MESSAGE_TYPE;
	/* Add in the SubSystem ID as needed */
	SubSystemId = CCSDS_RD_SUBSYSTEM_ID(MsgPtr->SpacePacket.ApidQ);
	MsgIdVal = (MsgIdVal | (SubSystemId << 8));
	/* Example code to add in the System ID as needed. */
	/* The default is to init this field to the Spacecraft ID but ignore for routing. */
	/* To fully implement this field would require significant SB optimization to avoid */
	/* prohibitively large routing and index tables. */
	/* uint16 SystemId; */
	/* SystemId = CCSDS_RD_SYSTEM_ID(HdrPtr->ApidQ); */
	/* MsgIdVal = (MsgIdVal | (SystemId << 16)); */
	#endif
	return CFE_SB_ValueToMsgId(MsgIdVal);
	}/* end CFE_SB_GetMsgId */

cFE/fsw/cfe-core/src/sb/cfe_sb_msg_id_util.c

Lines 155 to 187 in d217ca3

	/*
	* Function: CFE_SB_SetMsgId - See API and header file for details
	*/
	void CFE_SB_SetMsgId(CFE_SB_MsgPtr_t MsgPtr,
	CFE_SB_MsgId_t MsgId)
	{
	CFE_SB_MsgId_Atom_t MsgIdVal = CFE_SB_MsgIdToValue(MsgId);
	#ifndef MESSAGE_FORMAT_IS_CCSDS_VER_2
	CCSDS_WR_SID(MsgPtr->Hdr, MsgIdVal);
	#else
	CCSDS_WR_VERS(MsgPtr->SpacePacket.Hdr, 1);
	/* Set the stream ID APID in the primary header. */
	CCSDS_WR_APID(MsgPtr->SpacePacket.Hdr, CFE_SB_RD_APID_FROM_MSGID(MsgIdVal) );
	CCSDS_WR_TYPE(MsgPtr->SpacePacket.Hdr, CFE_SB_RD_TYPE_FROM_MSGID(MsgIdVal) );
	CCSDS_CLR_SEC_APIDQ(MsgPtr->SpacePacket.ApidQ);
	CCSDS_WR_EDS_VER(MsgPtr->SpacePacket.ApidQ, 1);
	CCSDS_WR_ENDIAN(MsgPtr->SpacePacket.ApidQ, CFE_PLATFORM_ENDIAN);
	CCSDS_WR_PLAYBACK(MsgPtr->SpacePacket.ApidQ, false);
	CCSDS_WR_SUBSYSTEM_ID(MsgPtr->SpacePacket.ApidQ, CFE_SB_RD_SUBSYS_ID_FROM_MSGID(MsgIdVal));
	CCSDS_WR_SYSTEM_ID(MsgPtr->SpacePacket.ApidQ, CFE_MISSION_SPACECRAFT_ID);
	#endif
	}/* end CFE_SB_SetMsgId */

cFE/fsw/cfe-core/src/sb/cfe_sb_msg_id_util.h

Lines 64 to 75 in d217ca3

	/*
	* Mission defined bit used to indicate message is a command type. A 0 in this bit position indicates
	* a telemetry message type. This bit is included in the message id.
	*/
	#define CFE_SB_CMD_MESSAGE_TYPE 0x00000080 /* 1 bit (position 7) for Cmd/Tlm */
	/*
	* Mission defined macros to extract message id fields from the primary and secondary headers
	*/
	#define CFE_SB_RD_APID_FROM_MSGID(MsgId) (MsgId & 0x0000007F) /* 0-6(7) bits for Pri Hdr APID */
	#define CFE_SB_RD_SUBSYS_ID_FROM_MSGID(MsgId) ( (MsgId & 0x0000FF00) >> 8) /* bits 8-15(8) bits for APID Subsystem ID */
	#define CFE_SB_RD_TYPE_FROM_MSGID(MsgId) ( (MsgId & CFE_SB_CMD_MESSAGE_TYPE) >> 7) /* 1 Cmd/Tlm Bit (bit #7) */

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....