tud_task() inhibits wfe on rp2350, turns low power loops into busy loops

[rlcamp]

The common pattern of

while (!condition) {
    tud_task();
    __wfe();
}

which should put the processor to sleep between interrupts, becomes a busy loop by default on RP2350 due to the use of pico-sdk spinlocks within tud_task(), which behave like sev() due to reasons discussed in https://github.com/raspberrypi/pico-sdk/issues/1812. Mitigations include disabling EXTEXCALL (which modifies the behaviour of ALL atomics and other exclusive load/stores between cores), or putting

#undef CFG_TUSB_OS
#define CFG_TUSB_OS OPT_OS_NONE

in tusb_config.h, disabling the pico-specific code paths in the TinyUSB OS abstraction layer. The latter is less of a big hammer but still has consequences wr interacting with TinyUSB from both cores.

[peterharperuk]

Maybe you should do something like this instead?

    absolute_time_t timeout_time = make_timeout_time_ms(10);
    do {
      if (tud_task_event_ready()) break;
    } while (!best_effort_wfe_or_timeout(timeout_time));
    tud_task(); // tinyusb device task

[rlcamp]

if (tud_task_event_ready()) break;

This could work, but code comments within the function it calls indicate that it's meant to be called with interrupts disabled, due to not being protected by a locking primitive. It's the lack of being protected by a locking primitive that means there is no problematic implicit sev, but I'm not sure whether it's actually safe in all cases.

If it is indeed safe, then it should be possible to modify tud_task() itself so that the original idiomatic usage of it is low power, as intended.

[kilograham]

added, then deleted my totally incorrect comment

[kilograham]

Ok, trying again:

    while (!condition) {
        tud_task();
        // do these at the end of tud_task() on RP2350 to consume the event (I'd suggest putting them in there)
        __sev();
        __wfe();
        // require tud_task_event_ready() to avoid getting stuck if another IRQ happened prior to the above `__wfe()`
        if (!tud_task_event_ready()) {
            __wfe();
        }
    }

Note we can't really do __sev(); __wfe() in the spin lock code, as eating events is certainly a non-desirable side effect in many cases.

However I'd suggest that someone calling tud_task() should not reasonably expect any guarantees related to processor events. For this reason I'd suggest that calling tud_task() followed by __wfe() is a poor idiom in general, that has just happened to work up until now.

arguably there should be a tud_block_until_event() or similar

note also that tud_task_event_ready() should probably be documented as safe to call without holding IRQs. it isn't guaranteed to be correct in that case, but it won't crash. The races are safe:

• If it says there is an event when there isn't due to a race, we'll loop once more
• If it says there is no event when there is, then there must have been an IRQ in the middle of the call, in which case the processor event will be set

[kilograham]

@rlcamp could you try my suggested fix

[rlcamp]

Willing to try things, but what is your suggested fix? Adding a sev wfe to consume the event is, as you point out above, a non starter.

[kilograham]

The code at the top of https://github.com/raspberrypi/pico-sdk/issues/2495#issuecomment-3067323222 - i was suggesting you CAN reasonably do a SEV/WFE here

[rlcamp]

i was suggesting you CAN reasonably do a SEV/WFE here

I don't think you can. In any code that is trying to sleep until either TinyUSB or unrelated things wake it up, it would be very bad for the calling code to reset the ER under the assumption that tud_task is the only thing that set it. That's certainly the case for the code I was prototyping when I ran into this.

For this reason I'd suggest that calling tud_task() followed by __wfe() is a poor idiom in general, that has just happened to work up until now.

The original intention seems to be that this usage should be okay:

You can put it in the main() loop and call __WFE at the end of the loop. USB event will wake up mcu, push event to the queue. And the task will process it and then go to sleep afterward with next wfe.

Originally posted by @hathach in #52

However, tud_task_event_ready() did not yet exist when the above comment was written, so it seems like the future-proof solution is to chase down loops in the wild that contain both an unguarded tud_task() and __wfe() and add the necessary tud_task_event_ready() guard. I'm still not sure this makes it safe to interact with TinyUSB from both cores, if one had a reason to do so.

[kilograham]

Yeah, as i say any code relying on the CPU event side effects (present or not) of tud_task() is probably built on shaky foundations.

If you need to sleep waiting on one of multiple conditions to be true, you should probably use an SDK semaphore. As of last checking TinyUSB does not provide a hook where you could set the semaphore, however the code in pico_stdio_usb https://github.com/raspberrypi/pico-sdk/blob/9a4113fbbae65ee82d8cd6537963bc3d3b14bcca/src/rp2_common/pico_stdio_usb/stdio_usb.c#L221 adds a shared IRQ handler to catch possible work required situations, and you could do the same thing

[kilograham]

moving to 2.3.0 as there will be no SDK change for 2.2.0 - it is still worth working towards a TinyUSB based solution

[rlcamp]

towards a TinyUSB based solution

One possible TinyUSB based solution would simply to be to factor out the use of critical_section_t within osal_pico.h and document that the calls to tud_task must be performed by the same core as the interrupt handler (which the SDK can enforce, in its own usage of TinyUSB), thereby eliminating the original issue?

[kilograham]

Yeah, another potential solution is to use an async_context for all tinyusb stuff, which is the direction pico_stdio_usb ought to go; just happens to predate the async_context code. The async_context can make sure the calls are all made from the same place, and is also design to wake up only when there is work to do