esp32: Fix ESP32-C3 usb serial/jtag on IDF v5.0.4, clean up native USB-CDC #15727
Conversation
|
Limitation of this PR on ESP32-S3, even if you set: Then the USB serial/JTAG still enables. This is no worse than the behaviour on master, though, so will submit a follow-up PR for it. |
|
I had some second thoughts about this last night, because it commits MicroPython to being the canonical source for its stdio routing (i.e. whether to connect stdio to UART serial, USB Serial/JTAG peripheral, and/or USB-CDC device with full USB OTG peripheral). The ESP-IDF stdio is not really kept consistent with this (in current master and in this PR). Currently on master the ESP-IDF/libc stdout can be routed to UART serial and USB serial/JTAG, but not to the USB-CDC serial device - IIUC that "port" only ever gets MicroPython stdio. This is mostly fine, I think it only really comes up when enabling ESP-IDF logging or if someone links a user C module that calls libc printf() or similar. However currently in master the ESP-IDF sdkconfig is the canonical source for both stdio configs, and this PR changes it to make MicroPython build macros the canonical source for the MicroPython stdio config. So there's potential for more divergence. I think the main limitation is that there are currently MicroPython configs that ESP-IDF's stdio can't support with its default Kconfig settings, i.e. output to UART0 and USB-CDC simultaneously. Maybe of more concern, configuring everything in ESP-IDF probably makes it fiddlier to change stdio around at runtime. So maybe this PR doubling down on the current approach is OK. If we want to sync up ESP-IDF/libc output (so e.g. IDF logs can go to CDC port) then we can do it by installing custom stdout or logging handlers when MicroPython starts up. |
ports/esp32/mpconfigport.h
Outdated
| #ifndef MICROPY_HW_ESP_USB_SERIAL_JTAG | ||
| #define MICROPY_HW_ESP_USB_SERIAL_JTAG (CONFIG_ESP_CONSOLE_USB_SERIAL_JTAG_ENABLED || ( \ | ||
| ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 1, 0) && ( \ | ||
| CONFIG_ESP_CONSOLE_SECONDARY_USB_SERIAL_JTAG || \ |
There was a problem hiding this comment.
I don't think we ever checked/used CONFIG_ESP_CONSOLE_SECONDARY_USB_SERIAL_JTAG, so could probably leave that out of this macro definition. But also doesn't hurt to include it.
There was a problem hiding this comment.
Actually... maybe the inclusion of this SECONDARY option is why S3 changes behaviour?
There was a problem hiding this comment.
Yes, this is why S3 changed behaviour. That option is set by default in the ESP-IDF config.
There was a problem hiding this comment.
So, isn't it possible to just exclude CONFIG_ESP_CONSOLE_SECONDARY_USB_SERIAL_JTAG from the check, and then it will work on S3?
Said differently: without this PR, S3 works. And the only thing that needs fixing is a rename of CONFIG_ESP_CONSOLE_USB_SERIAL_JTAG_ENABLED to CONFIG_ESP_CONSOLE_USB_SERIAL_JTAG for IDF < 5.1. And the latter can be achieved probably with:
#if CONFIG_ESP_CONSOLE_USB_SERIAL_JTAG
#define CONFIG_ESP_CONSOLE_USB_SERIAL_JTAG_ENABLED 1
#endifThere was a problem hiding this comment.
I think that suggested patch is actually equivalent behaviour-wise to the first commit in this PR, although I didn't fully understand that until just now when I went to look into it again:
Serial over S3 native USB port currently works on master in two different ways depending on which ESP-IDF version you have, with <5.1 you get native USB and with >=5.1 you get USB Serial/JTAG peripheral (because both CONFIG_ESP_CONSOLE_SECONDARY_USB_SERIAL_JTAG and CONFIG_ESP_CONSOLE_USB_SERIAL_JTAG_ENABLED are set by default on both S3 and C3).
That particular regression (switching to the USB/JTAG peripheral) was introduced by me unwittingly.
With the patch (either the three line one or the first commit in this PR by itself) then S3 would become USB Serial/JTAG regardless of ESP-IDF version.
So I think the three line patch would indeed fix the immediate issue with C3 on v5.0.4 and wouldn't make anything else obviously worse. Maybe that is worth doing now as a quick PR to unbreak C3 on nightly builds.
However, it also seems pretty clear that we should clean this up so it's easier to reason about. Either with the approach in this PR, or with something else.
I don't understand why this happened with S3. Also, is this problem with S3 only on IDF 5.0.4, or only on IDF 5.2.2, or both? |
See reply inline at #15727 (comment) . The first commit changes S3 behaviour on both versions. I can sit down next week and try to find a cleaner way to refactor this. I tried for this PR but it's fiddly (see #15727 (comment)). |
|
I for one fully support stdio management in micropython, rather than via IDF. Tangential to this change, I've been looking at the way all stm32 stdio is currently run via dupterm (if enabled) during my in-progress efforts to migrate stm32 to tinyusb. The advantage of this is full runtime / application support for enabling & disabling any combination of peripherals for stdio. This will require a stream class wrapper around the usb cdc code and registering it along with any uarts etc with dupterm, which I'm keen to re-implement in the shared tinyusb codebase. |
50aa3bb to
be77a35
Compare
@andrewleech @dpgeorge After thinking about this some more I've rewritten this PR to completely decouple the ESP-IDF stdio config and the MicroPython stdio config. They were already not equivalent(*), so I think this actually makes it way easier to reason about. (*) And could not be made equivalent because if you switch stdout to USB-CDC in ESP-IDF config then it disables UART stdout. Have rewritten the PR description as well, and re-ran the tests mentioned there. PTAL. |
This should be S2 as well I believe? |
| usb_serial_jtag_init(); | ||
| #elif CONFIG_USB_OTG_SUPPORTED | ||
| #elif MICROPY_HW_USB_CDC |
There was a problem hiding this comment.
I like this, using MICROPY_HW_xxx config options exclusively makes it easier to follow.
But, it seems this will clash with #15108, which uses MICROPY_HW_ENABLE_USBDEV here instead of MICROPY_HW_USB_CDC. Maybe the former is better because you want USB to be initialised if any USB mode is used (eg just MSC)?
I guess a lot of this PR will clash with #15108, eg the changes to mphalport.c. Don't want to have our efforts here wasted if #15108 will need to change things anyway.
There was a problem hiding this comment.
Yes, agree that eventually we want to split this into MICROPY_HW_ENABLE_USBDEV and MICROPY_HW_USB_CDC the same as the rp2 port. This did cross my mind when creating this, but I figured at the moment these two macros would be equivalent - so leave splitting them out for when introducing the functionality that needs to differentiate them. I can see the case for adding it now, though - a little less churn.
@andrewleech Is doing this one way or the other way going to be easier for you?
There was a problem hiding this comment.
Looked at #15108 again and can see the point, so I've pushed commit efe88a2 that adds MICROPY_HW_ENABLE_USBDEV. It's a bit odd because if you build with #define MICROPY_HW_ENABLE_USBDEV (1) and #define MICROPY_HW_USB_CDC (0) then the port enumerates as a USB-CDC device (as the descriptor is configured in ESP-IDF) but the port doesn't do anything.
It might be better if we drop that commit from here, and @andrewleech adds it to his branch the next time he rebases #15108. Not sure.
(EDIT: Updated commit hash after rebase and reformat.)
There was a problem hiding this comment.
Let's make the change to use MICROPY_HW_ENABLE_USBDEV in #15108 instead. That keeps this PR simple.
I always forget about the ESP32-S2, you're entirely correct! Have updated the PR description. |
be77a35 to
083f4da
Compare
Regression in 0a11832 in IDF 5.0.x where macro CONFIG_ESP_CONSOLE_USB_SERIAL_JTAG_ENABLED is not defined. With this patch, ESP32-S3 still USB Serial/JTAG incorrectly (now on all ESP-IDF versions). Closes micropython#15701 This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
efe88a2 to
97a5a49
Compare
This fixes issue of ESP32-S3 switching its config over to USB serial/JTAG instead of native USB. The the existing logic was hard to follow, adding this config macro makes it easier to see which USB is in use and to have board definitions that enable/disable different USB levels. This commit also drops (nominal) support for manually setting CONFIG_ESP_CONSOLE_USB_CDC in sdkconfig. No included board configs use this and it didn't seem to work (if secondary console was set to the default USB Serial/JTAG then there is no serial output on any port, and if secondary console was set to None then linking fails.) Can be re-added if there's a use case for it. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
97a5a49 to
5e692d0
Compare
|
Unfortunately it looks like this PR broke That board defines The option |
Summary
MICROPY_HW_USB_CDCfor native USB-CDC which defaults to 1 on supported SoCs (currently ESP32-S2,S3).MICROPY_HW_ESP_USB_SERIAL_JTAGto use the serial/JTAG peripheral which defaults to 1 on supported SoCs that don't have native USB-CDC enabled.MICROPY_HW_ENABLE_UART_REPLthis gives full control in the MicroPython board header over which serial options are enabled. This somewhat helps with ESP32-S3 forcing use of USB OTG #14217 although it's not a fix for it.CONFIG_ESP_CONSOLE_USB_CDCon S3 has been removed. This already didn't work on master, and no boards used it. Getting both ESP-IDF stdio and MicroPython stdio on the USB-CDC is actually now possible with this PR (didn't work without it), but requires setting both the sdkconfig option and the MicroPython build option independently.Testing
Build and verified REPL works with ESP-IDF v5.0.4 and v5.2.2 on:
Trade-offs and Alternatives
We could drop ESP-IDF v5.0.4 support now, but nightly builds have only just switched over so good to wait a little while and see if any other issues crop up. There is still the S3 issue to resolve.
There is a weird inconsistency where ESP-IDF stdout (and logs) go to different places than MicroPython stdout. However that's already the case on master and this PR makes it a little easier to reason about (because the ESP-IDF stdio config and the MicroPython stdio config are now fully independent of each other). If necessary then we could add a hook that routes ESP-IDF stdout into MicroPython stdout as part of app_main(), but I think safe to leave for now.
Future Work
This work was funded through GitHub Sponsors.