[EV3] Fix more USB corner cases and minor bugs #359
Conversation
|
@dlech @BertLindeman can you test this? (potential fix for EV3 enumeration issues) |
|
I tried it but didn't see any change. If anything it might even be worse as I never saw it enumerate even the string descriptors even after multiple tries/reboots. USB 3.0 hosts still work though. |
|
Hrm okay. Can you get a new packet capture of an unsuccessful enumeration? |
|
I do see improvement: linux usb-3: windows 11 My eye are getting smaller. |
Here you go: ev3-usb2.0-no-work.zip It is device 3.0. You can see that it just never responds to the GET DESCRIPTOR Request. |
ArcaneNibble
changed the title
|
I might've figured it out. @dlech can you test again? If this works, I still want to merge all the intermediate miscellaneous fixes. They don't visibly do anything, but they're still "more correct" |
|
It works! I didn't do super-thorough testing but it at least is working much better than before. |
|
USB-2 No longer error messages on plug/unplug. dmesg outputUSB-3 Did not flash on this pc. dmesg outputwindows |
|
Nice, looks like we can merge this. |
Regarding the last commit in the seriesThe However, clearing
By setting them separately like we were doing before, if the host controller completed the status phase while we were still running through the IRQ handler, the USB IP would detect SetupEnd, we would interpret it as an error, and then we would fail to act on the We now set the bits at the exact same time, so this race window is closed. This race window does not apply to requests with a data stage due to the extra packets which are expected. Regarding the first commit in the seriesIf a host controller is fast enough, it can turn around from getting a STALL during a control transaction to immediately issuing the next setup stage before the interrupt handler ever gets invoked. This can result in multiple status flags getting set (both the Because this is timing sensitive, it makes sense why, without the last commit in the series, it helped on Bert's machine but made David's machine worse. Having both fixes in place should work reliably with all ranges of timing. I don't know for sure, but I am speculating that xHCI doesn't manifest this bug because it is slower than EHCI at turning around basic operations like this if the host isn't intentionally trying to maximize the bus usage (which it won't be during initialization and enumeration) |
|
To complete the EV3 USB test, I found a keyboard with a low-power USB hub. Did this test with firmware at git hash ffb2184 The hub is Test with the EV3 in dmesg: Part of a [EDIT] tracebackCan create a new issue if David thinks it is worth it. |
This sounds like info that would be useful to add to the commit messages.
No, there isn't anything we can do about firmware flashing and USB 1.1. |
There is no guarantee that these events can only happen one at a time. If a host controller is fast enough, it can turn around from getting a STALL during a control transaction to immediately issuing the next setup stage before the interrupt handler ever gets invoked. This can result in multiple status flags getting set (both the SENTSTALL confirmation bit and RXPKTRDY for the new request). If we happen to miss an event, it may cause the USB control transfer state machine to fall out of sync, which can cause enumeration failures. This commit depends on a subsequent fix to work across all ranges of timings.
This is required by the USB specification.
Although this does not visibly change anything, the AM1808 errata document claims that not doing this sequence can theoretically result in a timing violation inside the logic, which can cause unpredictable results. Since it is simple, we can just apply the recommended workaorund sequence.
The SETUPEND bit in the PERI_CSR0 seems to do _exactly_ what it says. The bit gets set if a control transaction ends (either by receiving a new SETUP token, or by completing the status phase with an ACK) before the DATAEND bit is set. However, clearing RXPKTRDY seems to be the only flag needed before the MUSB IP moves on to automatically allowing the status phase to complete. The datasheet hints at this by saying: > The interval between setting SERV_RXPKTRDY bit and DATAEND bit > should be very small to avoid getting a SetupEnd error condition. By setting the bits separately like we were doing before, if the host controller completed the status phase while we were still running through the IRQ handler, the USB IP would detect SetupEnd, we would interpret it as an error, and then we would fail to act on, in particular, the SET_ADDRESS command. This manifested in the device suddenly no longer responding to subsequent GET_DESCRIPTORs. We now set the bits at the exact same time, so this race window is closed. This race window does not apply to requests with a data stage due to the extra packets which are expected.
|
Just added the long explanations into the commit messages. |
|
Thanks! |
4 participants
There is no guarantee that these events can only happen one at a time. If we happen to miss an event, it may cause the USB control transfer state machine to fall out of sync, which can cause enumeration failures.
This might fix pybricks/support#2297
Note that the bulk of the diff is indentation changes.