Rework USB OUT endpoint interrupts #459
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ianrrees
added a commit
to ianrrees/atsamd-usbd-uart
that referenced
this pull request
Jun 24, 2021
ianrrees
changed the title
ianrrees
force-pushed
the
usb-transfer-fail
branch
from
166798f to
f242e68
Compare
ianrrees
commented
Jul 8, 2021
hal/src/thumbv6m/usb/bus.rs
Outdated
| @@ -995,6 +997,7 @@ impl Inner { | |||
| } | |||
| Err(err) => { | |||
| dbgprint!("UsbBus::read from ep {:?} -> {:?}\n", ep, err); | |||
| drop(bank); // bank holds a reference to self.desc, which print_epstatus() needs | |||
There was a problem hiding this comment.
Just noticed this might've broken isochronous transfers; I've got some other local changes that might also be where the problem comes from, but need to do some more digging.
This was the problem - previously this PR just deleted the drop(bank); above.
ianrrees
force-pushed
the
usb-transfer-fail
branch
from
f242e68 to
53ef89f
Compare
|
I've made a few changes from the original version of this PR, the main one is that the transfer complete and transfer failed flags are cleared in |
ianrrees
force-pushed
the
usb-transfer-fail
branch
from
53ef89f to
4a0fff5
Compare
ianrrees
changed the title
ianrrees
commented
Jul 15, 2021
ianrrees
force-pushed
the
usb-transfer-fail
branch
3 times, most recently
from
799dc17 to
ca977fd
Compare
ianrrees
commented
Jul 17, 2021
ianrrees
added a commit
to ianrrees/usb-spi
that referenced
this pull request
Jul 22, 2021
Has a little debug code left from atsamd-rs/atsamd#459
ianrrees
force-pushed
the
usb-transfer-fail
branch
from
49ba6e7 to
8227495
Compare
TDHolmes
reviewed
Mar 28, 2022
TDHolmes
reviewed
Mar 28, 2022
ianrrees
force-pushed
the
usb-transfer-fail
branch
from
8227495 to
0eb198a
Compare
Previously, when the USB interrupt was unmasked and a host issued an OUT transfer to an endpoint that was not `read()` before the next OUT transfer attempt to that endpoint, the USB ISR would continually fire until `read()` was called on that endpoint. If a higher priority ISR didn't somehow clear the condition that prevented the `read()`, the CPU would lock up. This change moves the clearing of the endpoint's transfer complete and transfer failed interrupt flags in to `poll(), so that a USB Class can safely skip a `read()` without having to chose between discarding data, or locking up in the USB ISR.
ianrrees
force-pushed
the
usb-transfer-fail
branch
from
0eb198a to
bbd02ec
Compare
rtsuk
pushed a commit
to rtsuk/atsamd
that referenced
this pull request
Mar 29, 2022
* Rework USB OUT endpoint interrupts Previously, when the USB interrupt was unmasked and a host issued an OUT transfer to an endpoint that was not `read()` before the next OUT transfer attempt to that endpoint, the USB ISR would continually fire until `read()` was called on that endpoint. If a higher priority ISR didn't somehow clear the condition that prevented the `read()`, the CPU would lock up. This change moves the clearing of the endpoint's transfer complete and transfer failed interrupt flags in to `poll(), so that a USB Class can safely skip a `read()` without having to chose between discarding data, or locking up in the USB ISR. * Optimisation in USB poll()
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(this PR has been heavily edited from the original - sorry)
Previously, when the USB interrupt was unmasked and a host issued an OUT transfer to an endpoint, the USB interrupt would continually fire until
read()was called on that endpoint. This could lead to a hang if the USB class controlling the endpoint was dependent on some lower-priority task before it could successfully read. For example: if the class received data over USB to feed out to an interrupt-driven serial port, the class might have a full buffer so not call the endpointread(), but the buffer would not drain because the serial port interrupt was lower priority than the USB one. This behaviour came about because the USBpoll()method didn't clear the endpoint interrupts for OUT transfers, but relied on theread()methods on endpoints to clear them.The endpoints have a non-interrupting flag that indicates when the endpoint is ready to be read, and this change switches to using that to indicate data is available.
One subtle consequence of this change is that if a USB class doesn't
read()a successful OUT transfer, the OUT data will sit idle indefinitely, until either it or the host transfers data. A simple workaround is to enable the SOF interrupts using #456 so thatpoll()gets called within the next millisecond.