ports/esp32: Use shared/tinyusb integration. #15108

andrewleech · 2024-05-24T03:32:33Z

Uses newer TinyUSB synopsys/dwc2 driver for esp32s2 and esp32s3 rather than the IDF tinyusb component.

This allows re-use of other tinyusb integration code / features shared between ports.

Also add / fix automatic bootloader handling for S2 and S3 by adding support for the ESP standard DTR/RTS based reboot to bootloader.

On S2 and S3 this requires esptool to be run twice (fails on first run as the chip resets and usb drops out) unless used with: espressif/esptool#980

This PR also switches from OTG to Serial/Jtag mode on S3 to workaround issue discussed in:
espressif/arduino-esp32#6762

Also, after flashing with esptool, auto-reboot back to application (micropython) works when used with new enough esptool containing espressif/esptool@0215786
This is not in the current release 4.8, so 4.9 beta release or install from GitHub url is needed.

codecov · 2024-05-24T04:01:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.57%. Comparing base (ca6723b) to head (548f88d).
Report is 6 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #15108   +/-   ##
=======================================
  Coverage   98.57%   98.57%           
=======================================
  Files         164      164           
  Lines       21336    21336           
=======================================
  Hits        21031    21031           
  Misses        305      305

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-06-04T03:59:33Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

andrewleech · 2024-06-21T01:26:56Z

This is now tested and working on both S2 and S3.

I now consider it feature complete and ready for review.

dpgeorge · 2024-07-05T01:56:41Z

ports/esp32/mpconfigport.h

+#endif
+
+#ifndef MICROPY_HW_USB_DESC_STR_MAX
+#define MICROPY_HW_USB_DESC_STR_MAX         (16)


This now defaults to 40, so probably best left at that default and remove this setting here?

dpgeorge · 2024-07-05T02:00:16Z

ports/esp32/mpconfigport.h

+#endif
+#endif
+
+#define MICROPY_HW_USBDEV_TASK_HOOK extern void mp_usbd_task(void); mp_usbd_task();


Does it need to poll tud_task() like this? Surely it can be done by scheduling from a USB IRQ? That's how the rp2 port works.

How did the IDF handle calling tud_task() prior to this PR, did it have a dedicated FreeRTOS thread to run TinyUSB?

Yeah I'm pretty sure IDF has a low priority tight loop task servicing the USB.

I'll have to look into whether we can use the interrupt, you're right it would certainly be a better option.

You should be able to look at the source for that tight-loop task, and see what semaphore it waits on. Then see where that semaphore is set. That's the location we can schedule our tud task callback.

Oh, huh, the driver we're using here does that automatically for us!

https://github.com/hathach/tinyusb/blob/236aa9622a31b8c4727c98c6d683cee011fb8f9b/src/portable/espressif/esp32sx/dcd_esp32sx.c#L878

I'll just want to confirm that this is definitely getting called to configure it.

You'll need to wrap _dcd_int_handler at the linker level, I guess.

dpgeorge · 2024-07-05T02:02:46Z

ports/esp32/mphalport.h

@@ -52,6 +52,7 @@

 extern TaskHandle_t mp_main_task_handle;

+extern int mp_interrupt_char;


I think it's better to include shared/runtime/interrupt_char.h rather than declare this variable.

dpgeorge · 2024-07-05T02:08:12Z

shared/tinyusb/mp_usbd_cdc.c

@@ -162,6 +172,15 @@ tud_cdc_line_state_cb
        tud_sof_cb_enable(true);
    }
    #endif
+    #if MICROPY_HW_ESP_AUTOMATIC_BOOTLOADER


I reckon this new config option should be called MICROPY_HW_USB_CDC_DTR_RTS_BOOTLOADER, to indicate that it's part of the USB CDC code.

And then put a default value in shared/tinyusb/mp_usbd_cdc.h of 0 (disabled) with a comment that this should only be used on ESP32 boards.

dpgeorge · 2024-07-05T02:09:27Z

ports/esp32/Makefile

+endif
+ifneq ($(BAUD),)
+BAUD_ARG := -b $(BAUD)
+endif


So this now leaves the selection of port and baud to idf.py? Does that give a better user experience?

As a default, yes. Esptool does have some auto detection logic that has worked very well for me during development.

dpgeorge · 2024-07-19T04:37:29Z

@andrewleech I see you made changes. Is this ready for re-review?

andrewleech · 2024-07-19T05:12:55Z

Not quite sorry @dpgeorge I need to test them still.

andrewleech · 2024-07-20T09:52:05Z

Ok now it's re-tested on S2 and S3 with another rebase.

I've fixed up based on review feedback and yes it all appears to work correctly without the polling of the USB task so that's great!

Now ready for review thanks @dpgeorge

sebromero · 2024-10-02T07:11:08Z

Amazing work @andrewleech 👏 Will this allow us to use the existing USB HID classes (e.g. the keyboard)?

andrewleech · 2024-10-02T08:40:46Z

@sebromero the intention is to support the runtime USB subsystem yes! However I haven't enabled or tested that yet as part of this PR.

I'm not sure if I should just enable the runtime support defines here by default for the S2 and S3, what do you think @projectgus ?

sebromero · 2024-10-02T09:01:08Z

@andrewleech Nice! Let me know if / how I can help testing this. I can also get my students to run an experiment so we would have plenty of test data :-)

dpgeorge · 2024-10-03T02:13:45Z

I'm not sure if I should just enable the runtime support defines here by default for the S2 and S3

Let's leave that for a separate PR.

dpgeorge · 2024-10-03T05:49:37Z

shared/tinyusb/mp_usbd.c

@@ -49,12 +49,17 @@ void mp_usbd_task_callback(mp_sched_node_t *node) {

 extern void __real_dcd_event_handler(dcd_event_t const *event, bool in_isr);

+// Provide stub for ports that don't have/need the wake main task function.
+void __attribute((weak)) mp_hal_wake_main_task_from_isr(void) {


I suggest to do this slightly differently and force all ports using these shared TinyUSB bindings to define this function. The ports can just add the following to their mphalport.h:

static inline void mp_hal_wake_main_task_from_isr(void) { // Nothing needs to be done here. }

Reasons:

Reduce code size and don't need to call a useless empty function (see rp2 code size report).

Don't rely on weak linking, which may not be that portable and could lead to issues if the wrong function is linked instead.

Make it explicit that a port doesn't need to implement anything for this MP HAL function, and provide a clear place to add it if needed (self documenting).

Thanks @dpgeorge I'm not a huge fan on boilerplate but yes it's true weak functions can be a little unclear / non-deterministic, it's hard to be certain which version is actually used.

I considered using a define instead to set if this function should be needed, but that's hard to document / describe as well.

This probably is the best compromise, I've implemented it for each of the TinyUSB ports now.

I considered using a define instead to set if this function should be needed, but that's hard to document / describe as well.

Yeah, I had the same thoughts as you.

Thanks for updating!

dpgeorge · 2024-10-03T05:50:57Z

I tested this on ESP32 S2 and S3 and it works, and it looks all good except the comment above about mp_hal_wake_main_task_from_isr().

Uses newer TinyUSB synopsys/dwc2 driver for esp32s2 and esp32s3 rather than the IDF tinyusb component. This allows re-use of other tinyusb integration code and features shared between ports. Signed-off-by: Andrew Leech <andrew@alelec.net>

No longer needed as shared tinyusb is now used by the esp32 port. Signed-off-by: Andrew Leech <andrew@alelec.net>

Enables support for the ESP standard DTR/RTS based reboot to bootloader. Switches from OTG to Serial/Jtag mode to workaround issue discussed in: espressif/arduino-esp32#6762 Signed-off-by: Andrew Leech <andrew@alelec.net>

Signed-off-by: Andrew Leech <andrew@alelec.net>

The custom line state handling is no longer needed as MicroPython runs it directly now. Signed-off-by: Andrew Leech <andrew@alelec.net>

Signed-off-by: Andrew Leech <andrew@alelec.net>

dpgeorge · 2024-10-07T00:29:50Z

Merged!

Thanks very much @andrewleech for all your efforts on this, and everyone else who contributed and tested.

sebromero · 2024-10-14T00:13:19Z

I'm not sure if I should just enable the runtime support defines here by default for the S2 and S3, what do you think

@andrewleech Any hint on how to do this? 🙏 Would love to test this.

andrewleech · 2024-10-15T05:17:47Z

@sebromero keep an eye on this: #15564 (comment)

gampam2000 · 2024-10-18T06:13:23Z

Hi, recently I can't compile micropython anymore. I came across this PR in my search and could it be that the errors are related to this? I Use an ESP32S3.

I get errors like this:

In file included from /micropython/ports/esp32/usb.c:36:
/micropython/shared/tinyusb/mp_usbd.h: In function 'mp_usbd_init_tud':
/micropython/shared/tinyusb/mp_usbd.h:46:5: error: unknown type name 'tud_cdc_configure_fifo_t'
   46 |     tud_cdc_configure_fifo_t cfg = { .rx_persistent = 0, .tx_persistent = 1 };
      |     ^~~~~~~~~~~~~~~~~~~~~~~~
/micropython/shared/tinyusb/mp_usbd.h:46:38: error: field name not in record or union initializer
   46 |     tud_cdc_configure_fifo_t cfg = { .rx_persistent = 0, .tx_persistent = 1 };
      |                                      ^
/micropython/shared/tinyusb/mp_usbd.h:46:38: note: (near initialization for 'cfg')
/micropython/shared/tinyusb/mp_usbd.h:46:58: error: field name not in record or union initializer
   46 |     tud_cdc_configure_fifo_t cfg = { .rx_persistent = 0, .tx_persistent = 1 };
      |                                                          ^
/micropython/shared/tinyusb/mp_usbd.h:46:58: note: (near initialization for 'cfg')
/micropython/shared/tinyusb/mp_usbd.h:46:75: warning: excess elements in scalar initializer
   46 |     tud_cdc_configure_fifo_t cfg = { .rx_persistent = 0, .tx_persistent = 1 };
      |                                                                           ^
/micropython/shared/tinyusb/mp_usbd.h:46:75: note: (near initialization for 'cfg')
In file included from /micropython/ports/esp32/main.c:55:
/micropython/shared/tinyusb/mp_usbd.h: In function 'mp_usbd_init_tud':
/micropython/shared/tinyusb/mp_usbd.h:46:5: error: unknown type name 'tud_cdc_configure_fifo_t'
   46 |     tud_cdc_configure_fifo_t cfg = { .rx_persistent = 0, .tx_persistent = 1 };
      |     ^~~~~~~~~~~~~~~~~~~~~~~~
/micropython/shared/tinyusb/mp_usbd.h:46:38: error: field name not in record or union initializer
   46 |     tud_cdc_configure_fifo_t cfg = { .rx_persistent = 0, .tx_persistent = 1 };
      |                                      ^
/micropython/shared/tinyusb/mp_usbd.h:46:38: note: (near initialization for 'cfg')
/micropython/shared/tinyusb/mp_usbd.h:46:58: error: field name not in record or union initializer
   46 |     tud_cdc_configure_fifo_t cfg = { .rx_persistent = 0, .tx_persistent = 1 };
      |                                                          ^
/micropython/shared/tinyusb/mp_usbd.h:46:58: note: (near initialization for 'cfg')
/micropython/shared/tinyusb/mp_usbd.h:46:75: warning: excess elements in scalar initializer
   46 |     tud_cdc_configure_fifo_t cfg = { .rx_persistent = 0, .tx_persistent = 1 };
      |                                                                           ^
/micropython/shared/tinyusb/mp_usbd.h:46:75: note: (near initialization for 'cfg')
/micropython/shared/tinyusb/mp_usbd.h:47:5: error: implicit declaration of function 'tud_cdc_configure_fifo' [-Werror=implicit-function-declaration]
   47 |     tud_cdc_configure_fifo(&cfg);
      |     ^~~~~~~~~~~~~~~~~~~~~~
/micropython/shared/tinyusb/mp_usbd.h:47:5: error: implicit declaration of function 'tud_cdc_configure_fifo' [-Werror=implicit-function-declaration]
   47 |     tud_cdc_configure_fifo(&cfg);
      |     ^~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
.....

Any ideas how to fix this?

Thanks & Best regards

vshymanskyy · 2024-10-18T06:53:09Z

I can't compile micropython anymore

For some reason, i needed to run this manually:

make -f ../../py/mkrules.mk GIT_SUBMODULES="lib/berkeley-db-1.xx lib/tinyusb lib/micropython-lib" submodules

This didn't do anything useful (probably this is a regression):

make BOARD=ESP32_GENERIC_S3 submodules

gampam2000 · 2024-10-18T07:19:39Z

make BOARD=ESP32_GENERIC_S3 submodules does work

make[1]: Entering directory '/lib/micropython/ports/esp32'
Use make V=1 or set BUILD_VERBOSE in your environment to increase build verbosity.
Updating submodules: lib/berkeley-db-1.xx lib/tinyusb lib/micropython-lib
Synchronizing submodule url for 'lib/berkeley-db-1.xx'
Synchronizing submodule url for 'lib/micropython-lib'
Synchronizing submodule url for 'lib/tinyusb'
Submodule path 'lib/tinyusb': checked out '5217cee5de4cd555018da90f9f1bcc87fb1c1d3a'
make[1]: Leaving directory '/lib/micropython/ports/esp32'

It checks out 5217cee5de4cd555018da90f9f1bcc87fb1c1d3a of tinyusb, is this the correct Git ID?

gampam2000 · 2024-10-18T07:30:21Z

I found the issue: I needed to add set(MICROPY_PY_TINYUSB ON) in my boards CMakeLists.txt

andrewleech · 2024-10-18T08:29:26Z

5217cee5de4cd555018da90f9f1bcc87fb1c1d3a of tinyusb, is this the correct Git ID?

Yes that's the correct / current pinned version of TinyUSB.

Just to clarify, you were referring to building the ESP32_GENERIC_S3 board earlier, when you now say you needed to set MICROPY_PY_TINYUSB is this on a custom board definition of yours? Normally that setting is configured automatically in

micropython/ports/esp32/main_esp32s3/CMakeLists.txt

Line 10 in 1b89c50

so I'm not sure why it's not working for you.

gampam2000 · 2024-10-18T11:24:38Z

Just to clarify, you were referring to building the ESP32_GENERIC_S3 board earlier, when you now say you needed to set MICROPY_PY_TINYUSB is this on a custom board definition of yours?

Yes I used this only to initialize the submodules.

Normally that setting is configured automatically in

micropython/ports/esp32/main_esp32s3/CMakeLists.txt

Line 10 in 1b89c50

so I'm not sure why it's not working for you.

Thanks for pointing that out.
I'm using the micropython-example-boards setup, I guess that file is than not automatically included. Maybe this is an issue in the micropython-example-boards?

Update:
It should be included: CMakeLists.txt#L4C1-L4C50

cnadler86 · 2024-10-21T10:29:50Z

I can't compile micropython anymore

For some reason, i needed to run this manually:
make -f ../../py/mkrules.mk GIT_SUBMODULES="lib/berkeley-db-1.xx lib/tinyusb lib/micropython-lib" submodules
This didn't do anything useful (probably this is a regression):
make BOARD=ESP32_GENERIC_S3 submodules

I had also problems while compiling on my board definition. This solved also my problem. Maybe on a fresh micropython repo-checkout, this is not needed, because the submodules are automatically registered and that is why the pipeline is passing, but on existing repos the submodule is not been registered automatically.

andrewleech mentioned this pull request May 24, 2024

feat(reset): Automatically reconnect if port disconnects during reset. (ESPTOOL-862) espressif/esptool#980

Closed

andrewleech force-pushed the esp32sX_tinyusb_integration branch from c5120d6 to 23cf623 Compare May 24, 2024 04:19

andrewleech mentioned this pull request May 29, 2024

shared/tinyusb: Buffer startup stdout (banner) to send once usb cdc is connected. #14485

Merged

cheops mentioned this pull request May 31, 2024

Deploying to Fox doesn't automatically restart the badge Fri3dCamp/badge_2024_micropython#2

Closed

dpgeorge added the port-esp32 label Jun 3, 2024

andrewleech force-pushed the esp32sX_tinyusb_integration branch 3 times, most recently from 2360143 to 9e54307 Compare June 4, 2024 03:47

andrewleech force-pushed the esp32sX_tinyusb_integration branch 2 times, most recently from 340212b to c17e1c6 Compare June 7, 2024 01:16

andrewleech force-pushed the esp32sX_tinyusb_integration branch 6 times, most recently from 722590c to c2691ee Compare June 20, 2024 09:23

andrewleech force-pushed the esp32sX_tinyusb_integration branch from c2691ee to 33facbd Compare June 27, 2024 06:11

dpgeorge added this to the release-1.24.0 milestone Jul 2, 2024

dpgeorge reviewed Jul 5, 2024

View reviewed changes

andrewleech force-pushed the esp32sX_tinyusb_integration branch from 33facbd to c80ed32 Compare July 18, 2024 12:38

andrewleech force-pushed the esp32sX_tinyusb_integration branch 2 times, most recently from 2372a32 to c7d8651 Compare July 20, 2024 09:48

dpgeorge reviewed Oct 3, 2024

View reviewed changes

dpgeorge closed this Oct 3, 2024

dpgeorge reopened this Oct 3, 2024

andrewleech force-pushed the esp32sX_tinyusb_integration branch 3 times, most recently from fae3ea7 to 67d9d67 Compare October 5, 2024 04:36

pi-anl added 6 commits October 7, 2024 11:06

shared/tinyusb: Remove MICROPY_HW_USB_EXTERNAL_TINYUSB.

641f600

No longer needed as shared tinyusb is now used by the esp32 port. Signed-off-by: Andrew Leech <andrew@alelec.net>

esp32: Add automatic bootloader handling for S2 and S3.

5ae622e

Enables support for the ESP standard DTR/RTS based reboot to bootloader. Switches from OTG to Serial/Jtag mode to workaround issue discussed in: espressif/arduino-esp32#6762 Signed-off-by: Andrew Leech <andrew@alelec.net>

esp32/Makefile: Allow auto-port selection if not passed on cmdline.

386771e

Signed-off-by: Andrew Leech <andrew@alelec.net>

esp32/boards: Update ARDUINO_NANO_ESP32 USB configuration.

11bc7d0

The custom line state handling is no longer needed as MicroPython runs it directly now. Signed-off-by: Andrew Leech <andrew@alelec.net>

shared/tinyusb: Wake main task if needed at end of USB ISR.

548f88d

Signed-off-by: Andrew Leech <andrew@alelec.net>

dpgeorge force-pushed the esp32sX_tinyusb_integration branch from 67d9d67 to 548f88d Compare October 7, 2024 00:08

dpgeorge merged commit 548f88d into micropython:master Oct 7, 2024
62 checks passed

vshymanskyy mentioned this pull request Oct 17, 2024

esp32: Allow disabling USB CDC. #16035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ports/esp32: Use shared/tinyusb integration. #15108

ports/esp32: Use shared/tinyusb integration. #15108

andrewleech commented May 24, 2024 •

edited

Loading

codecov bot commented May 24, 2024 •

edited

Loading

github-actions bot commented Jun 4, 2024 •

edited

Loading

andrewleech commented Jun 21, 2024 •

edited

Loading

dpgeorge Jul 5, 2024

dpgeorge Jul 5, 2024

andrewleech Jul 5, 2024

dpgeorge Jul 5, 2024

andrewleech Jul 5, 2024 •

edited

Loading

dpgeorge Jul 5, 2024

dpgeorge Jul 5, 2024

dpgeorge Jul 5, 2024

dpgeorge Jul 5, 2024

andrewleech Jul 5, 2024

dpgeorge commented Jul 19, 2024

andrewleech commented Jul 19, 2024

andrewleech commented Jul 20, 2024 •

edited

Loading

sebromero commented Oct 2, 2024 •

edited

Loading

andrewleech commented Oct 2, 2024

sebromero commented Oct 2, 2024

dpgeorge commented Oct 3, 2024

dpgeorge Oct 3, 2024

andrewleech Oct 5, 2024

dpgeorge Oct 5, 2024

dpgeorge commented Oct 3, 2024

dpgeorge commented Oct 7, 2024

sebromero commented Oct 14, 2024

andrewleech commented Oct 15, 2024

gampam2000 commented Oct 18, 2024

vshymanskyy commented Oct 18, 2024

gampam2000 commented Oct 18, 2024

gampam2000 commented Oct 18, 2024

andrewleech commented Oct 18, 2024

gampam2000 commented Oct 18, 2024 •

edited

Loading

cnadler86 commented Oct 21, 2024

		@@ -52,6 +52,7 @@

		extern TaskHandle_t mp_main_task_handle;

		extern int mp_interrupt_char;

ports/esp32: Use shared/tinyusb integration. #15108

ports/esp32: Use shared/tinyusb integration. #15108

Conversation

andrewleech commented May 24, 2024 • edited Loading

codecov bot commented May 24, 2024 • edited Loading

Codecov Report

github-actions bot commented Jun 4, 2024 • edited Loading

andrewleech commented Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewleech Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge commented Jul 19, 2024

andrewleech commented Jul 19, 2024

andrewleech commented Jul 20, 2024 • edited Loading

sebromero commented Oct 2, 2024 • edited Loading

andrewleech commented Oct 2, 2024

sebromero commented Oct 2, 2024

dpgeorge commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge commented Oct 3, 2024

dpgeorge commented Oct 7, 2024

sebromero commented Oct 14, 2024

andrewleech commented Oct 15, 2024

gampam2000 commented Oct 18, 2024

vshymanskyy commented Oct 18, 2024

gampam2000 commented Oct 18, 2024

gampam2000 commented Oct 18, 2024

andrewleech commented Oct 18, 2024

gampam2000 commented Oct 18, 2024 • edited Loading

cnadler86 commented Oct 21, 2024

andrewleech commented May 24, 2024 •

edited

Loading

codecov bot commented May 24, 2024 •

edited

Loading

github-actions bot commented Jun 4, 2024 •

edited

Loading

andrewleech commented Jun 21, 2024 •

edited

Loading

andrewleech Jul 5, 2024 •

edited

Loading

andrewleech commented Jul 20, 2024 •

edited

Loading

sebromero commented Oct 2, 2024 •

edited

Loading

gampam2000 commented Oct 18, 2024 •

edited

Loading