Fix #689, Align all software bus message definitions

[jphickey]

Describe the contribution
Replace uint8[] arrays which reserve space for the header with an instance of the header struct as defined by SB.

Note this structure was the basis for the array size, so it is the same size, but by actually using the structure the resulting message will have the correct alignment.

Fixes #689

Testing performed
Execute all unit tests and confirm passing
Sanity check on CFE, builds and runs and processes commands
Also Build and run on 32-bit platform with strict alignment and confirm no warnings/issues

Expected behavior changes
No impact to behavior

System(s) tested on
Ubuntu 20.04 LTS (native, little endian 64 bit)
MIPS 32-bit via QEMU (big endian, strict alignment requirements)

Additional context
Just applies the same fix from #678 to all messages.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

[jphickey]

Initially created as draft to due to baseline issue. Only real commit is d1da5f2. Will rebase after next merge to master.

[skliper]

CCB 20200513 - Approved conceptually on telemetry side, needs rebase

Recommended splitting up since cmd impacts should go into 7.0

[astrogeco]

@jphickey can you split up this PR and separate the CMD and TLM changes?

[skliper]

Rebase and go forward with this?

[jphickey]

The issue with this one is that it does potentially change padding, so we need a consensus on how to deal with that. Ideas have been tossed around but I don't know of an accepted path forward. The real solution is a data dictionary/message definition DB of some type, but my understanding is that 7.0 still won't have that, so we should have some sort of steering committee discussion on how to handle changes int he binary formats for this release.

[skliper]

So I've been tripping up on this lately. Can we be explicit about the impacts? Since the packets are already 32 bit aligned when sent via CFE_SB_SendMsg (after the union with CFE_SB_Msg_t) and the header sizes are already multiples of 32 bits (8 or 12 bytes without extended headers, 12 or 16 bytes with extended headers), what would change relative to the packet structure after the union? Isn't anything that was going to change already changed with the union?

Any payloads that require more than 32-bit alignment still break CFE_SB_GetUserData either way, since it won't point to the start of the payload due to padding to 64 bits after the header.

[skliper]

Or was the issue with the variable legacy TIME formats, if set to a size of 8 this would have made the CCSDS_TelemetryPacket_t size 14? But the union with CFE_SB_Msg_t to create CFE_SB_TlmHdr_t would have rounded that to 16 typically anyways...

[jphickey]

Commands are (currently) not defined as aligned structures. In fact they are explicitly UN-aligned.

For instance, consider CFE_ES_Restart_t:

cFE/fsw/cfe-core/src/inc/cfe_es_msg.h

Lines 1140 to 1156 in a148b97

	/**
	** \brief Restart cFE Command
	**
	** For command details, see #CFE_ES_RESTART_CC
	**
	**/
	typedef struct
	{
	uint16 RestartType; /**< \brief #CFE_PSP_RST_TYPE_PROCESSOR=Processor Reset
	or #CFE_PSP_RST_TYPE_POWERON=Power-On Reset */
	} CFE_ES_RestartCmd_Payload_t;
	typedef struct
	{
	uint8 CmdHeader[CFE_SB_CMD_HDR_SIZE]; /**< \brief cFE Software Bus Command Message Header */
	CFE_ES_RestartCmd_Payload_t Payload;
	} CFE_ES_Restart_t;

The size of this command is "traditionally" 10 bytes, but if defined properly with 32-bit alignment the sizeof() this struct becomes 12 bytes, so it will fail the size check unless the ground system either includes this extra padding OR the system accounts for/adjusts for the padding difference in either CI or within the software bus code itself.

[jphickey]

This type of issue is why this was held off as draft ... it is likely that a bunch of commands will start failing their size checks due to implicit padding differences after this is merged. If we had an accepted approach to handling that then we can move forward.

[skliper]

But doesn't the union with CFE_SB_Msg_t already make the resulting structure size 12 for your example above (typical compiler behavior is to pad)? My point is we already did the union, so in effect we've already changed the sizes of the commands, and this change will not actually change the sizes again. Confirmed on linux...

[jphickey]

the union with CFE_SB_Msg_t

Which union are you referring to, specifically? sizeof(CFE_ES_Restart_t) is not dependent on sizeof(CFE_SB_Msg_t) -- the header in this struct is defined as uint8 CmdHeader[CFE_SB_CMD_HDR_SIZE]; so it has alignment of 1. I can confirm that on my system right now sizeof(CFE_ES_Restart_t) is indeed 10, not 12.

Yes - CFE_SB_Msg_t has 32-bit alignment so one can cast from CFE_SB_Msg_t* to CFE_ES_Restart_t* without a warning but not vice-versa.

[skliper]

OK, I'm on the same page again... the issue is with commands for which we didn't already make a typedef union w/ CFE_SB_Msg_t. I was looking at telemetry, which we did already align.

[jphickey]

Right - for packets which are locally generated (TLM) we make a union - so the buffer in memory is aligned even if the actual TLM structure definition isn't aligned - so it in turn can be safely passed to CFE_SB_SendMsg() which accepts a CFE_SB_Msg_t*. But as a receiver (CMD) we don't make our own buffer, just cast the buffer we get to the correct type -- which should only be of less stringent alignment than CFE_SB_Msg_t (double/long long being the exception/issue).

I believe we've done all we can in 6.8 to make this safe as it can be without a more comprehensive approach to dealing with CPU-specific padding and alignment issues.

[skliper]

Concur w/ @astrogeco, split out the tlm changes since those could be applied w/o impact.

[ghost]

Why not pad it up front? What I mean is, put the actual CCSDS header offset a couple bytes from the front to ensure architecture-dependent alignment of the entire message? In this way, the CCSDS header may be not truly aligned, but the message data would be. (It's up to the definer of the message to align their own data via definition, as has always been the case.) In this way, the only impact is the SB implementation must handle out-of-alignment accesses for the 3 2-byte CCSDS header fields. That's not a big deal. On some architectures like x86 the hardware will deal with it. We could have a config option for HW or SW alignment for CFE. I do this with CF. If your architecture will generate alignment traps and handle mis-aligned access in software, you don't want the overhead, so instead it does everything byte-by-byte. I can't force alignment due to the fact the CFDP PDUs are defined with strange alignments.

[skliper]

Note we expect to receive cmd/tlm structure definitions (potentially platform/compiler specific) from the stakeholder that are generated from a common data model and produced with alignment/padding per their requirements. The open source community can evaluate use/incorporation of those structures, but the database for CTF automated testing will depend on those definitions.

[skliper]

Latest discussion - stakeholder is going to use the method implemented here (updating both commands and telemetry definitions to use the real headers), for open source to match we should plan to get this merged and update the related ground databases. For the framework it impacts cFS-GroundSystem and the EDS definitions. Recommending community notification.

[skliper]

We should get this updated and move forward. It would simplify some other changes I'm working, and we aren't gaining anything by putting it off.

EDIT - marked ready so at least we talk about it again.

[jphickey]

@skliper can you elaborate on what the issue is that you are facing?

As far as I can tell the earlier concern still exists (why I was holding off on this) in that it WILL change the overall size of any command whose size is not currently a multiple of 4 bytes. For instance the "Restart" command but others too. This change in size will effectively break anything sending these commands.

[skliper]

I'm doing the tlm packets now as part of #777. They don't break and it makes sense with the MSG APIs to do it right. For command packets, we've got to do it at some point so why not now? Will it break, yes. Better to break it now than late in the cycle.

[astrogeco]

CCB 2020-11-12

• We already have telemetry messages
• Depends on how we decide to handle the cFE_Msg header
• Pending Refactor message header alignment and "raw" types to fit a clear pattern #1009

[skliper]

#689 is OBE, no longer enforcing worst case alignment in all messages per #1009. This is mostly a duplicate of #1015.