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

Clarify buffer ownership for ByteStreamDriverModel #3040

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

celskeggs
Copy link
Contributor

Related Issue(s) n/a
Has Unit Tests (y/n) n/a
Documentation Included (y/n) yes

Change Description

TcpClientComponentImpl::send_handler and TcpServerComponentImpl::send_handler deallocate the provided Fw::Buffer if (and only if) they return SEND_OK or SEND_ERROR. If they return SEND_RETRY, they do not deallocate the buffer.

Rationale

To make sure that users of the ByteStreamDriverInterface can implement the correct buffer handling logic, it's important to define what happens with ownership of the buffer.

Testing/Review Recommendations

N/A

Future Work

UdpComponentImpl::send_handler does not implement the interface in the same way: notably, it also deallocates the buffer when it returns SEND_RETRY. I believe this is incorrect and has not been noticed because SEND_RETRY is unlikely (or maybe impossible) for UDP connections. Some revision to that code may be necessary.

@LeStarch
Copy link
Collaborator

@celskeggs thanks for this clarification! Also, you are right on the UDP issue and we should fix it for conformity with this pattern.

I took a look at the UART driver (also a byte stream) and it has one line we might review:

if (isConnected_deallocate_OutputPort(0)) {

That is, it only takes ownership in the case when the port is attached. I'd argue that it would be better to require that port to be attached. There are other patterns to prevent deallocation that would be clearer given the intended constraints of this pattern you documented here.

@LeStarch LeStarch merged commit f350719 into nasa:devel Nov 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants