CCSDS secondary header consistency update

[skliper]

Update data structure definition of CCSDS command secondary header to be consistent with the rest of the message header definition.

   from

/----- CCSDS command secondary header. -----/
typedef struct
{
uint16 Command; /* command secondary header /
/ bits shift ------------ description ---------------- /
/ 0x00FF 0 : checksum, calculated by ground system /
/ 0x7F00 8 : command function code /
/ 0x8000 15 : reserved, set to 0 */
} CCSDS_CmdSecHdr_t;

   to

/----- CCSDS command secondary header. -----/
typedef struct
{
uint8 Command[2]; /* command secondary header /
/ bits shift ------------ description ---------------- /
/ 0x00FF 0 : checksum, calculated by ground system /
/ 0x7F00 8 : command function code /
/ 0x8000 15 : reserved, set to 0 */
} CCSDS_CmdSecHdr_t;

Requested by Tam via email.

[skliper]

Imported from trac issue 266. Created by jhageman on 2019-03-26T09:56:39, last modified: 2019-05-23T16:43:54

[BaldBeacon]

Sorry to bother you @skliper but could I get a status on this issue?

The 16 bit secondary command header has recently caused problems when writing packets on little endian systems and was wondering if this was being worked towards on the GSFC internal version of the cFS.

[skliper]

There is no internal version of cFE. The ticket has not been assigned, so as far as I know it isn't currently being worked. Happy to take a contribution if you have time to submit. If so, just comment that you will take the issue and I'll assign it.

Note that in addition to the type change, the comment should be updated to reflect the change (explicitly define element 0 and 1). Likely also requires changes in code.

[BaldBeacon]

Sure, I'll make an effort to approach this soon. I'm a bit swamped hitting deadlines atm.

[BaldBeacon]

Since I'll be a new contributor to the open source CFE was there a specific set of steps you'd like me to adhere to?

[skliper]

Contributor guide and updated pull request template are in the works. For now, just match original code format, and your changes should pass CI and unit testing as defined in https://github.com/nasa/cFS/blob/dev-vdd/VDD.md. You'll be contacted for any additional contribution paperwork, if needed.

[skliper]

Encountered again at GSFC, this is a bug on little endian machines (header gets interpreted incorrectly by cFE).

[BaldBeacon]

If this seems like an issue that's being encountered more frequently, I'll go ahead and construct a solution. I still need to sign the contributor documentation so if this is something you'd like to do fast I wouldn't be slighted if this issue was distributed amongst the cFE team.

[jphickey]

Encountered again at GSFC, this is a bug on little endian machines (header gets interpreted incorrectly by cFE).

It's not really a "bug" - it is working as designed. Only the CCSDS primary header is put into a fixed byte ordering. The secondary header is not in a specified byte order, nor is the payload. The design of CFE leaves this up to systems engineering to make it compatible.

It is not clear what value making the secondary header into a constant byte order will have, when the payload of the command inside it is also an unspecified byte order. If the code cannot decode the secondary header correctly, how will it decode the command contents?

For an actual solution that enables full compatibility between different CPU architectures, something like EDS is needed. See https://github.com/jphickey/cfe-eds-framework as a proof of concept - not saying this is the only way, but this was used on some previous projects with success.

[skliper]

It is a bug in that it does not match the GSFC secondary header definition which is byte order agnostic. This field should NOT be uint16, it definitely does not meet the spec.

EDIT - grammer.

[jphickey]

it definitely does not meet the spec.

Can you clarify which spec the secondary header is to be in accordance with? As far as I know there is no spec for the secondary header. (CCSDS does not define this, does it? Now I need to go back and look...)

[jphickey]

FWIW, I went back and reviewed book 133 section 4.1.3 (https://public.ccsds.org/Pubs/133x0b1c2.pdf)

The only thing it really says about secondary header is that it might have a time code and/or ancillary data, but both are optional -- see 4.1.3.2.1.5. Depending on how one interprets this, it could be argued that the CFE command header isn't even compliant with this. (i.e. we should possibly be setting the secondary header bit 0 and treating the whole packet as user data, which happens to start with a CFE command header).

[skliper]

Correct, CCSDS does not cover it. Although I think @jwilmot may be working to register 2ndary headers? It's carried in various GSFC project specs, I'll need to dig (or maybe @acudmore knows) if there's something more general.

[jphickey]

If it is documented as above in GSFC specs then yes, I concur that the definition in the C header as a uint16 value is incorrect and should be two uint8 values.

However, the problem remains that this still will not really gain anything in terms of cross-system compatibility issues, except maybe for commands that do not have payloads (maybe one can make a case for scheduler running on a separate CPU of a different endianness from the app its scheduling?)

[jwilmot]

The secondary header must be in network byte order (Big Endian) It's kinda hard to parse without it. GSFC lucked out in the past as we really only fly Big Endian architectures. When we register in SANA it will be Big Endian. The payload will still be in the form it was sent. That is where the EDS comes in.
Artemis/Gateway will use the secondary header in a mixed endian system. Systems should use the EDS to interpret the payload as Joe states. The secondary header is part of the key to interpret the payload based on EDS

[skliper]

We gain compliance with the secondary header definition which is currently an issue for multiple projects we support locally. Understood there's other documented issues with payload and/or other byte ordering but this one could be solved independently and immediately.

[BaldBeacon]

except maybe for commands that do not have payloads

From my experience, it's easy to control the user data field at an app level but needing to only flip 1 word in the header leads to a weird counter-intuitive situation. A networking spec usually defines the header (and in this case the secondary) as being byte-order agnostic and leaves the user data section to the user to handle (in this case the app developer).

this one could be solved independently and immediately

I think this is important to note with extra emphasis put on the since multi-byte value in the secondary header which isn't consistent with the rest.

[jphickey]

A networking spec usually defines the header (and in this case the secondary) as being byte-order agnostic and leaves the user data section to the user to handle (in this case the app developer).

Agreed, this would be true if CCSDS had specified the format of the secondary header, but up to this point it is really just a free-form "ancillary data field" in accordance with CCSDS book 133 section 4.1.3.2.3.

But if the format is to be registered in SANA, then yes I concur that this then becomes a specific format requirement and should be updated.

Note on the telemetry side of things that somebody added a means to force the timestamp to be in network byte order, but it is optional. Assuming both the CMD and TLM secondary headers are being registered in SANA, this option will need to be deprecated - as only the network byte order would be correct at that point.

[BaldBeacon]

this option will need to be deprecated

I'd rather not handle that as part of fixing this issue but the CFE team should most likely open an issue to deprecate the option once a resolution is made for this issue.

[jphickey]

Separating the commits is fine/good but I do think we should strive to address the complete compliance issue as part of this change request, as both the CMD and TLM headers are probably inconsistent. The goal should be to make the code fully compliant with whatever GSFC has officially submitted to SANA for the definition of the secondary headers employed by CFE.

The TLM is likely just a matter of removing the noncompliant half of an #ifdef compile time option, not sure if it needs to go through the full deprecation process like an API would, perhaps a good topic of discussion for the next meeting. But I don't see much point in preserving old behavior if it is noncompliant with a published interface.

[skliper]

#311 is the broad issue (at least for headers). Let's keep this one focused, since projects are having issues with it now. The general issue isn't really non-compliance until we get an agreed-to standard (I haven't seen any progress on that front). I agree with the goal, which has been the goal and will continue to be the goal, but this issue is well defined and can be solved immediately.

[jphickey]

I'm still confused about this .... As far as I can tell the only real justification for changing the code would be to make it compliant with a published (or otherwise documented and agreed-to) format.

since projects are having issues with it now

This is the part I don't get -- is this actually a blocking issue for a project, and do they expect that changing this will enable them to proceed and become unblocked?

This is why I keep bringing this up, sorry if its repetitive, but I'm detecting undertones of "if this was just fixed then my system would work" -- but it probably won't actually work, they'll just get caught by the next layer of incompatibility. So I want to be sure we aren't setting false expectations by "fixing" this.

[skliper]

Projects are actively having to actively violate the GSFC standard (the snippet above) on the ground side to "work around" this issue. They've requested a fix to this specific issue, regardless of if they will will run into others.

[jwilmot]

The Secondary header is a standards issue and must be parsable on any system without a prior knowledge of the source. Network Byte order is required. The first few bits will be the header version number to distinguish between ESA ECSS PUS Space Packets and NASA Space Packets on Artemis/Gateway and other future missions that must interoperate. The EDS will then allow the payloads to be interpreted between system. Both the ESA header and the NASA header will be registered in CCSDS SANA.

[BaldBeacon]

Just to be clear, nothing has indicated to me that this issue should be closed. Can I move forward on making the change necessary? I'd assume it's just the one line change, altering the secondary header definition to use an int8[2] rather than an int16. Since the memory is contiguous and of the same size, I'd assume that all the convenience macros and functions should still work correctly but should I run the unit-tests to be sure?

[skliper]

The comments should also be updated to match the definition. I wouldn't assume anything with any references (in code, macros, etc), they should all be visually confirmed as well as run against the unit tests.

[jphickey]

Note that the access macros for the command code and checksum will need to be updated as well.
In addition to unit tests, make sure to run the FSW and send a few commands (I like to do at least a few different ES commands, and at least something with a nonzero command code i.e. beyond noop)

[BaldBeacon]

send a few commands

Do you have a recommended way of doing this without standing up a ground system?

[jphickey]

Do you have a recommended way of doing this without standing up a ground system?

Yes, use the "cmdUtil" command line tool. Use the --pktid and --cmdcode options (as well as --endian LE) to send a command via CI_LAB. Hopefully this tool uses the same macros to assemble the packet - I think it should, but needs to be checked. Note the trace and make sure it sends the packet correctly.

[irowebbn]

Was the byte order for the secondary header ever standardized with SANA? I don't see any documents regarding that here.

[skliper]

@irowebbn - Not that I know of. Although the secondary command header has been updated to be endian agnostic:

cFE/modules/msg/option_inc/default_cfe_msg_sechdr.h

Lines 55 to 65 in 10898c9

	typedef struct
	{
	uint8 FunctionCode; /**< \brief Command Function Code */
	/* bits shift ---------description-------- */
	/* 0x7F 0 Command function code */
	/* 0x80 7 Reserved */
	uint8 Checksum; /**< \brief Command checksum (all bits, 0xFF) */
	} CFE_MSG_CommandSecondaryHeader_t;

[irowebbn]

@skliper, I notice that the comment for the telemetry secondary header says the 6-byte time field is big endian- is that required by a standard or listed in a document somewhere?

[skliper]

@irowebbn - I've never seen these secondary headers detailed in any "external" standards. Only project/internal ICD's and similar.