Fix host issue with with turnaround (inter-packet) timing #179
Conversation
…after received data from device host: send keep-alive packet instead of SOF on lowspeed bus
hathach
force-pushed
the
fix-lowspeed-turraround
branch
from
1173833 to
791bce4
Compare
hathach
force-pushed
the
fix-lowspeed-turraround
branch
from
8c89c99 to
791bce4
Compare
|
PR is ready for review, @sekigon-gonnoc please let me know if the changes make sense to you. rp2350 still has difficulty detecting device disconnection, somehow it couldn't read SE0 on line state after device disconnected. I am still checking it, but we can fix it with another PR. This is crucial to get host working with more low speed device and/or overclock the rp2350. |
src/pio_usb.c
Outdated
| @@ -97,7 +97,7 @@ void __not_in_flash_func(pio_usb_bus_usb_transfer)(pio_port_t *pp, | |||
| continue; | |||
| } | |||
| pp->pio_usb_tx->irq = IRQ_TX_ALL_MASK; // clear complete flag | |||
| while (*pc < PIO_USB_TX_ENCODED_DATA_COMP) { | |||
| while (*pc <= PIO_USB_TX_ENCODED_DATA_COMP) { | |||
There was a problem hiding this comment.
This line is probably timing critical for RP2040/RP2350 without overclocking. Please consider controlling it with a compile-time option.
c1fea8c
There was a problem hiding this comment.
I found that this is required to work with some low speed device even e.g https://www.adafruit.com/product/6285 with standard 120Mhz espcially with rp2350. since it is M33 it runs much faster than rp2040 with the same clock. Some LS device is probably too slow without this. How about we only do this when the pp is running as low speed host only ?
There was a problem hiding this comment.
How about we only do this when the pp is running as low speed host only ?
Sounds good
There was a problem hiding this comment.
thank you, I push the update to only wait for complet EOP sent for low speed mode only, along with some description. I hope my comment make sense to you.
…evice disconnection
hathach
force-pushed
the
fix-lowspeed-turraround
branch
from
ca396a0 to
08a9b06
Compare
src/pio_usb.c
Outdated
| if (pp->low_speed) { | ||
| turnaround_in_cycle = 6 * pp->clk_div_ls_tx.div_int; // 1.5 bit time | ||
| } else { | ||
| turnaround_in_cycle = 4 * pp->clk_div_fs_tx.div_int; // 1 bit time |
There was a problem hiding this comment.
actually I am not sure if we should wait 1-bit time for FS, since it is rather short, maybe we can omit it ? I haven't got any issue with FS, but didn't do much testing. Maybe we can comment that out for now.
| pio_usb_bus_usb_transfer(pp, sof_packet_encoded, sof_packet_encoded_len); | ||
| } else { | ||
| // Send Keep alive for low speed | ||
| pio_usb_bus_usb_transfer(pp, keepalive_encoded, 1); |
There was a problem hiding this comment.
LS only regconize keep alive
| @@ -141,6 +141,18 @@ void pio_usb_bus_send_token(pio_port_t *pp, uint8_t token, uint8_t addr, | |||
|
|
|||
| static __always_inline port_pin_status_t | |||
| pio_usb_bus_get_line_state(root_port_t *root) { | |||
| #ifdef PICO_RP2350 | |||
There was a problem hiding this comment.
rp2350-E9 cause host failed to read SE0 when device unplugged (floating). It always read J-state. This workaround is needed.
| // detection and NRZI decoding but it is unlikely. | ||
| // Exit since we've gotten an EOP. | ||
| // Timing critical: per USB specs, handshake must be sent within 2-7 bit-time strictly | ||
| if (pp->low_speed) { |
There was a problem hiding this comment.
as with the pio_usb_bus_usb_transfer(), I think for FS, the overhead is probably more than 2-bit time and additional delay is not needed. We can enable this later if having issue with FS on highly overclocked cpu later on :)
| gpio_set_input_enabled(root->pin_dm, false); | ||
|
|
||
| // short delay to drain leaked current, required when overclocked CPU, tested with 264Mhz | ||
| __asm volatile("nop; nop; nop; nop; nop; nop; nop; nop;"); |
There was a problem hiding this comment.
added additional delay, needed when testing with the 264Mhz overlocked rp2350
|
@sekigon-gonnoc I have update the PR to only add inter-packet delay for low speed only, and add delay for E4 to drain leaked current even with 264Mhz rp2350. I think the PR is good for merge. let me know if you liek to make any other changes :) |
| while (*pc <= PIO_USB_TX_ENCODED_DATA_COMP) { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
if (pp->low_speed) {
while (*pc <= PIO_USB_TX_ENCODED_DATA_COMP) {
continue;
}
} else {
while (*pc < PIO_USB_TX_ENCODED_DATA_COMP) {
continue;
}
}
This change may have less impact for the FS device
There was a problem hiding this comment.
right, it looks better as well.
|
thank you very much |
USB 2.0 specs 7.1.18 specify 2 bit time for inter-packet, especially when switching bus direction.
This is essential when we overlock mcu to higher clock such as 240/264 Mhz. We basically send handshake too fast after EOP and give device no time to capture the bus correcly. This also occurs with some slow device such as LS gamepad in adafruit/circuitpython#10243
turnarounddelay is done in cycle which is conveniently = 4 pp->clk_div_/fsls_tx.div_int. Since we also have overhead, I think it is best to do 1.5 bit time for LS and 1 bit time for FS.@tannewt @FoamyGuy @ladyada Please try to see if this fixes your issue with gamepad (direct attached) and 264Mhz overclocked with hub + mouse + keyboard on fruit jam
PS: Also include the
fixworkaround for the famous RP2350-E9. all rp2350 A2/B0 cannot reliably read gpio when floating. This is the root cause that pio host cannot detect the device disconnection ((floating), when it tries to read line state, it is always get J state instead of SE0.