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

Split message definitions from headers #35

Closed
skliper opened this issue Sep 30, 2019 · 11 comments
Closed

Split message definitions from headers #35

skliper opened this issue Sep 30, 2019 · 11 comments
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

All of the cFS messages are currently defined in C structures. As a first step toward moving to "electronic data sheets" to describe the external data format, these need to be slightly modified to better separate the header portion of the structure from the payload portion of the structure.

Currently, message structures are typically defined by reserving a block of space for the header as a uint8 array of size CFE_SB_CMD_HDR_SIZE or CFE_SB_TLM_HDR_SIZE. This approach has several issues:

  • Using a fixed-size block assumes only a single type of encapsulation (CCSDS) will ever occur. This may not be the case, as other non-CCSDS encapsulations may be a requirement for some missions.
  • The fixed size block is not guaranteed to be properly aligned for a CCSDS header. Since it is declared as a uint8 array, the compiler will not ensure any alignment this structure. It is technically not valid to cast this as a CCSDS header since that contains uint16's.
  • This is unlikely to be compatible with electronic data sheets (EDS) no matter what specific implementation is used. Since the definition of the message content (payload) and the message header (CCSDS or other format) will come from different data sheets, it becomes very problematic to have them mixed together like this.

To solve this problem requires a bit of restructuring: instead of declaring the format of the payload directly within the message structure, declare a separate "Payload" structure and define it in there.

This adds one extra layer to the structure tree but will improve flexibility going forward, and it will NOT change the external data format, so compatibility with ground systems is unaffected. It only affects the syntax of code accessing members of the payload structure.

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

skliper commented Sep 30, 2019

Imported from trac issue 4. Created by jphickey on 2014-12-22T13:49:40, last modified: 2019-03-05T14:57:55

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2014-12-26 10:12:52:

Pushed branch "trac-4-cfe_payload_separation" which addresses this issue. Final commit is [changeset:4fdeade6] which includes the change for all core applications and unit tests.

In general:

  • For all command and telemetry messages a new "_Payload_t" structure is defined which reflects the content of the message //without// any headers.
  • The existing "_t" structure now just wraps together the Payload structure with a reserved block of space for a CCSDS header, exactly as was done before.
  • References are fixed in one of two ways: either the CFE_SB_MsgPtr_t (message header pointer) is changed to a a CFE_SB_MsgPayloadPtr_t, or the reference was fixed to have an extra ".Payload."

Merging this change will ease the forthcoming electronic data sheets (EDS) change since the Payload structures and the Header structures come from different data sheets, they are different structures. This just updates the code to follow that model so the data sheet will drop-in easily. There should be no difference in the binary format of the final message.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-03-04 14:17:49:

Accepting this change into CFE will have to be done in lockstep
with accepting a corresponding change to several CFS applications.

Too bad "unnamed structure fields" are a C-11 feature, it would have
been a nifty way to do this in a source-compatible way.

Side note: this may change where the compiler inserts alignment padding
and could change the size of the packet. Consider the case where the
border between the header and the payload is in the middle of a word,
and the payload contains data elements that require word alignment: the
inner structure will have padding to assure the aligned field is at an aligned
offset within the inner structure, and separately, padding will be inserted
before the payload structure in the packet structure so that the payload
starts at an appropriately aligned offset.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-03-04 14:50:10:

Greg - it may be possible to do this for the apps separately. In general, the apps define their own messages and most generally just talk to the ground station and not to each other (exceptions perhaps for the aggregator apps like HS/HK, these might be more complicated).

Also - the alignment issue is noted and I can confirm that this is not a problem in the currently defined messages since the combined CCSDS primary + secondary headers are either 8 or 12 bytes, it is always a multiple of 4.

There is actually a side benefit to this too: it ensures that all packet payloads can also be instantiated without encapsulation headers - this allows use of alternate encapsulations in the future, even the option for a transparent encapsulation added by "SB" as the packet goes out.

In general having message constructs with misaligned words (made possible by an odd-sized element before it) should be very much frowned upon as it may impede portability to other processors or other types of message encapsulation.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2015-04-06 11:21:23:

This is ready for review/merge

Note - #68 may need to be merged at the same time which fixes some UT code that was only built when using UT_VERBOSE.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by acudmore on 2015-04-06 14:26:03:

recommend accept.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jwilmot on 2015-04-06 15:15:12:

Concur.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-04-07 12:44:23:

Tested changeset [changeset:4fdeade] as part of the ic-2015-03-10 merge.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2015-04-13 15:28:43:

Part of integration candidate 2015-03-10,
committed to cFS CFE Development branch on 2015-04-10
as part of merge [changeset:7d6f6d0].

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2016-02-25 10:17:32:

these will be fixed in CFE 6.5

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jhageman on 2019-03-05 14:57:55:

Milestone renamed

@skliper skliper closed this as completed Sep 30, 2019
@skliper skliper removed their assignment Sep 30, 2019
dmknutsen pushed a commit that referenced this issue Jan 23, 2020
Fix #32, #34
Reviewed and approved at 2019-12-18 CCB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant