Update EndpointIn to allow arbitrary size writes

[simpkins]

trafficstars

This updates the EndpointIn::write() API to support writes larger than the endpoint's maximum packet size. A new EndpointInSinglePacket helper trait was added to support driver implementations that only allow writing a single packet at a time. At the moment all of the provided drivers are still using this implementation, but they could be updated to write more data at once in the future.

This has not yet changed any code in embassy-usb to send larger packets. I wasn't sure if it would be better to bump the embassy-usb-driver version number before doing that, since this is a meaningful change, and we would want to ensure that larger-sized writes aren't sent to older driver implementations that do not support this.

[simpkins]

One caveat here: I haven't tested the embassy-stm32, embassy-nrf, or embassy-rp implementations on actual hardware. That said, the unit test should hopefully provide some confidence that it works correctly.

I also have a corresponding change almost ready for the read() APIs, but figured I would send the write() change out for comments first.

One possible concern about this API change: if a write() or read() operation is dropped (say, if it was abandoned because of a timeout), the caller can no longer really reason about how much data may have been sent or received before the operation was dropped. Unless this is an isochronous endpoint, the user would presumably need to reset the bus after a dropped operation to get back to a known state. I'm not sure if people feel like this problem matters in practice, and if callers would want to attempt to resume using an endpoint without resetting it even after a timeout. Even today, it feels somewhat burdensome to require that usb driver implementations guarantee no data loss after a dropped read(): it means that driver implementations must always have enough buffer space of their own to store read data between a dropped read() and the next time the user calls read() to get this data. This just happens to be more feasible to do today with at most 1 packet outstanding, and harder if the read could be many packets long.

[Dirbaio]

Since this is a breaking change to embassy-usb-driver which is somewhat disruptive, i'd prefer to wait until at least one in-tree driver implements the write() API (and ideally demonstrates a perf improvement as a result). Otherwise if we merge this now we're taking the downside of the break with no upside.

Also, I wonder if instead we could

• achieve the increased performance with the current API, for example implementing double-buffering in the drivers.
• achieve the better user-friendliness with improvements in embassy-usb only, for example EndpointIn/EndpointOut wrappers that do the chunking for you, but still call the current "write packet / read packet" API.

[simpkins]

Since this is a breaking change to embassy-usb-driver which is somewhat disruptive, i'd prefer to wait until at least one in-tree driver implements the write() API (and ideally demonstrates a perf improvement as a result). Otherwise if we merge this now we're taking the downside of the break with no upside.

Sure, that makes sense. I'm happy to contribute an implementation for embassy-stm32 and wait before that is done for this to go in. I did just get a black pill board and have that working now, so I can do some testing on stm32. I have code for esp32 that supports streaming transfers, and I should be able to port that over.

[simpkins]

This update incorporates some of the review feedback:

• I renamed write_one_packet() to write_packet()
• To mitigate some of the breaking changes in the embassy-usb-driver API, I left the write() API the same, and added this new behavior as a separate write_data() API. (write() now has a default implementation that calls write_data(), so it supports larger writes, but any existing code that calls it with a single packet at a time should still behave exactly the same.) That said, this is still a breaking change for embassy-usb-driver implementations (since they now have to implement write_data()), but it avoids breaking the API for users of the API in embassy_usb
• The new write_data() API accepts the data, plus a force_short_packet parameter to indicate if the caller wishes the implementation to send a zero-length packet at the end of the transfer if the data is a multiple of the max packet size.

I also have an implementation for embassy-stm32 mostly ready, over here: https://github.com/simpkins/embassy/commit/867c7c1908caecda6913015c4b6eba172b15aed7 However, I've only done pretty cursory testing of it so far (I mainly just hacked up one of the usb_serial examples to always send back large chunks of data on any input, rather than echoing the input data).

[Dominaezzz]

the caller can no longer really reason about how much data may have been sent or received before the operation was dropped

Can the method just return a count of the sent packets or bytes?

Is EndpointError::BufferOverflow still needed after this PR?

[simpkins]

Can the method just return a count of the sent packets or bytes?

No, unfortunately it can't: if the operation is dropped there is no opportunity to return anything at all.

Is EndpointError::BufferOverflow still needed after this PR?

It is still needed in the receive case. Even if we update the read APIs to allow reading more than a single packet worth of data at once, if the caller provides a buffer whose length is not a multiple of the max packet size, we can encounter a BufferOverflow situation if the final packet contains more data than the caller expected.

[Dirbaio]

marking as draft until these concerns are solved.