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

Add new read_setup() for control endpoints #153

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ianrrees
Copy link
Contributor

Background can be found at atsamd-rs/atsamd#738 .

The USB spec says that the SETUP transactions used with control endpoints must always be accepted by the device [1], and at least in the Microchip SAMD21's USB peripheral that behaviour is handled in hardware [2]. We've observed that some hosts will send an OUT ZLP, followed very shortly by a SETUP for a new transaction - on the SAMD21 parts this results in the OUT data (none, in this case) being overwritten by SETUP, which in the existing implementation results in an overflow error (since read() was called with a zero length slice for the OUT ZLP, but >0 bytes are available from the SETUP that overwrote it). The user-observable result is that the device fails to enumerate at all on some hosts, or sporadically on others.

In light of this, it seems the control pipe code needs some way to differentiate reads of OUT data from reads of SETUP data. This PR changes the existing read() methods to return an error if SETUP data is present, making it specific to OUT data, and adds a new read_setup() method specifically for reading SETUP data.

Most of the commits/diff here are work on the test framework, to add a new test that resets the device and times how long it takes to re-enumerate. The test is a bit crude, but seems to validate the changes to the read methods.

[1] USB 2.0 spec section 5.5.5 Control Transfer Data Sequences:

If a Setup transaction is received by an endpoint before a previously initiated control transfer is completed,
the device must abort the current transfer/operation and handle the new control Setup transaction. A Setup
transaction should not normally be sent before the completion of a previous control transfer. However, if a
transfer is aborted, for example, due to errors on the bus, the host can send the next Setup transaction
prematurely from the endpoint’s perspective.

[2] SAMD21 datasheet DS40001882H section 32.6.2.6 Management of SETUP Transactions. The key thing here is that the data is copied in to the buffer regardless of BK0RDY.

Need to support a test that hard resets the device and ensures that it
re-enumerates.  The current model supplies a device handle to the test
case, but this case will require a handle to the rusb context so that it
can determine when the device has enumerated.
This reduces log noise for tests that reset the device under test
Needed for testing enumeration time
This change was made in response to an issue found on the Microchip
SAMD21, where an expected OUT Zero Length Packet (ZLP) was sometimes
overwritten by a subsequent SETUP transaction.  The USB spec requires
that devices accept SETUP transactions, and in the SAMD parts this
behaviour is implemented in hardware.  To allow the control pipe
implementation to distinguish between OUT data and SETUP data, the
read() method was changed to return an error if SETUP data was present,
and a new read_setup() was added that expects SETUP data not OUT data.
@ianrrees
Copy link
Contributor Author

@ithinuel - this is the PR discussed the other day in Matrix, I've put the corresponding atsamd-rs changes here, and test class implementations for SAMD21 and SAMD51 are here

@ianrrees
Copy link
Contributor Author

@9names ran some tests of this with Raspberry Pi 5 host and SAMD21, confirming the problem (enumeration being unreliable) and that this appears to resolve it.

While confirming the problem, 9names saw the new test case error out in an unexpected way - reporting Entity not found - I think that might require a bit more work to make the enumeration time test more reliable.

@ianrrees ianrrees changed the title Draft: Add new read_setup() for control endpoints Add new read_setup() for control endpoints Oct 12, 2024
@ianrrees
Copy link
Contributor Author

Sorry, I've not had an opportunity to look at the test code changes here, and probably won't be able to test Rust USB stuff until mid December at earliest. Since the test stuff isn't really critical, I'd like to proceed with this PR so that implementors can do their part.

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.

1 participant