CFE_MSG_InitMsg does not always set secondary header bit

[johnphamngc]

Describe the bug

The behavior of CFE_MSG_InitMsg seems to be a regression from the old 6.7 CFE_SB_InitMsg, which always set the secondary header flag regardless of the Clear/InitMsg parameter. There's always a CCSDS secondary header, so this bit should always be set.

To Reproduce
Call CFE_MSG_InitMsg with the Clear parameter set to false

Expected behavior
Secondary header flag on message is set to 1

Code snips

cFE/modules/msg/src/cfe_msg_init.c

Lines 43 to 47 in 3077251

	if (Clear)
	{
	memset(MsgPtr, 0, Size);
	CFE_MSG_InitDefaultHdr(MsgPtr);
	}

CFE_MSG_InitDefaultHdr should always be called.

System observed on:

• SP0
• OS: VxWorks
• Versions: cFE 6.8

Additional context
Discovered when SBNg did not properly set the command code due to missing secondary header flag. This was worked around by calling CFE_SB_InitMsg w/ the InitMsg parameter set to true.

Reporter Info
John N Pham, Northrop Grumman

[skliper]

What was your use case for initializing a message with clear=false? Was it just to avoid the overhead of clearing the payload section or was there something in the header you wanted to retain? The previous implementation for clear=false was somewhat inconsistent in the interpretation of what "clear" meant, in that the length and message ID bits were set, the sequence counter was retained, and everything else in the "header" was set to zero. This mixed behavior was difficult to abstract into an API that's implementation independent (and still does what apps expect).

In an effort to simplify the abstraction, the current implementation just sets the information that is passed in - length and msgid bits (which you can do with CFE_MSG_SetSize and CFE_MSG_SetMsgId). The expected use was the message was initialized at least once with clear=true (sets the entire message to known values), and the only the passed in values needed to change. Would it make more sense for clear=false to set the entire header (zero header, set defaults, then set length and msgid), and just not touch the payload? Alternatively if some things need to be retained and others not, it may be easier to just use the individual APIs.

EDIT - Or should clear=false not zero anything, and just set defaults/length/msgid? If not previously cleared this could lead to random values...

[johnphamngc]

I did see this on the gateway version of SBN, and the bugfix was to simply set clear = true, I assume the original thought was just avoiding the overhead of clearing the payload section that would be filled in and updated elsewhere anyway, and was concerned the behavior change might break things elsewhere.

It's not clear from the comments that the intent was to initialize at least once w/ Clear=true, and that initializing a new packet w/ 'Clear=false` results in an invalid packet w/o the secondary header bit set. Intuitively to me it'd make sense to set the header and not touch the payload and use the individual APIs to set things if necessary, but I guess if it makes the abstraction messy could just leave things as is and update the header comments.

[skliper]

Oh, I see the old CFE_SB_InitMsg still has the old comments... that API will be removed as part of #777. I'll update CFE_MSG_Init (the new/preferred API) comments and implementation to clear/set the entire header and not touch the payload.

[skliper]

Digging into implementation, is it really worth supporting the clear flag going forward? To just clear the header there's additional logic to determine the message type, adds paths, etc. I haven't done a performance check to compare the two, but I wouldn't expect a significant benefit compared to just clearing the entire message. Seems sensible to just clear the entire message all the time (just make it what CFE_MSG_Init does).

[johnphamngc]

Yeah, the overhead of memset is probably negligible. If CFE_SB_InitMsg will be deprecated, I don't think much will be lost if support for the clear flag is removed, since all the code has to be updated anyway.

[astrogeco]

CCB 2020-10-28 APPROVED

• Use in the real world is inconsistent and possibly unnecessary
• Behavior is unclear
• PROPOSAL: outright remove the clear flag