rp2: Use local tinyusb submodule rather than pico-sdk one

[dpgeorge]

So that all MicroPython ports that use tinyusb use the same version. Also requires fewer submodule checkouts when building rp2 along with other ports that use tinyusb.

TODO: check that other ports using tinyusb still work with this version.

[tannewt]

We're on https://github.com/hathach/tinyusb/tree/045674745afa59028fbeed6dac5cb5a9c4a6033e and haven't heard of any issues across our ports.

[lurch]

rp2 port / build (pull_request) Failing after 1m — build

The error is

/home/runner/work/micropython/micropython/ports/rp2/tusb_port.c:27:18: fatal error: tusb.h: No such file or directory
 #include "tusb.h"

so I guess you need to tweak the include flags passed to the rp2 port?

[robert-hh]

I had that error too initially. You have to update the submodules for tinyusb and pico-sdk with the --recursive flag.
git submodule update --recursive tinyusb pico-sdk
But that should be better with the recent update of the repository.

[lurch]

You have to update the submodules for tinyusb and pico-sdk with the --recursive flag.

I've had bad experiences with recursively updating submodules - it seems to pull in much more than you actually need and just generally slows things down. IMHO in this case it's better to be explicit (choose which specific submodules to update) than implicit (recursively updating all submodules)!

[lurch]

I guess you need to tweak the include flags passed to the rp2 port?

Looks like I was a bit too hasty with my reply last night (oops) - the include flags are of course managed by CMake. Looking again at the build log, the actual cause of the error is actually displayed slightly earlier:

CMake Warning at /home/runner/work/micropython/micropython/lib/pico-sdk/src/rp2_common/tinyusb/CMakeLists.txt:14 (message):
  PICO_TINYUSB_PATH specified but content not present.

I'm afraid I dunno enough about CMake and/or the MicroPython build environment to suggest a suitable fix. But I can confirm that a local build of pico-sdk definitely does work with an out-of-tree tinyusb repo with a correctly-set PICO_TINYUSB_PATH:

TinyUSB available at /home/andrew.scheller/tinyusb/src/portable/raspberrypi/rp2040; adding USB support.

[dpgeorge]

I'm afraid I dunno enough about CMake and/or the MicroPython build environment to suggest a suitable fix.

Actually all it needed was for the CI environment to check out tinyusb!

[dpgeorge]

I've now updated all the ports that use tinyusb (nrf, samd, mimxrt) to build with this new version.

[dpgeorge]

I've now updated all the ports that use tinyusb (nrf, samd, mimxrt) to build with this new version.

The problem is, these other ports don't work reliably with tinyusb and updating tinyusb seems to make it worse (at least for mimxrt). These other ports call tud_task() from IRQ context which is apparently not allowed within tinyusb's execution model. So they really need to be fixed first.

[dpgeorge]

These other ports call tud_task() from IRQ context which is apparently not allowed within tinyusb's execution model

@hathach can you please confirm if this is indeed the case, that we must call tud_task() from a thread (top level) execution context, ie never from within the USB IRQ handler? What are the constraints around calling tud_task()?

[tannewt]

Thach is taking vacation this week for the Chinese New Year I believe. In cp we’ve always called tud_task from the main task. Generally the usb interrupt handler queues events into the fifo so they are processed on the main task. Calling tud_task from the irq could cause issues if the task takes a while and the interrupt causes events to be missed and not queued up.

…

On Mon, Feb 8, 2021, at 9:26 PM, Damien George wrote: > These other ports call `tud_task()` from IRQ context which is apparently not allowed within tinyusb's execution model @hathach <https://github.com/hathach> can you please confirm if this is indeed the case, that we must call `tud_task()` from a thread (top level) execution context, ie never from within the USB IRQ handler? What are the constraints around calling `tud_task()`? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6835 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAM3KPDK4HBYXQAFJWHZ43S6DBRLANCNFSM4XAGTBGQ>.

[hathach]

These other ports call tud_task() from IRQ context which is apparently not allowed within tinyusb's execution model

@hathach can you please confirm if this is indeed the case, that we must call tud_task() from a thread (top level) execution context, ie never from within the USB IRQ handler? What are the constraints around calling tud_task()?

Yeah, you should not call tud_task() within ISR context. It is much better to call it from thread-context. tud_task() will perform all the processing including calling class driver callback, such as msc read/write which can do I/O op which cause huge latency when invoked within ISR().

Sum up please call tud_int_handler() in ISR and tud_task() in main/background

[hathach]

Thach is taking vacation this week for the Chinese New Year I believe. In cp we’ve always called tud_task from the main task. Generally the usb interrupt handler queues events into the fifo so they are processed on the main task. Calling tud_task from the irq could cause issues if the task takes a while and the interrupt causes events to be missed and not queued up.

Thank Scott, today is my last working day before the holiday, though I probably still be around whenever I could :)

[dpgeorge]

Yeah, you should not call tud_task() within ISR context. It is much better to call it from thread-context.

It may be much better, but is it absolutely necessary to call tud_task() from thread context?

tud_task() will perform all the processing including calling class driver callback, such as msc read/write which can do I/O op which cause huge latency when invoked within ISR().

Does it matter if there is a large latency within the USB IRQ handler? As long as the USB IRQ is low priority (but still higher than thread context) it shouldn't matter, the hardware should block any IRQs from coming in until the handler is done.

On stm32 (where we use ST's USB device driver) we process all USB code within USB IRQs, including MSC operations that call disk IO. This has never been a problem. The latency in the IRQ is actually good because it introduces flow control with the host PC (the PC's USB is blocked until the MSC operation completes, for example).

From another point of view, what if the tud_int_handler() call takes a little bit too long, won't that then introduce a problem just like calling tud_task() would?

[hathach]

It may be much better, but is it absolutely necessary to call tud_task() from thread context?

No, it isn't absolutely necessary, at least I cannot think of any reasons, though I haven't tested it invoked with ISR context. So I may miss something. Could you tell what is the reliability issue you are dealing with and how to reproduce it, I will try to analyze to see what is the underlying issue.

Does it matter if there is a large latency within the USB IRQ handler? As long as the USB IRQ is low priority (but still higher than thread context) it shouldn't matter, the hardware should block any IRQs from coming in until the handler is done.

On stm32 (where we use ST's USB device driver) we process all USB code within USB IRQs, including MSC operations that call disk IO. This has never been a problem. The latency in the IRQ is actually good because it introduces flow control with the host PC (the PC's USB is blocked until the MSC operation completes, for example).

Yeah, I guess it depends on design choice and execution. Some RTOS like FreeRTOS does not like to run on ISR context, they have separated API for thread and ISR. But since MP doesn't use RTOS, it may not be an issue.

From another point of view, what if the tud_int_handler() call takes a little bit too long, won't that then introduce a problem just like calling tud_task() would?

tud_int_handler() doesn't do any high level works, it mostly take note and clear the usb event from registers such bus_reset, setup received, data complete and submit the event to the event queue. All the processing is later done by tud_task().

[dpgeorge]

No, it isn't absolutely necessary, at least I cannot think of any reasons, though I haven't tested it invoked with ISR context.

Ok, thanks for the reply. I will do some more investigation on this.

Could you tell what is the reliability issue you are dealing with and how to reproduce it, I will try to analyze to see what is the underlying issue

I don't have a minimal reproduction of the problem. But you can just build the mimxrt port and run it against the test suite, on a Teensy 4.0:

$ cd ports/mimxrt
$ make
$ make deploy
$ cd ../../tests
$ ./run-tests --target minimal --device /dev/ttyACM0 -d basics

That last line will run the test suite, and usually locks up the device part way through.

[hathach]

thank you for your instruction, I am able to reproduce the issue, with imxrt, REPL doesn't seem to work much at all. I will try my best to look at the issue and post the update here.

[hathach]

Update: I couldn't do much of debugging since somehow the imxrt board manage to cause my PC (ubuntu) to disable all usb port when testing the issue. I will spend more time on this later on. Though I have an theory why it could cause the issue.

• tud_init_hanlder() read and clear register that cause the IRQ then push event to the queue
• tud_task() pull event from the queue and processed it, then calling transfer API. And USB HSPHY of imxrt is actually fast enough to complete the physical usb transfer before the tud_task() could return, then set the register flag for it. Which I have experience with Fix endpoint busy flag race condition hathach/tinyusb#383. Although we don't have rtos to preempt as the issue, however since we are already in USB IRQ, hardware may not trigger the IRQ calls after tud_task() return. Which cause the event completed while in ISR not handled at all --> causing irresponding behavior.

To be honest, I am not sure if IRQ will be triggered again or not (may depend on MCU), and not sure why it isn't a issue before this PR. But is is quite possible. I will try to have a better test setup with better analysis during/after the Lunar New Year (aka TET).

[dhalbert]

Copy-paste two comments, edited slightly, from #6866:

We had a similar-sounding problem [to #6866] in CircuitPython recently. Copying multiple large files to the MSC drive would stall, then there would be a 30-second timeout, and USB would re-enumerate. The problem did not occur if there was a CDC REPL connection open to the board, even if there was no activity on that connection.

The problem was that tud_task() was not being scheduled to be called. We used to schedule a call ro tud_task() on every 1ms tick . Usually there was no work to do, but it got called promptly all the time. (It's cheap to call.) Then we started enqueuing an event to call it only after servicing a USB interrupt. If we were idle, we would do WFI to save power and just wait for an interrupt to trigger more work. This scheme worked most of the time, but we missed scheduling a tud_task() call in a particular circumstance. The details are not really relevant to MicroPython because they have to do with our auto-reload mechanism. But they are related to scheduling tud_task() properly when there is work to do, as is being discussed here.

In CircuitPython 5.x and before, we used to schedule a call to tud_task() on every 1ms tick. We did not call it from the IRQ handler. We use MICROPY_VM_HOOK_LOOP to run background tasks, and running tud_task() (and many other background tasks) happened if 1ms had passed.

We subsequently revised the mechanism so that not every background task was called when 1ms had passed. Instead the tasks are enqueued when there is work to do. We no longer have a 1ms tick scheduling the overall set of background task. We still use MICROPY_VM_HOOK_LOOP (renamed to RUN_BACKGROUND_TASKS) and it does the queue checking, but only runs what is enqueued.

[dpgeorge]

I tried to fix mimxrt and samd USB behaviour (with the older tinyusb, not this PR) but ran in to other issues (like the fact that i.mx SysTick cannot wake the CPU from WFI). So I propose to just merge this PR to get the latest tinyusb for all ports, then move on to fix things from there.

[hathach]

Yeah, systick behaves the same with nrf as well and Nordic advises user to use rtc1 as tick timer instead for low power. My guess is that systick run on cpu clock which got suspended by wfi/wfe, this may (or not) be true for many of arm coterx-m.