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

Fix #1020, refactor SB buffer descriptor object #1154

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Feb 4, 2021

Describe the contribution
Refactor the SB buffer descriptor object (CFE_SB_BufferD_t) and simplify the zero-copy buffer paradigm.

Combine the zero-copy and the normal CFE buffer descriptor into a single unified CFE_SB_BufferD_t object. This cleans up a bunch of extra logic related to zero-copy buffers, including the extra descriptor object. The result is a simpler zero-copy design that is much less different from the standard (non-zero-copy) message path.

All message descriptor objects are now tracked in a list by SB, not just the zero-copy descriptors (for consistency - if any buffers need to be tracked, they should all be tracked).

This notably puts the buffer content as a member within the descriptor, rather than a calculated pointer, so it will be aligned properly.

Fixes #1020

Testing performed
Build and run all unit tests
Sanity check CFE

Expected behavior changes
All changes are internal to SB. This does not affect API or behavior of any existing APIs (but see note)

It also ensures that zero-copy buffers (and the associated CFE_SB_TransmitBuffer API) behave as similarly as possible to the normal CFE_SB_TransmitMsg API. Notably this corrects a minor issue where the MsgSendErrorCounter would get incremented if there were no subscribers, but only in the zero copy API.

System(s) tested on
Ubuntu 20.04

Additional context
This does not change public API in any way. However now there is no extra descriptor for the "zero copy" buffers - they are all just buffer descriptors, regardless of whether they were allocated on the fly or pre-allocated by the application.

This means that the CFE_SB_ZeroCopyHandle_t value that is output from CFE_SB_ZeroCopyGetPtr() and others is now largely redundant. It is equal to the buffer descriptor address, but it can be easily determined from the buffer content pointer too, so it is really not necessary to pass around two values for zero copy buffers. This can be simplified in a future change - but that will change the public API.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey
Copy link
Contributor Author

jphickey commented Feb 4, 2021

Like others this will need a rebase when the next mainline is done, actual change is in commit 365c095

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Feb 5, 2021
bd->MsgId = MsgId;
bd->UseCount = 1;
bd->Size = Size;
bd->Buffer = (CFE_SB_Buffer_t *)address;
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to see this updated. This line in the current main branch increases alignment requirements on ARM32 when going from the uint8* to CFE_SB_Buffer_t*.

@skliper skliper added the Priority: Mission Feature or bug related to stakeholder needs label Feb 16, 2021
@skliper
Copy link
Contributor

skliper commented Feb 16, 2021

@jphickey can you rebase this one? Blocking tests on vxworks...

@astrogeco
Copy link
Contributor

CCB 2021-02-17 APPROVED

  • Adds to CFE_SB_BUFFER_t
  • Removes CFE_SB_ZeroCopyD_t
  • Consolidates information about buffer and descriptor without distinction between a "zero copy" and a "regular" one

@astrogeco astrogeco added conflicts and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Feb 17, 2021
@jphickey
Copy link
Contributor Author

jphickey commented Feb 26, 2021

Rebased to the new main branch....

Edit: looks like a new warning popped up, so not quite ready yet - will take a look.

Combine the "zero copy" and the normal CFE buffer descriptor into a single
unified CFE_SB_BufferD_t object.  This cleans up a bunch of extra logic
related to zero-copy buffers, including the extra descriptor object.  The
result is a simpler zero-copy design that is much less different from the
standard (non-zero-copy) message path.

All message descriptor objects are now tracked in a list by SB, not just
the zero-copy descriptors (for consistency - if any buffers need to be
tracked, they should all be tracked).
@jphickey jphickey marked this pull request as ready for review February 26, 2021 20:46
@astrogeco astrogeco changed the base branch from main to integration-candidate March 1, 2021 20:31
@astrogeco astrogeco merged commit c808e91 into nasa:integration-candidate Mar 1, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Mar 1, 2021
@jphickey jphickey deleted the fix-1020-sb-buffers branch March 10, 2021 14:43
@skliper skliper added this to the 7.0.0 milestone Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Mission Feature or bug related to stakeholder needs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Questionable address adjustment in SB buffers may break alignment requirements
4 participants