fix(rp2040): replace loop counter with hw timer for USB SetAddressReq… #4796
Conversation
|
Hello @rdon-key thank you for your PR. Looks like you need to run Unformatted:
src/machine/machine_rp2040_usb.go
make: *** [GNUmakefile:185: fmt-check] Error 1Can you please correct that and then rebase those changes into this PR? Thanks! |
rdon-key
force-pushed
the
fix/usb-set-address-hw-timer
branch
from
97cdc50 to
bc7d82b
Compare
|
@deadprogram I've fixed the formatting issue and rebased the changes into a single commit. I ran |
src/machine/machine_rp2040_usb.go
Outdated
| // last, set the device address to that requested by host | ||
| // wait for transfer to complete | ||
| timeout := 3000 | ||
| rp.USBCTRL_REGS.SIE_STATUS.Set(rp.USBCTRL_REGS_SIE_STATUS_ACK_REC) | ||
| start := rp.TIMER.TIMERAWL.Get() |
There was a problem hiding this comment.
For portability to rp2350, I suggest using the global variable machine.timer and adding a method for short delays.
There was a problem hiding this comment.
@eliasnaur Thank you for your review. Based on your suggestion, I've added the waitUntil function to machine.timer. Looking forward to your further feedback.
There was a problem hiding this comment.
@eliasnaur
As you can see, the implementation successfully handles the SET ADDRESS Request and proceeds with enumeration on rp2350 as well.
| No. | Time | Source | Destination | Protocol | Length | Info |
|---|---|---|---|---|---|---|
| 876 | 5.087713700 | host | broadcast | USBLL | 3 | SOF |
| 877 | 5.088171533 | host | 0.0 | USBLL | 3 | SETUP |
| 878 | 5.088174783 | host | 0.0.0 | USB | 11 | SET ADDRESS Request |
| 879 | 5.088183450 | 0.0 | host | USBLL | 1 | ACK |
| 880 | 5.088713700 | host | broadcast | USBLL | 3 | SOF |
| 881 | 5.088720116 | host | 0.0 | USBLL | 3 | IN |
| 882 | 5.088723450 | 0.0 | host | USBLL | 3 | DATA1 |
| 883 | 5.088726783 | host | 0.0 | USBLL | 1 | ACK |
| 884 | 5.089713700 | host | broadcast | USBLL | 3 | SOF |
| 885 | 5.090713700 | host | broadcast | USBLL | 3 | SOF |
| 886 | 5.091713700 | host | broadcast | USBLL | 3 | SOF |
| 887 | 5.092713700 | host | broadcast | USBLL | 3 | SOF |
| 888 | 5.093713700 | host | broadcast | USBLL | 3 | SOF |
| 889 | 5.094713700 | host | broadcast | USBLL | 3 | SOF |
| 890 | 5.095713700 | host | broadcast | USBLL | 3 | SOF |
| 891 | 5.096713700 | host | broadcast | USBLL | 3 | SOF |
| 892 | 5.097713700 | host | broadcast | USBLL | 3 | SOF |
| 893 | 5.098713700 | host | broadcast | USBLL | 3 | SOF |
| 894 | 5.098936033 | host | 1.0 | USBLL | 3 | SETUP |
| 895 | 5.098939283 | host | 0.1.0 | USB | 11 | GET DESCRIPTOR Request DEVICE |
| 896 | 5.098947950 | 1.0 | host | USBLL | 1 | ACK |
| 897 | 5.099713700 | host | broadcast | USBLL | 3 | SOF |
| 898 | 5.099720116 | host | 1.0 | USBLL | 3 | IN |
| 899 | 5.099723450 | 0.1.0 | host | USB | 21 | GET DESCRIPTOR Response DEVICE |
| 900 | 5.099738783 | host | 1.0 | USBLL | 1 | ACK |
| 901 | 5.100713616 | host | broadcast | USBLL | 3 | SOF |
| 902 | 5.100720033 | host | 1.0 | USBLL | 3 | OUT |
| 903 | 5.100723283 | host | 1.0 | USBLL | 3 | DATA1 |
| 904 | 5.100726700 | 1.0 | host | USBLL | 1 | ACK |
rdon-key
force-pushed
the
fix/usb-set-address-hw-timer
branch
from
4328d19 to
6ff0baf
Compare
rdon-key
force-pushed
the
fix/usb-set-address-hw-timer
branch
from
6ff0baf to
8135eb3
Compare
|
You can safely rebase, so please rebase onto the latest dev branch as the target. @rdon-key |
|
It’s difficult to determine whether 570us is truly the correct value. However, on RP2040—or more specifically, RP2350—the timeout might be too short. Also, simply using a counter-based wait can cause issues when overclocking, so using a timer-based wait is a good approach. At this point, I believe this PR improves the situation, so I’m in favor of it. |
…ressRequest timeout.
rdon-key
force-pushed
the
fix/usb-set-address-hw-timer
branch
from
74ae3bc to
ebe7781
Compare
|
Now squash/merging. Thanks for working on this @rdon-key and to @sago35 @eliasnaur for review. |
|
@deadprogram @sago35 @eliasnaur |
Replacing Simple Loop Counter with Hardware Timer for USB SetAddressRequest Timeout to Improve Stability
Overview
This pull request addresses Issue #4137, which involves USB enumeration issues on RP2040 devices.
Problem
The previous implementation relied on a simple loop counter to determine the timeout for SetAddressRequest. If SetAddressRequest fails, subsequent USB enumeration will not function correctly. This approach faces challenges due to environment-dependent timing variations caused by factors such as:
Solution
This PR proposes replacing the timeout mechanism from a simple loop counter to a hardware timer. This ensures a precise and consistent timeout duration, eliminating variations across different environments.
Observations
Benefits
Test Results
Three test scenarios were observed with packet captures shown below:
Packet Capture Comparison
1. Failure (AMD PC)
2. Success (Intel PC)
3. Success (AMD PC with USB hub)
These packet captures demonstrate how the implementation with a simple loop counter affects USB communication sequences across different platforms.
After Implementation with Hardware Timer
After implementing the hardware timer solution, all platforms showed successful USB enumeration:
1. Success (Intel PC with hardware timer)
2. Success (AMD PC with USB hub with hardware timer)
3. Success (AMD PC with hardware timer)
The most significant improvement is seen in the third scenario (AMD PC), which previously failed with the loop counter implementation but now successfully completes the SetAddressRequest and proceeds with enumeration when using the hardware timer.
These results conclusively demonstrate that the hardware timer implementation provides consistent behavior across all tested platforms, eliminating the timing variability issues that previously caused failures on certain systems.