CCSDSv2 packing issues

[johnphamngc]

CCSDSv2 msgId's are actually 32 bits and not 16 bits (even though only the lower 16-bits are used). On big-endian platforms, the MIDs will be incorrectly right-padded with zeros. There may be additional CCSDSv2 issues and I'll update this ticket as I run into them.

Patch for this issue below:

diff --git a/fsw/src/sbn_pack.h b/fsw/src/sbn_pack.h
index 6805707..6258c3a 100644
--- a/fsw/src/sbn_pack.h
+++ b/fsw/src/sbn_pack.h
@@ -72,7 +72,14 @@ static inline uint32 Pack_Time(Pack_t *PackPtr, OS_time_t Data)
 static inline uint32 Pack_MsgID(Pack_t *PackPtr, CFE_SB_MsgId_t Data)
 {
     CFE_SB_MsgId_t D;
-    D = CFE_MAKE_BIG16(Data);
+    if (sizeof(D) == sizeof(uint32))
+    {
+        D = CFE_MAKE_BIG32(Data);
+    }
+    else
+    {
+        D = CFE_MAKE_BIG16(Data);
+    }
     return Pack_Data(PackPtr, &D, sizeof(D));
 }

@@ -140,7 +147,14 @@ static inline uint32 Unpack_MsgID(Unpack_t *Unpack, CFE_SB_MsgId_t *DataBuf)
     {
         return 0;
     }
-    *DataBuf = CFE_MAKE_BIG16(D);
+    if (sizeof(D) == sizeof(uint32))
+    {
+        *DataBuf = CFE_MAKE_BIG32(D);
+    }
+    else
+    {
+        *DataBuf = CFE_MAKE_BIG16(D);
+    }
     return 1;
 }

[CDKnightNASA]

I think it's probably most sensible to just always have the MID as a 32-bit, whether using CCSDS v1 or v2, this may also allow a v1 and a v2 system to inter-connect (which may lead to its own special set of issues. :o )

[skliper]

Note MsgId's vs CCSDS headers vs MID values are likely up for a major rework in the somewhat near future. The current V1/V2 implementation and abstraction isn't currently ideal.

[jphickey]

The MsgId size is currently a mission scope typedef, meaning all CPUs within the same CFE mission are required to be the same size. Normally these values aren't passed between systems, as they have system scope (only exception being the ground system, which is external but assumed to be "aware' of the FSW workings).

It is important that apps do not assume a particular structure of this value though. Even 32 bits is technically insufficient as the v2 header has (iirc) something like 39 bits of addressing domain, so we should reserve the right to expand it further or even make it a compound value (maybe).

My recommendation is to simply punt the issue and do nothing right now, don't attempt to swap this value at all, just copy it as-is. AFAICT SBN can't fully support mixed endian deployments due to the payload issue, so it seems implicit that anyone deploying SBN must have all the CPUs in the system be the same endianness if they actually want to use it as intended. Any sort of partial fix adds complication and other possible issues and its not clear what it accomplishes.

I think what @johnphamngc proposed is OK to bridge the gap if you really want to do something, but I think in realistically at this stage all CPUs using SBN must be of a compatible architecture.

[skliper]

mission scope typedef, meaning all CPUs within the same CFE mission...

We may need a new word for this. Many may assume mission is being used in the classical sense (for example a NASA mission), which includes all the systems. This is the scope at which I think @jwilmot has requested for MsgIds, not just CPUs within a single CFE build.

[jwilmot]

A mission is where the message IDs (topics) are all in the same name space allowing them to be unambiguously exchanged between systems and subsystems. The uint32 size allows 2^32 unique topics. I expect cFS will have been long replaced before that is exhausted. Once the current internal limit of 2^16 is exhausted SB will need new algorithms for performance reasons. At this, Artemis and Gateway systems together should not exceed the 64k topic limit.

[jwilmot]

SBN's name space remapping feature should only be needed when you did not design for systems being interconnected.

[skliper]

Or if you need to convert between internal and external for performance reasons? Would you force ever LE system to convert every local message, or just those leaving the system?