cFE Tlm Packet 64-bit alignment issue

[dmknutsen]

Describe the bug
Compiler add padding is applied to cFE Telemetry packets if the data region does not start on a 64 bit boundary and the packet contains a 64 bit variable.

To Reproduce
Steps to reproduce the behavior:

1. Nominal Build Process.
2. Enable Tlm.
3. Bit bust a telemetry packet containing a 64 bit variable.

Expected behavior
No compiler added padding should be applied.

System observed on:
Ubuntu 64-bit - 19.10

Reporter Info
Dan Knutsen
NASA/GSFC

[jphickey]

The solution to this is EDS. There is no easy solution as long as the TLM is defined directly as a C structure and the TLM header is 12 bytes (note the header can be smaller or larger too, depending on timestamp resolution).

[skliper]

CCB 20200122 - Touched on possible short term solutions (it's broken now in that TLM is not consistent across systems). Pending Gateway discussion today on headers.

[skliper]

@tngo67 Do you have strong feelings on this one for the short term approach? Basically padding depends on the contents right now (if I remember the issue correctly) since the headers don't end on a 64-bit boundary. Do we need a solution this round or can it wait for a longer term solution?

[CDKnightNASA]

Describe the bug
Compiler add padding is applied to cFE Telemetry packets if the data region does not start on a 64 bit boundary and the packet contains a 64 bit variable.

I assume you're talking TLM payload, not header (which has very specific byte positioning, and hopefully nothing in cFS tries to shortcut that with a 64-bit type or a struct.) For payloads, is it cFS's job to enforce this? 'course, if there is TLM being generated by CFS or one of the "stock GSFC" apps, those components and apps need to be fixed...Curious where you've seen this behavior?

And I don't know how we could enforce/ensure that data is "packed" when it's sent to the SB, as the payload is opaque to CFE_SB.

SBN had to address this with its network protocol with packing/unpacking. 'course, EDS will make it all better...

[dmknutsen]

I believe I first noticed the issue in the TBL Housekeeping TLM packet when I was trying to decom it via the GroundSystem.py tool. The issue should however be present in any TLM packet that contains a 64-bit type. In the case of the TBL housekeeping packet, MemPoolHandle is a uintptr_t - which causes the issue (on a 64 bit system).

We touched on this a few months ago in the CCB and I think we discussed forcing the TLM header to end on a 64-bit boundary by adding padding to CCSDS_TlmSecHdr_t (4 bytes if CCSDS_TIME_SIZE is equal to 6 and 2 bytes if it is equal to 8). If I remember correctly Joe recommended a solution within EDS that would not require redefining packet definitions.

[skliper]

@tngo67 The short term question for certification is we have a requirement somewhere to explicitly define all padding. The question is do we need to make the header explicitly align to 64, and waste space where it's not needed, or just disposition this ticket until we adopt a new toolchain/EDS/or whatever.

[CDKnightNASA]

We touched on this a few months ago in the CCB and I think we discussed forcing the TLM header to end on a 64-bit boundary by adding padding to CCSDS_TlmSecHdr_t (4 bytes if CCSDS_TIME_SIZE is equal to 6 and 2 bytes if it is equal to 8). If I remember correctly Joe recommended a solution within EDS that would not require redefining packet definitions.

Copy that, although personally I'd rather the cFS core and "GSFC apps" all standardize on packed, big-endian cmd and tlm message formats, but it's a preference of mine and I realize there are reasons against such a standard.

Needless to say whatever way the CCB decides, we'll need to scrub all TLM-generating code to ensure that it conforms with the standard. Happy to help, time-permitting. ('course, EDS will address this and make it moot anyways.)

[skliper]

Explicitly adding the pad on a packet level basis where needed (before the user data structure) breaks the abstracted functions. For example extra bits in the CFE_TBL_HousekeepingTlm_t...

typedef struct
{
    uint8                       TlmHeader[CFE_SB_TLM_HDR_SIZE];       /**< \brief cFE Software Bus Telemetry Message Header */
    uint8 EXPLICITPADDINGHERE[X];
    CFE_TBL_HousekeepingTlm_Payload_t  Payload;
} CFE_TBL_HousekeepingTlm_t;

GetUserData would need to be aware of the different padding... So I think it needs to be global.

[skliper]

Or at least the app would need to uniquely handle the padding from the result of GetUserData (and any other user data getter/setters).

[jphickey]

No - padding cannot exist between a header and a payload on some packets and not others, that is effectively the same issue whether it is explicit or implicit. The point is that the receiver needs to decode it, which can only be done if it (reliably/consistently) knows how big the header is.

There is no easy fix for this. The only possible short term workaround would be to increase CFE_SB_TLM_HDR_SIZE from 12 to 16 by including padding for every packet and wasting bytes.

[skliper]

No - padding cannot exist between a header and a payload on some packets and not others, that is effectively the same issue whether it is explicit or implicit.

It does now...

[skliper]

Anyways, I think we are agreeing on short term possible fix? Just a question now for @tngo67 if we should implement this for the certification or not (would change most telemetry packets on the wire, ripples into CTF ground stations and CCDD).

[dmknutsen]

If we decide to move forward...can I volunteer to take this one? I've been auditing all of the unit tests for the the past few days - and it is slowly starting to drive me insane lol. Would be a welcome break/change of pace.