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

Proposal: Add data mask to StreamInterface #121

Open
zyp opened this issue Jun 15, 2021 · 2 comments
Open

Proposal: Add data mask to StreamInterface #121

zyp opened this issue Jun 15, 2021 · 2 comments
Labels
enhancement potential new feature

Comments

@zyp
Copy link
Contributor

zyp commented Jun 15, 2021

Hi, in 47daf80 I changed the USBStreamOutEndpoint endpoint to give a transfer oriented stream rather than a packet oriented stream, but there are a couple of corner cases that it still doesn't handle correctly.

The first one is a transfer consisting of one or more full packets; it doesn't know which packet is the last packet before the terminating ZLP arrives, and by that time the last byte of the transfer is already on the stream (or in the FIFO) without the last flag set.

The simple and obvious solution would be to hold back the last byte of a full packet until the next packet arrives, so the last flag can be correctly determined, but this can potentially cause issues with bytestream pipes like ACM, where some hosts will happily send you a series of full packets without following up with a ZLP.

Another corner case is zero length transfers -- with zero bytes of data, they would cause zero stream transfers and effectively get blackholed.

To solve both these cases, I propose adding a data mask to StreamInterface. A zero length transfer can then be signalled as a single stream transfer with first=1, last=1, mask=0, and a ZLP arriving after a full packet can terminate the ongoing transfer with last=1, mask=0.

To make the stream easier for consumers to work with, I also propose a couple of stream filters that strips out transfers with mask=0:

  • A simple one that just does valid = valid & mask for consumers that doesn't care about transfer boundaries.
  • A buffered one that holds one byte back until it gets the last flag.

The mask field can also serve a dual purpose as a byte mask for a multibyte stream.

@zyp
Copy link
Contributor Author

zyp commented Jun 19, 2021

I just realized that this would also simplify bytestream IN buffer management.

The problem with converting bytestreams into packets is that you generally don't want to wait for a full packet before you send it, but it's also not efficient to send one packet per byte, so you generally need a timeout for when you've waited long enough for the buffer to fill.

With a data mask, the buffering could be implemented by a stream filter that passes the data straight through while monitoring activity, and when it hits a timeout it'll generate a stream transfer with last=1, mask=0 that'll tell the USBStreamInEndpoint to either send the data it got so far as a short packet or send a ZLP to ensure that the host processes the previous full packet that was sent in case it's still pending in the middle of a transfer.

This is doable without a data mask, but then requires holding back one byte so the last flag can be sent together with it at timeout, so having the data mask would be a simplification.

@straithe straithe added the enhancement potential new feature label Nov 1, 2021
@martinling
Copy link
Member

Hi @zyp,

I have just run into this issue from the IN side whilst working on the USB analyzer gateware.

When capture has been stopped, we want to send out all the remaining data from our capture buffer to the host. But the last byte may have already been sent to the USBStreamInEndpoint, without last having been set, so bytes end up stuck in the packet buffer in USBInTransferManager, and there's no way to push them out as it stands.

We need a way to retroactively signal that the stream is now complete. I was thinking about adding a separate flush signal, but I like the generality of your solution of adding a data mask to StreamInterface and signalling this as last=1, mask=0.

I'd be interested in any further thoughts anyone has on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement potential new feature
Projects
None yet
Development

No branches or pull requests

3 participants