Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CFE_SB_GetMsgTime() and CFE_SB_SetMsgTime() do not handle byte-swapping on _EL platforms #92

Closed
skliper opened this issue Sep 30, 2019 · 13 comments · Fixed by #801 or #833
Closed
Assignees
Labels
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

Assuming CCSDS telemetry packet secondary header timestamps should be big-endian format, on little endian platforms (when _EL is defined), CFE_SB_SetMsgTime and CFE_SB_GetMsgTime should swap bytes for proper time interpretation.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 61. Created by cdknight on 2015-06-01T17:41:51, last modified: 2019-07-03T12:58:32

@skliper skliper added this to the 6.8.0 milestone Sep 30, 2019
@skliper skliper self-assigned this Sep 30, 2019
@skliper skliper added the bug label Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-06-04 12:20:04:

In 6.4.1 everything beyond the CCSDS primary header is in the "native" endianness of the target. So this behavior (timestamp as little endian) is actually correct/intended in the 6.4.1 release.

It is also that way in the command secondary header as well - the 16-bit field will be swapped on EL platforms.

This is part of a much bigger issue that extends well beyond the headers -- the entire payloads of all messages are subject to this.

The "fix" for this is to use electronic data sheets, so that all messages can use a consistent encoding across the entire mission, and CPUs of different architectures (incl. ground systems) can understand each other's messages reliably.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by cdknight on 2015-06-04 16:13:27:

(FYI, I am new to CCSDS and the CFS community, so I defer to experts and the community consensus.)

fsw/cfe-core/src/inc/ccsds.h defines the structures of primary and secondary headers and macros for examining the (big-endian) primary headers. This, to me, implies that both primary and secondary headers should be big-endian.

Of course, the payload data of the CCSDS packets is application-dependent and the byte ordering is up to the application. But for cross-application applications (such as limit checker), having secondary headers in an unknown/mixed byte order will be problematic.

Conversely, if we rely on the mission to define byte ordering of secondary headers, why not return to the 6.3.X world and not define byte order for the primary header?

Also FYI, my ds_replay application takes a per-StreamID configuration as to what secondary header's byte order is; it works but is clunky and a very manual process but, certainly if the mission picks one byte order, would be over-engineering the solution.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-06-04 16:40:46:

The CCSDS primary header (only) is specified as big-endian.

The secondary header is implementation-specific. The TLM header is defined in ccsds.h as a uint8 array not for endian-swapping but rather due to misalignment (you cannot instantiate a uint32 at a 6 byte structure offset directly, so it is memcpy'ed in instead).

I fully agree that this is not ideal and not even workable in many circumstances. Attempting to have CPUs/ground stations of different architectures communicate with each other presents a minefield of problems, TLM timestamps being just one example.

The complete solution to this problem --- the CCSDS SOIS-App working group has defined an XML schema for "electronic data sheets" (EDS) that can be used to describe the binary format of the inter-processor message components. With this, different encodings/byte orders/etc can be reliably handled in a translation layer where needed so that everything always works properly. EDS solves this problem not only for the TLM headers noted here but also CMD headers and packet payloads for ALL apps.

EDS is actually implemented/working in a babelfish branch called {{{eds-preview}}} -- feel free to check this out if you'd like. The only difference is this currently uses an undocumented/unofficial XML schema, and I am currently working to convert this to use the official CCSDS SOIS-App schema.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by sstrege on 2016-11-22 15:14:36:

This ticket will be worked/resolved under ticket #207

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2017-10-19 16:00:15:

Propose pushing this out to cfe-next release.

For CFE 6.6 we are including the EDS datasheets but not (yet) the remainder of the runtime support. In the meantime the fix is available via the techdev-sois-eds branch for interested parties (it fixes this issue described here)

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-03-20 15:40:16:

See #243

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-05-15 16:39:44:

MSFC brought this up, marked for CCB review - see email sent 5/15, subject "cFS Items to Report".

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-05-22 15:55:12:

CCB 5/22/2019 - Discussed

  • Current state is cFS uses target endianness for payload, any interactions would have to match

  • Populates ENDIAN field to match target, and everything except primary header is in target format

  • No secondary header conversion based on extended/V2 which indicates ENDIANNESS

    • Command code on the way in or time on the way out
  • Need Jonathan's input as to where extended header is at in terms of standards

  • Issue is related to entire payload, which from CCSDS perspective includes secondary header (not just time)

  • Long term is EDS and all the tool chain updates, possibly 7.0 target

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-05-30 12:18:42:

Follow up discussions (see cfs_cfe #92 email chain)

  • Jonathan stated secondary should be network byte order (again still in proposed state) and the endianness indicator flag would cover the rest/payload.
  • Joe suggested could check endian flag, dump, and generate event/syslog if it doesn't match system
  • Note conversation initiated from MSFC email, and they were using CCSDS version 1 and just assuming everything was network order.

Lowered priority back to major.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-05-30 16:28:26:

Spawned off #331 to address the extended header in network byte order.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-06-10 14:40:09:

Pending documentation/standard definition by Jonathan.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-07-03 12:58:32:

Moved unfinished 6.7.0 tickets to next release

@skliper skliper assigned jwilmot and unassigned skliper Sep 30, 2019
@skliper skliper removed this from the 6.8.0 milestone Nov 4, 2019
@skliper skliper changed the title CFE_SB_GetMsgTime() and CFE_SB_TimeStampMsg() do not handle byte-swapping on _EL platforms CFE_SB_GetMsgTime() and CFE_SB_SetMsgTime() do not handle byte-swapping on _EL platforms May 12, 2020
@skliper skliper added this to the 7.0.0 milestone Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants