esp32/usb: Wake main thread when usb receives data. #12845
Conversation
|
This is a very good improvement! |
ports/esp32/usb.c
Outdated
| @@ -58,6 +59,7 @@ static void usb_callback_rx(int itf, cdcacm_event_t *event) { | |||
| ringbuf_put(&stdin_ringbuf, usb_rx_buf[i]); | |||
| } | |||
| } | |||
| mp_hal_wake_main_task_from_isr(); | |||
There was a problem hiding this comment.
Is this USB callback actually an ISR? Or should we instead be using vTaskNotifyGive(mp_main_task_handle) instead?
There was a problem hiding this comment.
Good question! I've tried to trace it through the code path in micropython/ports/esp32/managed_components/espressif__tinyusb/ - this function is called indictly by TinyUSB's "standard" tud_cdc_rx_cb callback which eventually traces back up to the top level TinyUSB task tud_task_ext() / tud_task() - so, no, it's not an IRQ it's running directly from a separate FreeRTOS Task.
edit: missed the important not above
There was a problem hiding this comment.
Just pushed the change, xTaskNotifyGive(mp_main_task_handle); does provide the same speed up thanks!
There was a problem hiding this comment.
which eventually traces back up to the top level TinyUSB task tud_task_ext() / tud_task() - so, no, it's an IRQ it's running directly from a separate FreeRTOS Task.
So is it an IRQ, or is it running at thread-level on a separate task? I'm guessing the latter.
There was a problem hiding this comment.
thread-level on a separate task.
ports/esp32/managed_components/espressif__esp_tinyusb_mod/tusb_tasks.c
/**
* @brief This top level thread processes all usb events and invokes callbacks
*/
static void tusb_device_task(void *arg)
{
ESP_LOGD(TAG, "tinyusb task started");
while (1) { // RTOS forever loop
tud_task();
}
}
esp_err_t tusb_run_task(void)
{
// This function is not garanteed to be thread safe, if invoked multiple times without calling `tusb_stop_task`, will cause memory leak
// doing a sanity check anyway
ESP_RETURN_ON_FALSE(!s_tusb_tskh, ESP_ERR_INVALID_STATE, TAG, "TinyUSB main task already started");
// Create a task for tinyusb device stack:
xTaskCreate(tusb_device_task, "TinyUSB", CONFIG_TINYUSB_TASK_STACK_SIZE, NULL, CONFIG_TINYUSB_TASK_PRIORITY, &s_tusb_tskh);
ESP_RETURN_ON_FALSE(s_tusb_tskh, ESP_FAIL, TAG, "create TinyUSB main task failed");
return ESP_OK;
}There was a problem hiding this comment.
OK, thanks for clarifying.
(I'm surprised it's a simple while-1 loop... hopefully tud_task() is a blocking call that waits for events on a FreeRTOS queue.)
0bcd3c5 to
642affc
Compare
ports/esp32/usb.c
Outdated
| @@ -58,6 +59,7 @@ static void usb_callback_rx(int itf, cdcacm_event_t *event) { | |||
| ringbuf_put(&stdin_ringbuf, usb_rx_buf[i]); | |||
| } | |||
| } | |||
| xTaskNotifyGive(mp_main_task_handle); | |||
There was a problem hiding this comment.
I think it would be good to put this in a function called mp_hal_wake_main_task().
| @@ -41,7 +41,8 @@ | |||
| static uint8_t usb_rx_buf[CONFIG_TINYUSB_CDC_RX_BUFSIZE]; | |||
|
|
|||
| static void usb_callback_rx(int itf, cdcacm_event_t *event) { | |||
| // TODO: what happens if more chars come in during this function, are they lost? | |||
There was a problem hiding this comment.
Might be worth adding a comment (on line 42 just before the function), that it's called from a separate FreeRTOS task (and not an ISR).
642affc to
84a5d9a
Compare
|
For some reason your author email address is totally empty?? |
|
Oh that's strange. Seems the git config for that repo got an |
84a5d9a to
7643233
Compare
Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
This improves (decreases) the latency on stdin, on SoCs with built-in USB and using TinyUSB, like S2 and S3. Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
7643233 to
06a7bf9
Compare
|
Nice work, @andrewleech! In the context of #12846 and trying to support Python callbacks from USB code, I've been wondering about the relative merits of running TinyUSB in its own task as ESP-IDF does. I think it probably is worthwhile on dual-core systems, although I'd also like to measure it. If we did it more similarly to the other ports then some of these problems would go away (USB interrupt could schedule the TinyUSB handler to run in the main task.) Plus would reclaim some RAM (the handler task stack). |
This improves latency on stdin on esp32 parts with built in (tinyusb based) USB like S2 and S3.
The latency is highly visible when importing a module on a
mpremote mountsession, directly slowing down the import process.With a 14KB python file in mpremote mount it brings the import time from 12.1 sec down to 4.2 sec
When combined with #12836 the import time drops to ~900ms