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

RP2040 HCD driver does not correctly recover from data sequence errors per USB Spec 8.6.4 #2776

Open
1 task done
rppicomidi opened this issue Aug 26, 2024 · 1 comment
Open
1 task done
Labels

Comments

@rppicomidi
Copy link
Contributor

Operating System

Linux

Board

Raspberry Pi Pico

Firmware

usb_midi_host_example

What happened ?

This is with regards the RP2040 USB Host mode chip bug described here: raspberrypi/pico-feedback#394.

TinyUSB implements host mode bulk and interrupt endpoints using the automatically polled endpoint hardware on the RP2040. That hardware has a bug that prevents it from recovering from data sequence errors per the USB specification section 8.6.4. Normally, data sequence errors due the device missing ACK are rare unless the environment is noisy or the hardware is bad. However, there are commercial devices with at least one IN endpoint where the first Bulk IN or Interrupt IN transfer starts with DATA1 PID instead of DATA0. Either way, any data sequence error freezes up the host hardware for that endpoint in an infinite loop instead of replying ACK and discarding the data. The RP2040 has no way to detect this condition and recover from it.

The right thing to do is to rewrite the RP2040 HCD to use only EPx. At least that endpoint hardware has a way to detect data sequence errors and should actually implement USB specification 8.6.4 correctly. However, rewriting the HCD is a relatively long task. The new HCD has to route all Bulk IN and and Interrupt IN transfers through EPx to avoid the hardware bug. Once we are doing that, it seems that we should try to improve all Bulk and Interrupt transfer throughput by managing transfers in software.

For a quicker solution, I have a patch to TinyUSB that allows an application to tell the RP2040 HCD about specific device IN endpoints on devices with a given VID:PID that are known to always start the first Bulk IN or Interrupt IN transfer with DATA1 PID. The patch changes the expected first PID to DATA1 instead of DATA0 when the HCD opens the known faulty endpoint. This patch is known to work: See rppicomidi/usb_midi_host#14 near comments 40.

@hathach, if I submit this patch as a pull request, would you consider it, or is it too much of a hack? In my mind, it should be a temporary solution. It requires applications to have a list of commercial devices that are known to be buggy. The patch still does not allow the RP2040 USB Host mode to recover from a data sequence error due to noise. It is useful as a temporary fix until the HCD gets a rewrite, however.

How to reproduce ?

See the discussion here: https://github.com/raspberrypi/pico-feedback/

Debug Log as txt file (LOG/CFG_TUSB_DEBUG=2)

N/A

Screenshots

N/A

I have checked existing issues, dicussion and documentation

  • I confirm I have checked existing issues, dicussion and documentation.
Repository owner deleted a comment Aug 26, 2024
Repository owner deleted a comment Aug 26, 2024
Repository owner deleted a comment Aug 26, 2024
@rppicomidi
Copy link
Contributor Author

@hathach Really fixing this bug requires writing a new HCD for the RP2040 hardware that only uses epx hardware. These are my thoughts on the design. Do you have any comments or suggestions?

  • The interrupt handler must schedule every pending transfer
  • To do that, the interrupt handler must service SOF interrupts to properly allocate bandwidth per frame
  • The interrupt handler must service NAK interrupts to know when to try a different pending transfer
  • The HCD cannot rely on the automatic retry after NAK feature of the RP2040 hardware. However, the NAK_POLL feature cannot be disabled, even by setting NAK_POLL interval to 0 or to the maximum value (I tried both). Therefore, it must set NAK_POLL to the maximum value, which is 1023 microseconds, and the interrupt handler must abort the current transfer when it handles the NAK interrupt. It has about 1ms to handle the NAK interrupt and abort the pending transfer before the automatic retry mechanism will start another transfer.
  • The interrupt handler scheduler will maintain a list of pending transfers. When the handler gets SOF or a NAK interrupt, it will attempt the next pending transfer. The scheduler will prioritize reserved bandwidth pending transfers (interrupt endpoints), then control transfers, then bulk. Support of isochronous transfers is a future feature.

This design will greatly increase interrupt frequency because it handles the SOF interrupt and will increase it more because it handles every NAK. Hopefully, the RP2040 can handle interrupts fast enough that this change will improve performance of bulk transfers.

rppicomidi added a commit to rppicomidi/tinyusb that referenced this issue Sep 24, 2024
This commit fixes issue hathach#2776
hathach#2776.
The HCD now routes all USB transfers through the EPx
endpoint. It no longer uses the "interrupt" endpoint
hardware to handle INTERRUPT and BULK endpoints. The
fix avoids the data sequence error handling bug in
the RP2040 USB IP's "interrupt" endpoint hardware
and allows the host to correctly drop the IN packet
with the error without locking up. That fixes
rppicomidi/usb_midi_host#14

This fix requires the CPU to handle the SOF interrupt
(every ms). That might be an issue for some systems.

A benefit of this fix is that BULK transfers are
more than 2x faster. There is an opportunity to speed
them up further by forcing BULK transfer to begin
immediately instead of waiting for the next SOF interrupt.

This code has been tested with MSC flash drives, HID
devices, and USB Hubs. It works with full speed and low
speed HID devices connected at the same time through a hub.
With the usb_midi_host application host driver, 4 MIDI
devices plugged to a hub can send messages to each other
(see the midi2usbhub project).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant