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.

Sign up for GitHub

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

Jump to bottom

Fix #62, Apply message alignment pattern #63

Merged
merged 2 commits into from
Dec 1, 2020
Merged

Fix #62, Apply message alignment pattern #63

merged 2 commits into from
Dec 1, 2020

Conversation

skliper
Copy link
Contributor

@skliper skliper commented Nov 18, 2020

Describe the contribution
Fix #62 (updates related to nasa/cFE#1009, implementing a consistent message alignment pattern)

  • Use CFE_SB_Buffer_t for receiving and casting to command types
  • Use CFE_MSG_CommandHeader_t and CFE_MSG_TelemetryHeader_t in
    comamnd and telemetry type definitions
  • Use CFE_SB_TransmitMsg_t to copy the command and telemetry
    into a CFE_SB_Buffer_t and send it where needed
  • Avoids need to create send buffers within the app (or union
    the packet types with CFE_SB_Buffer_t)
  • Eliminates references to CFE_SB_CmdHdr_t and CFE_SB_TlmHdr_t
    that formerly enforced alignment since these had potential
    to change the actual packet sizes
  • No need to cast to CFE_MSG_Message_t anywhere since it's
    available in the CFE_SB_Buffer_t union

Testing performed
Normal build, tested cmd/tlm interface and noops

Expected behavior changes
None.

System(s) tested on

  • Hardware: cFS Dev Server
  • OS: Ubuntu 18.04
  • Versions: Bundle main + this commit

Additional context
See nasa/cFE#1009

Third party code
None.

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman - NASA/GSFC

@skliper skliper added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Nov 18, 2020
@astrogeco
Copy link
Contributor

astrogeco commented Nov 18, 2020 •
edited
Loading

CCB 2020-11-18 APPROVED

@skliper
Copy link
Contributor Author

skliper commented Nov 18, 2020

Push fixed typo in commit message and applied "*Cmd_t" pattern to all command types.

jphickey
jphickey approved these changes Nov 24, 2020
View reviewed changes
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one minor comment/suggestion

fsw/src/ci_lab_app.c
@@ -47,26 +47,19 @@
*/
typedef union
{
CFE_MSG_Message_t MsgHdr;
CFE_MSG_Message_t Msg;
uint8 bytes[CI_LAB_MAX_INGEST];
uint16 hwords[2];
} CI_LAB_IngestBuffer_t;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "hwords" member here really should not be needed. AFAIK it served two purposes - to align the buffer to 16 bits (not needed if using CFE_MSG_Message_t) and in the event the message didn't validate it would actually report the value of the first two words in an error message. But that message is fundamentally flawed because if the length check doesn't work the values probably are undefined/uninitialized and thereby meaningless.

@skliper skliper force-pushed the fix62-alignment-pattern branch 2 times, most recently from 42b969b to 05c1424 Compare November 30, 2020 14:02
@skliper
Fix nasa#62, Apply message alignment pattern
e6d740a 
- Use CFE_SB_Buffer_t for receiving and casting to command types
- Use CFE_MSG_CommandHeader_t and CFE_MSG_TelemetryHeader_t in
  command and telemetry type definitions
- Use CFE_SB_TransmitMsg to copy the command and telemetry
  into a CFE_SB_Buffer_t and send it where needed
- Avoids need to create send buffers within the app (or union
  the packet types with CFE_SB_Buffer_t)
- Eliminates references to CFE_SB_CmdHdr_t and CFE_SB_TlmHdr_t
  that formerly enforced alignment since these had potential
  to change the actual packet sizes
- No need to cast to CFE_MSG_Message_t anywhere since it's
  available in the CFE_SB_Buffer_t union
@skliper
Fix nasa#62, Removed hwords and replaced deprecated elements
3ed636d 
 - Replaced CFE_SB_RcvMsg with CFE_SB_ReceiveBuffer
 - Replaced CFE_MSG_Size_t with size_t
@skliper
Copy link
Contributor Author

skliper commented Nov 30, 2020

Minor updates to minimize future changes, and removed hwords. Left in report as a "clue".

@astrogeco astrogeco added IC-20201124 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Dec 1, 2020
@astrogeco astrogeco changed the base branch from main to integration-candidate December 1, 2020 17:03
@astrogeco astrogeco merged commit dee470d into nasa:integration-candidate Dec 1, 2020
astrogeco added a commit to astrogeco/cFS that referenced this pull request Dec 1, 2020
@astrogeco
Add nasa/ci_lab#63
c4958e4
@astrogeco astrogeco mentioned this pull request Dec 1, 2020
Merged
astrogeco added a commit to astrogeco/cFS that referenced this pull request Dec 2, 2020
@astrogeco
Add nasa/ci_lab#63
3a7c68e
skliper added a commit to skliper/cFS that referenced this pull request Dec 3, 2020
@skliper
Bundle main + updates for alignment pattern
3899057 
- nasa/cFE#1015
- nasa/ci_lab#63
- nasa/sample_app#112
- nasa/sch_lab#57
- nasa/to_lab#69
skliper added a commit to skliper/cFS that referenced this pull request Dec 3, 2020
@skliper
Bundle main + updates for alignment pattern
fbfd230 
- nasa/cFE#1015
- nasa/ci_lab#63
- nasa/sample_app#112
- nasa/sch_lab#57
- nasa/to_lab#69
astrogeco added a commit to nasa/cFS that referenced this pull request Dec 8, 2020
@astrogeco
Add nasa/ci_lab#63
e952ad7
astrogeco added a commit to nasa/cFS that referenced this pull request Dec 9, 2020
@astrogeco
Add nasa/ci_lab#63
d30f38c
@skliper skliper deleted the fix62-alignment-pattern branch February 1, 2021 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jphickey jphickey jphickey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update for suggested alignment enforcement pattern (nasa/cFE#1009)
3 participants
@skliper @astrogeco @jphickey