ESP32S2: move to official IDF submodule #3492

hierophect · 2020-09-30T15:36:59Z

Up to now, the ESP32S2 port has been using a modified version of the ESP32-IDF (IoT Development Framework, the core framework for developing on the ESP32-S2 with all HAL APIs, drivers, FreeRTOS components, etc) forked by @tannewt. This was because the IDF itself contains a lot of undef type preprocessor errors that conflict with our use of -Werror, which Scott manually fixed in his fork.

We recently discovered a workaround for this, which involves using the "-isystem" directory option to mark all IDF files as system files, ignoring their warnings. This lets use use the official IDF, though it does require a lot of changes, both to make sure all preprocessor includes use an -isystem derived path, and to represent a bunch of other changes to the IDF since we forked (lots of files were moved refactored from "soc" locations to "hal" locations, and a critical system binary was renamed).

With @hathach and @julianrendell's help, the USB issue with this has been fixed. Tested successfully on a Saola 1 Wrover.

hathach · 2020-09-30T15:45:12Z

I have no problem with my IDF local with espressif/esp-idf@6c17e3a

I am not fully aware of what and when cause the issues, but we can start to go through commits list from there. There is an issue with persistent mode as mentioned by @me-no-dev in this tinyusb pr hathach/tinyusb#492 . I am not sure if the work on IDF got updated with this, or fixed else, maybe it is just pin config changing that happens as last time.

jepler

Without minimizing the amount of work this actually required, the changes to this point look quite sensible to the point of being simple and straightforward (to read).

I am surprised that the PR does not include a change to the esp-idf submodule itself, but that is presumably due to its draft state.

hierophect · 2020-09-30T16:08:08Z

I am surprised that the PR does not include a change to the esp-idf submodule itself, but that is presumably due to its draft state.

You can't see the Submodule changes? Should be visible in .gitmodules and as a ~1000 file submodule update further down the file list. They're visible for me, is it possible you expected the module change to be at the top and missed that it was further down? I'm not sure why it did that this time, normally it puts submodule updates at the top of the changelist.

jepler · 2020-09-30T16:15:38Z

Yeah my bad, it's there. It just, uh, looked different than I was looking for (because it didn't call out the list of files modified in the submodule)

I may just return to bed, I'm not doing so good at detail oriented stuff today.

julianrendell · 2020-09-30T17:34:05Z

@hierophect thanks for the very clear summary; I was curious what the -isystem was changing.

@hathach thanks for a known good working point.

I have a mostly-admin task day today, so I'm going to learn about git bisect and start working from espressif/esp-idf@6c17e3a to the current latest, and see if I can find the point where it stops working.

julianrendell · 2020-09-30T18:20:36Z

Woot- I'm liking git bisect!

0aa1c1302705b16fa5560ee4491a12e42631fe7f is the first bad commit
commit 0aa1c1302705b16fa5560ee4491a12e42631fe7f
Author: me-no-dev <[email protected]>
Date:   Fri Aug 21 01:44:11 2020 +0300

    Fix USB CLK always reset and USB with swapped pins not working

 components/esp_system/port/esp32s2/clk.c | 1 -
 components/soc/src/esp32s2/usb_hal.c     | 1 -
 2 files changed, 2 deletions(-)

julianrendell · 2020-09-30T19:10:45Z

Green LED!

@hierophect @hathach @me-no-dev: the following patch gets (CircuitPython) UF2 working:

git diff                                                                                                                                             406ms  Wed 30 Sep 18:43:32 2020
diff --git a/components/esp_system/port/esp32s2/clk.c b/components/esp_system/port/esp32s2/clk.c
index 0306d5d02..041efc699 100644
--- a/components/esp_system/port/esp32s2/clk.c
+++ b/components/esp_system/port/esp32s2/clk.c
@@ -233,6 +233,7 @@ void esp_perip_clk_init(void)
 #if CONFIG_ESP_CONSOLE_UART_NUM != 1
                            DPORT_UART1_CLK_EN |
 #endif
+                           DPORT_USB_CLK_EN |
                            DPORT_SPI2_CLK_EN |
                            DPORT_I2C_EXT0_CLK_EN |
                            DPORT_UHCI0_CLK_EN |
@@ -315,4 +316,4 @@ void esp_perip_clk_init(void)

     /* Enable RNG clock. */
     periph_module_enable(PERIPH_RNG_MODULE);
-}
\ No newline at end of file
+}
diff --git a/components/hal/esp32s2/usb_hal.c b/components/hal/esp32s2/usb_hal.c
index 359f50d68..c0188804c 100644
--- a/components/hal/esp32s2/usb_hal.c
+++ b/components/hal/esp32s2/usb_hal.c
@@ -22,5 +22,6 @@ void usb_hal_init(usb_hal_context_t *usb)
         usb_ll_ext_phy_enable();
     } else {
         usb_ll_int_phy_enable();
+        usb_ll_int_phy_pullup_conf(true, false, false, false);
     }
 }

@hathach - I think this means that the change to esp_idf that triggered hathach/tinyusb#492 to be needed is now live in esp_idf. But skimming all the comments, it sounds like there are some stability issues with that PR?

I've tried updating tinyusb in uf2_esp32 to the latest, adding me-no-devs fork as a remote, and merging in esp32-s2-persistence. Unfortunately that didn't work.

@hierophect & @hathach - so I think we have a short term work-around by patching esp_idf, but longer term should we be aiming for no changes to esp_idf and focus on getting hathach/tinyusb#492 working and update the reference in CircuitPython?

hierophect · 2020-09-30T19:44:55Z

@julianrendell thanks for putting in the work to help track this down! I've also been circling this issue and your contributions have been super helpful.

In general, our goal is to transition completely over to the Espressif repo of the IDF as soon as possible. We need the latest changes to the ESP-IDF in order to enable the ADC calibration measures that allow ADC readings to be accurate to within 0.1V, and we don't want to be operating off a fork anyway (see the PR description for why that was done at all). So issuing another patch to solve this issue halfway is not something I'd like to do, if possible.

I'm not confident enough with my USB familiarity to understand what exactly went wrong, here. So espressif/esp-idf@0aa1c13 broke USB on TinyUSB, if I understand properly? Why exactly was this change made by Espressif? Is the solution to submit a pull request to Espressif informing them of an error, or to change our own code in either Circuitpython or TinyUSB to reflect the changes they made? @hathach and @tannewt I'd appreciate your thoughts on next steps here.

From a debugging standpoint, I can confirm that on Circuitpython, this issue does appear to be related to TinyUSB, since the debugger shows no obvious problems with the Circuitpython core. @julianrendell I assume your tests are referring to adafruit/tinyuf2#12 (comment), and you are still using your same methods for debugging?

julianrendell · 2020-09-30T19:50:18Z

After a git mistake... I've just recompiled CP clean using esp_idf patched as per above, and at least the uf2 build appears to be working (REPL appears in mu.)

hierophect · 2020-09-30T19:50:39Z

Also, for reference, here are the relevant commits:

Thach's commit, apparently working (I haven't tested yet) : 6c17e3a Aug 19
USB breaking commit: 0aa1c13 Aug 25
Commit before USB break: dde6222 Aug 25
HAL Refactor commit, which made many breaking changes to file paths and names: 5425ef4 Sept 1
ADC Calibration commit, which is our end goal to include and the point of all these schenanigans: 6a0951e Aug 13

I have been trying to use git merge-base --is-ancestor <commit> <commit> to get a sense of how these relate to each other, but I'm struggling a little bit because my results in attempting to compile seem to contradict my results... is-ancestor seems to indicate that the commit right before the USB break should support the HAL refactor changes, but when I check out to it, it breaks all of my file includes.

Edited to show submission dates, which I should have just checked in the first place.

julianrendell · 2020-09-30T20:00:02Z

@hierophect - you're welcome.

Here's what I've done:

figured out the point in esp_idf that caused Hard Lock-up on Saola-Wroom tinyuf2#12, discovered it's two lines, pulled it forward into a locally patched version of esp_idf based on pulling latest of esp_idf
verified that gets uf2-esp32s working
recompiled CP with patched esp_idf, dropped the generated uf2 on to the working uf2-esp32s and done minimal verification (Mu sees it and print("hello") works from the REPL)

I understand and agree re wanting to be able to use an unpatched esp_idf (to be super clear, I'm not proposing the we keep the change to esp_idf that got things working, just verifying this is the change that breaks uf2-esp32s)

Here's my understanding from reading the PR @hathach linked to: there is a needed change to esp_idf for persistent USB to work (not sure what that means, but it sounds good :-) ). @me-no-dev proposed some required changes to tiny_usb (hathach/tinyusb#492) but there are some instabilities with it?

I did try pulling the latest tinyusb into uf2-esp32s and applying hathach/tinyusb#492 (directly from @me-no-dev's branch) but that failed to work.

So in summary, I think for uf2-esp32s (and maybe CP?) to work with the latest, unmodified esp_idf, there are changes/fixes needed to tinyusb. That's out of my realm of experience.

@hathach @tannewt - does that sound about right? Suggestions for how to proceed appreciated :-)

hierophect · 2020-09-30T20:09:02Z

Ach, I figured out that the ADC calibration changes are actually the oldest of everything, which is why I had my timeline confused. So it's actually entirely feasible that by using the pre-break version, we'll also solve the ADC issue, and don't have to deal with any of the breaking file changes. Giving it a shot now.

julianrendell · 2020-09-30T20:19:04Z

@hierophect I've just checked and discovered that CP and uf2-esp32s are pulling in different versions of tinyusb.

My suggestion is that we aim to:

keep this PR working with latest IDF, with all the needed changes due to things moving in IDF and using -isystem
pull in the latest TinyUSB into a branch of uf2_esp32s2 and ask @hathach for help with getting that to work with the latest (unpatched) esp_idf
verify the updated TinyUSB works with CP, both as a "full flash" and as a UF2 app
then talk to @tannewt and team about updating tinyusb in CircuitPython- as that affects all boards, not just esp32s2 (I think)
update tinyusb in CP to a version that works with the latest esp_idf (and everything else), merge in this PR, and update the esp_idf in CP to point at the latest, vanilla, esp_idf. I think all of this step has to be done together due to dependencies.
celebrate :-)

-edit-
And also update TinyUSB in uf2-esp32s to match CP.

…dde6222

hierophect · 2020-09-30T20:40:38Z

Ok, I reverted the changes representing the ESP-IDF's refactor from components/soc to components/hal, along with the system binary name change also included in that commit (5425ef4). Confirmed working on my Saola 1 Wrover!

I'm merging this into my ADC branch so I can continue work on that. I don't think we have any other urgent reasons to be on the latest version of the ESP-IDF - if we merge this as is, we can always revisit IDF updates by simply reverting the above commit (e01e8dd). We can come back to it whenever TinyUSB is stable alongside an up-to-date ESP-IDF version.

hierophect · 2020-09-30T20:46:47Z

Actually, I spoke a little too soon there. TinyUSB does need a change for this to work. In src/portable/espressif/esp32s2/dcd_esp32s2.c, we need the includes altered to use the -isystem path, or they will throw undef errors:

-#include "driver/periph_ctrl.h"
-#include "freertos/xtensa_api.h"
-#include "esp_intr_alloc.h"
-#include "esp_log.h"
-#include "esp32s2/rom/gpio.h"
-#include "soc/dport_reg.h"
-#include "soc/gpio_sig_map.h"
-#include "soc/usb_periph.h"
+#include "components/driver/include/driver/periph_ctrl.h"
+#include "components/freertos/xtensa/include/freertos/xtensa_api.h"
+#include "components/esp32s2/include/esp_intr_alloc.h"
+#include "components/log/include/esp_log.h"
+#include "components/esp_rom/include/esp32s2/rom/gpio.h"
+#include "components/soc/soc/esp32s2/include/soc/dport_reg.h"
+#include "components/soc/soc/esp32s2/include/soc/gpio_sig_map.h"
+#include "components/soc/soc/esp32s2/include/soc/usb_periph.h"

@hathach should I make a PR for this? Or should I be handling it some other way, like using the version of TinyUSB inside the IDF?

julianrendell · 2020-09-30T20:54:29Z

If you use the version in esp_idf does that bring problems for the other CP platforms?

I’m out of time for now, but I’d be curious to try the tinyusb from latest idf with uf2_esp32s2 and CP and see If that works- ie brute force copy and replace. Maybe they’ve added a fix and have yet to PR it to tinyusb.

At this point it feels like a big enough problem that it’s worth trying to fix sooner than later- seems like it’s accumulating technical debt.

hathach · 2020-10-01T06:20:33Z

Actually, I spoke a little too soon there. TinyUSB does need a change for this to work. In src/portable/espressif/esp32s2/dcd_esp32s2.c, we need the includes altered to use the -isystem path, or they will throw undef errors:
-#include "driver/periph_ctrl.h"
-#include "freertos/xtensa_api.h"
-#include "esp_intr_alloc.h"
-#include "esp_log.h"
-#include "esp32s2/rom/gpio.h"
-#include "soc/dport_reg.h"
-#include "soc/gpio_sig_map.h"
-#include "soc/usb_periph.h"
+#include "components/driver/include/driver/periph_ctrl.h"
+#include "components/freertos/xtensa/include/freertos/xtensa_api.h"
+#include "components/esp32s2/include/esp_intr_alloc.h"
+#include "components/log/include/esp_log.h"
+#include "components/esp_rom/include/esp32s2/rom/gpio.h"
+#include "components/soc/soc/esp32s2/include/soc/dport_reg.h"
+#include "components/soc/soc/esp32s2/include/soc/gpio_sig_map.h"
+#include "components/soc/soc/esp32s2/include/soc/usb_periph.h"
@hathach should I make a PR for this? Or should I be handling it some other way, like using the version of TinyUSB inside the IDF?

To be honest, I don't like this change since IDF move files around quite a lot and there will be more mcu such as esp32s3, it is better if we could use the default include path set by IDF toolchain. Though if you think it is a must, please try to submit PR to tinyusb to see if it could pass the ci buld with stock IDF (which lots of people will also use as well), I am not sure if components is one of included path.

hierophect · 2020-10-01T15:36:58Z

To be honest, I don't like this change since IDF move files around quite a lot and there will be more mcu such as esp32s3, it is better if we could use the default include path set by IDF toolchain. Though if you think it is a must, please try to submit PR to tinyusb to see if it could pass the ci buld with stock IDF (which lots of people will also use as well), I am not sure if components is one of included path.

@hathach the issue is that we need these files to use the -isystem path, or they will cause errors like error: "CONFIG_IDF_TARGET_ESP32" is not defined, evaluates to 0 [-Werror=undef] #if CONFIG_IDF_TARGET_ESP32 while compiling. @tannewt do you have any suggestions?

hierophect · 2020-10-01T17:16:53Z

Sorry, I thought this was more complicated than it actually was. It just needs an update to override the esp-idf/components/log/include/ that tinyusb uses in this case. So no TinyUSB updates should be needed.

hathach · 2020-10-01T17:53:56Z

@hathach the issue is that we need these files to use the -isystem path, or they will cause errors like error: "CONFIG_IDF_TARGET_ESP32" is not defined, evaluates to 0 [-Werror=undef] #if CONFIG_IDF_TARGET_ESP32 while compiling. @tannewt do you have any suggestions?

Sorry, I thought this was more complicated than it actually was. It just needs an update to override the esp-idf/components/log/include/ that tinyusb uses in this case. So no TinyUSB updates should be needed.

I am glad you figure it out, espressif should really define this to CONFIG_IDF_TARGET_ESP32 to 0 to avoid this warning/error. Can we also just define this symbol to 0, cpy won't support ESP32 anyway. That is just my thinking, you obviously know more of the details :)

microdev1 · 2020-10-02T10:05:33Z

@hierophect The gdbgui fix 96cfdf3 was made just after 5425ef4 on Sep 2.

Also I tried reverting 0aa1c13 in esp-idf master.....it compiles just fine but no usb.

hierophect · 2020-10-05T16:38:42Z

@jepler @tannewt so, the current issue with this PR is that, while it works just fine on a host machine, the above gdbgui issue will fail CI. We also cannot update to the ESP-IDF version that fixes this without breaking USB, since TinyUSB does not work with any esp-idf commit after dde6222.

I'm not sure what to do here. We could merge this PR anyway and accept CI failures on every ESP32 build for a little while. Or we could wait until TinyUSB becomes compatible with the latest ESP-IDF versions, and just put a hold on the Analog APIs until then.

microdev1 · 2020-10-05T16:50:54Z

I don't think gdbgui error is just a CI thing. The esp-idf installation fails in my environment Ubuntu 20.04 LTS.

hierophect · 2020-10-05T17:03:57Z

I don't think gdbgui error is just a CI thing. The esp-idf installation fails in my environment Ubuntu 20.04 LTS.

Interesting. I wonder why it does not with my setup on Mac OSX?

hierophect · 2020-10-05T17:08:56Z

I guess that means we have no choice but to wait for TinyUSB compatibility. @hathach do you have a moment to check out TinyUSB compatibility with the latest release of the ESP-IDF this week? @julianrendell mentioned it might have something to do with hathach/tinyusb#492 but I'm not familiar enough with USB specifics to be sure. All I know for certain is that TinyUSB does not work with any ESP-IDF commit after espressif/esp-idf@0aa1c13

hathach · 2020-10-05T17:11:27Z

I guess that means we have no choice but to wait for TinyUSB compatibility. @hathach do you have a moment to check out TinyUSB compatibility with the latest release of the ESP-IDF this week? @julianrendell mentioned it might have something to do with hathach/tinyusb#492 but I'm not familiar enough with USB specifics to be sure. All I know for certain is that TinyUSB does not work with any ESP-IDF commit after espressif/esp-idf@0aa1c13

I will test it out this week for sure 👌👌

tannewt · 2020-10-05T20:52:13Z

@hierophect Sounds like you should make a new fork of the idf to fix gdbgui with tinyusb working. It'll still be better than mine because it'll be closer to the espressif version.

tannewt

Thank you!

Change submodule, rework all includes

66b8559

hierophect requested a review from tannewt September 30, 2020 15:36

jepler previously approved these changes Sep 30, 2020

View reviewed changes

tannewt added the espressif applies to multiple Espressif chips label Sep 30, 2020

Revert HAL refactor changes to avoid USB breaking changes past esp-idf …

e01e8dd

…dde6222

hierophect dismissed jepler’s stale review via e01e8dd September 30, 2020 20:36

Update TinyUSB include compatibility

9a3bd82

hierophect marked this pull request as ready for review October 1, 2020 17:20

hierophect requested a review from jepler October 1, 2020 17:20

hierophect mentioned this pull request Oct 1, 2020

ESP32S2: Add AnalogIn #3501

Merged

tannewt removed their request for review October 2, 2020 22:55

Update ESP-IDF to newer fixed fork

0d7552b

tannewt requested review from tannewt and removed request for jepler October 6, 2020 23:53

tannewt approved these changes Oct 6, 2020

View reviewed changes

tannewt merged commit 8c428a3 into adafruit:main Oct 6, 2020

hierophect deleted the esp32-update-idf branch October 7, 2020 16:16

hierophect mentioned this pull request Oct 7, 2020

Update ESP-IDF to official version #3516

Closed

hierophect mentioned this pull request Oct 26, 2020

ESP32-S2: Fix readme link, improve instruction clarity #3607

Merged

ESP32S2: move to official IDF submodule #3492

ESP32S2: move to official IDF submodule #3492

Uh oh!

Conversation

hierophect commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hathach commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jepler left a comment

Choose a reason for hiding this comment

Uh oh!

hierophect commented Sep 30, 2020

Uh oh!

jepler commented Sep 30, 2020

Uh oh!

julianrendell commented Sep 30, 2020

Uh oh!

julianrendell commented Sep 30, 2020

Uh oh!

julianrendell commented Sep 30, 2020

Uh oh!

hierophect commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julianrendell commented Sep 30, 2020

Uh oh!

hierophect commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julianrendell commented Sep 30, 2020

Uh oh!

hierophect commented Sep 30, 2020

Uh oh!

julianrendell commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hierophect commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hierophect commented Sep 30, 2020

Uh oh!

julianrendell commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hathach commented Oct 1, 2020

Uh oh!

hierophect commented Oct 1, 2020

Uh oh!

hierophect commented Oct 1, 2020

Uh oh!

hathach commented Oct 1, 2020

Uh oh!

microdev1 commented Oct 2, 2020

Uh oh!

hierophect commented Oct 5, 2020

Uh oh!

microdev1 commented Oct 5, 2020

Uh oh!

hierophect commented Oct 5, 2020

Uh oh!

hierophect commented Oct 5, 2020

Uh oh!

hathach commented Oct 5, 2020

Uh oh!

tannewt commented Oct 5, 2020

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hierophect commented Sep 30, 2020 •

edited

Loading

hathach commented Sep 30, 2020 •

edited

Loading

hierophect commented Sep 30, 2020 •

edited

Loading

hierophect commented Sep 30, 2020 •

edited

Loading

julianrendell commented Sep 30, 2020 •

edited

Loading

hierophect commented Sep 30, 2020 •

edited

Loading

julianrendell commented Sep 30, 2020 •

edited

Loading