esp32: IDF v5.4(v5.5-dev) compile. #16565

IhorNehrutsa · 2025-01-10T12:08:07Z

Summary
This PR attempts to fix the esp32 compilation with IDF v5.4 (v5.5-dev)

Testing
ESP32 generic, ESP32-S3 boards tested with esp32/machine_pin: Add mode and pull in machine_pin_print() as repr() function. #12293.

codecov · 2025-01-10T12:10:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.58%. Comparing base (c69f0e4) to head (a4ab847).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #16565   +/-   ##
=======================================
  Coverage   98.58%   98.58%           
=======================================
  Files         167      167           
  Lines       21596    21596           
=======================================
  Hits        21291    21291           
  Misses        305      305

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

github-actions · 2025-01-10T12:16:14Z

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

DvdGiessen

I made the same changes locally to compile against IDF v5.4; found this PR so adding a few comments here instead of opening a new one.

I think this commit/PR should probably be referring to v5.4 instead of v5.5, since the latter is not out yet.

DvdGiessen · 2025-01-12T17:11:28Z

ports/esp32/network_ppp.c

@@ -145,7 +145,7 @@ static mp_obj_t ppp_active(size_t n_args, const mp_obj_t *args) {
                return mp_const_true;
            }

-            self->pcb = pppapi_pppos_create(&self->pppif, ppp_output_callback, ppp_status_cb, self);
+            self->pcb = pppapi_pppos_create(&self->pppif, (void *)ppp_output_callback, ppp_status_cb, self);


Instead of erasing the callback type, since the function is IDF-specific anyway wouldn't it be better to update its signature? That way we'll still get compiler errors if there's potentially incompatible changes in the future.

diff --git a/ports/esp32/network_ppp.c b/ports/esp32/network_ppp.c index c739264b9..a6e3b152f 100644 --- a/ports/esp32/network_ppp.c +++ b/ports/esp32/network_ppp.c @@ -101,7 +101,7 @@ static mp_obj_t ppp_make_new(mp_obj_t stream) { } MP_DEFINE_CONST_FUN_OBJ_1(esp_network_ppp_make_new_obj, ppp_make_new); -static u32_t ppp_output_callback(ppp_pcb *pcb, u8_t *data, u32_t len, void *ctx) { +static u32_t ppp_output_callback(ppp_pcb *pcb, const void *data, u32_t len, void *ctx) { ppp_if_obj_t *self = ctx; mp_obj_t stream = self->stream; @@ -110,7 +110,7 @@ static u32_t ppp_output_callback(ppp_pcb *pcb, u8_t *data, u32_t len, void *ctx) } int err; - return mp_stream_rw(stream, data, len, &err, MP_STREAM_RW_WRITE); + return mp_stream_rw(stream, (void *)data, len, &err, MP_STREAM_RW_WRITE); } static void pppos_client_task(void *self_in) {

Thanks, done.

IhorNehrutsa · 2025-01-13T11:26:02Z

ports/esp32/network_wlan.c

@@ -770,7 +770,9 @@ static const mp_rom_map_elem_t wlan_if_locals_dict_table[] = {
 };
 static MP_DEFINE_CONST_DICT(wlan_if_locals_dict, wlan_if_locals_dict_table);

-#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 3, 0)
+#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 4, 0)
+_Static_assert(WIFI_AUTH_MAX == 16, "Synchronize WIFI_AUTH_XXX constants with the ESP-IDF. Look at esp-idf/components/esp_wifi/include/esp_wifi_types_generic.h");


Thanks, done.

dpgeorge · 2025-01-12T23:11:59Z

py/persistentcode.c

@@ -402,7 +402,7 @@ static mp_raw_code_t *load_raw_code(mp_reader_t *reader, mp_module_context_t *co

    #if MICROPY_EMIT_MACHINE_CODE
    } else {
-        const uint8_t *prelude_ptr;
+        const uint8_t *prelude_ptr = NULL;


This change is unrelated to this PR, and shouldn't be needed.

I discovered independently this change is required to prevent an error while building against IDFv5.4 with a compiler error; perhaps a change in the compiler version included used by the IDF caused this issue to be exposed?

Without this change:

-- The C compiler identification is GNU 14.2.0 (...) [1325/1467] Building C object esp-idf/main_esp32s3/CMakeFiles/__idf_main_esp32s3.dir/build/micropython/py/persistentcode.c.obj FAILED: esp-idf/main_esp32s3/CMakeFiles/__idf_main_esp32s3.dir/build/micropython/py/persistentcode.c.obj (...) /build/micropython/py/persistentcode.c: In function 'load_raw_code': /build/micropython/py/persistentcode.c:439:38: error: 'prelude_ptr' may be used uninitialized [-Werror=maybe-uninitialized] 439 | children[n_children] = (void *)prelude_ptr; | ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~ /build/micropython/py/persistentcode.c:405:24: note: 'prelude_ptr' was declared here 405 | const uint8_t *prelude_ptr; // = NULL; | ^~~~~~~~~~~ cc1: some warnings being treated as errors (...) ninja failed with exit code 1

EDIT: Although I agree it shouldn't be needed; reading it myself it appears correct regardless of which ifdef block is used. Not sure why the compiler disagrees.

Although I agree it shouldn't be needed; reading it myself it appears correct regardless of which ifdef block is used. Not sure why the compiler disagrees.

Yeah, it looks like a compiler optimisation bug here.

I'm using gcc 14.2.1 and arm-none-eabi-gcc 14.2.0 here, and neither of them complain (with MICROPY_EMIT_NATIVE_PRELUDE_SEPARATE_FROM_MACHINE_CODE enabled).

In the end, are we forced to leave this fix as is?

Yes, we can leave this change in. It doesn't affect any other ports (the code size diff is zero for them all).

dpgeorge · 2025-01-12T23:13:36Z

@IhorNehrutsa when you open a pull request, can you please make sure you fill in the GitHub PR template, with a summary, testing that you did, and a list of trade-offs/alternatives. Otherwise we may just close the PR.

IhorNehrutsa · 2025-01-13T11:35:18Z

@dpgeorge

@IhorNehrutsa when you open a pull request, can you please make sure you fill in the GitHub PR template, with a summary, testing that you did, and a list of trade-offs/alternatives. Otherwise we may just close the PR.

The latest line in the template is

Delete this heading if not relevant (i.e. small fixes) -->

It seems that these small fixes are described in the PR/commit title.

Thanks.

dpgeorge · 2025-01-14T04:08:37Z

The latest line in the template is

Delete this heading if not relevant (i.e. small fixes) -->

That line is only referring to the Trade-offs and Alternatives heading, not the entire template.

Please can you update the top post of this PR with the following template filled out:

### Summary

<!-- Explain the reason for making this change. What problem does the pull request
     solve, or what improvement does it add? Add links if relevant. -->


### Testing

<!-- Explain what testing you did, and on which boards/ports. If there are
     boards or ports that you couldn't test, please mention this here as well.

     If you leave this empty then your Pull Request may be closed. -->

DvdGiessen

Combined changes seem correct to me, but the commits can perhaps be organized to provide a bit more context to future readers.

The first commit message still mentions this is for IDFv5.5 which isn't out yet; I think it should only mention v5.4?
The other two commits could probably be squashed into the first one; they belong together after all.
It might make sense to split out the change to py/persistentcode.c into a separate commit and add a short comment that this is to work around a compiler optimization bug in the compiler used by ESP-IDFv5.4. That way it'll be clear why that change is there, instead of being grouped in with the ESP32 port-specific changes. The change itself seems harmless enough to me; and with a clear separate commit documenting the reason for it I think it should be alright.

IhorNehrutsa · 2025-01-21T08:44:31Z

@DvdGiessen
Thanks for the detailed review.

dpgeorge · 2025-01-23T02:32:39Z

ports/esp32/network_ppp.c

@@ -100,7 +100,7 @@ static mp_obj_t ppp_make_new(mp_obj_t stream) {
 }
 MP_DEFINE_CONST_FUN_OBJ_1(esp_network_ppp_make_new_obj, ppp_make_new);

-static u32_t ppp_output_callback(ppp_pcb *pcb, u8_t *data, u32_t len, void *ctx) {
+static u32_t ppp_output_callback(ppp_pcb *pcb, const void *data, u32_t len, void *ctx) {


This change now gives a compiler warning for IDF 5.2.2 (at least, probably also 5.3). So I suggest having something like:

#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 4, 0) static u32_t ppp_output_callback(ppp_pcb *pcb, const void *data, u32_t len, void *ctx) #else static u32_t ppp_output_callback(ppp_pcb *pcb, u8_t *data, u32_t len, void *ctx) #endif { ...

dpgeorge

Thanks for updating, this looks good now.

Add WIFI_AUTH_WPA3_ENTERPRISE and WIFI_AUTH_WPA2_WPA3_ENTERPRISE, and update PPP callback signature for latest lwIP. Co-authored-by: Daniel van de Giessen <daniel@dvdgiessen.nl> Signed-off-by: IhorNehrutsa <Ihor.Nehrutsa@gmail.com>

The esp32 IDF toolchain can give a "may be used uninitialized" warning, at least for ESP32-S3 with gcc 14.2.0. Silence that warning by initializing the variable with NULL. Co-authored-by: Daniel van de Giessen <daniel@dvdgiessen.nl> Signed-off-by: IhorNehrutsa <Ihor.Nehrutsa@gmail.com>

dpgeorge · 2025-01-24T12:57:44Z

I updated the esp32 README to note that IDF v5.4 is now supported.

IhorNehrutsa · 2025-01-24T18:59:10Z

Thanks, Daniel.
Thanks, Damien.

IhorNehrutsa mentioned this pull request Jan 10, 2025

esp32/machine_pin: Add mode and pull in machine_pin_print() as repr() function. #12293

Draft

DvdGiessen suggested changes Jan 12, 2025

View reviewed changes

dpgeorge added the port-esp32 label Jan 12, 2025

dpgeorge reviewed Jan 12, 2025

View reviewed changes

IhorNehrutsa changed the title ~~esp32: IDF v.5.5 compile.~~ esp32: IDF v5.4(v5.5) compile. Jan 13, 2025

IhorNehrutsa force-pushed the esp32_idf_5_5 branch 2 times, most recently from 1212006 to 70ffd8e Compare January 20, 2025 07:56

IhorNehrutsa requested a review from DvdGiessen January 20, 2025 08:09

IhorNehrutsa force-pushed the esp32_idf_5_5 branch from 70ffd8e to 3a9088c Compare January 20, 2025 09:33

DvdGiessen reviewed Jan 20, 2025

View reviewed changes

IhorNehrutsa force-pushed the esp32_idf_5_5 branch from 3a9088c to 74cf76b Compare January 21, 2025 07:56

IhorNehrutsa changed the title ~~esp32: IDF v5.4(v5.5) compile.~~ esp32: IDF v5.4(v5.5-dev) compile. Jan 21, 2025

IhorNehrutsa force-pushed the esp32_idf_5_5 branch 3 times, most recently from 2dcdcaa to 46ee241 Compare January 21, 2025 08:20

IhorNehrutsa requested a review from DvdGiessen January 21, 2025 08:44

dpgeorge reviewed Jan 23, 2025

View reviewed changes

IhorNehrutsa force-pushed the esp32_idf_5_5 branch from 46ee241 to 9ef55a6 Compare January 23, 2025 08:34

IhorNehrutsa requested a review from dpgeorge January 23, 2025 08:37

dpgeorge approved these changes Jan 23, 2025

View reviewed changes

dpgeorge added this to the release-1.25.0 milestone Jan 23, 2025

IhorNehrutsa and others added 2 commits January 24, 2025 23:17

esp32: Add support for IDF v5.4.

865a4c8

Add WIFI_AUTH_WPA3_ENTERPRISE and WIFI_AUTH_WPA2_WPA3_ENTERPRISE, and update PPP callback signature for latest lwIP. Co-authored-by: Daniel van de Giessen <daniel@dvdgiessen.nl> Signed-off-by: IhorNehrutsa <Ihor.Nehrutsa@gmail.com>

dpgeorge force-pushed the esp32_idf_5_5 branch from 9ef55a6 to a4ab847 Compare January 24, 2025 12:32

dpgeorge merged commit a4ab847 into micropython:master Jan 24, 2025
64 checks passed

dpgeorge mentioned this pull request Feb 28, 2025

ESP32-P4 #16830

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esp32: IDF v5.4(v5.5-dev) compile. #16565

esp32: IDF v5.4(v5.5-dev) compile. #16565

IhorNehrutsa commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

github-actions bot commented Jan 10, 2025

DvdGiessen left a comment

DvdGiessen Jan 12, 2025

IhorNehrutsa Jan 13, 2025

This comment was marked as resolved.

IhorNehrutsa Jan 13, 2025

dpgeorge Jan 12, 2025

DvdGiessen Jan 12, 2025 •

edited

Loading

dpgeorge Jan 12, 2025

IhorNehrutsa Jan 13, 2025

dpgeorge Jan 22, 2025

dpgeorge commented Jan 12, 2025

IhorNehrutsa commented Jan 13, 2025 •

edited

Loading

dpgeorge commented Jan 14, 2025

DvdGiessen left a comment

IhorNehrutsa commented Jan 21, 2025

dpgeorge Jan 23, 2025

IhorNehrutsa Jan 23, 2025

dpgeorge left a comment

dpgeorge commented Jan 24, 2025

IhorNehrutsa commented Jan 24, 2025

esp32: IDF v5.4(v5.5-dev) compile. #16565

esp32: IDF v5.4(v5.5-dev) compile. #16565

Conversation

IhorNehrutsa commented Jan 10, 2025 • edited Loading

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

github-actions bot commented Jan 10, 2025

DvdGiessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DvdGiessen Jan 12, 2025 • 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

dpgeorge commented Jan 12, 2025

IhorNehrutsa commented Jan 13, 2025 • edited Loading

dpgeorge commented Jan 14, 2025

DvdGiessen left a comment

Choose a reason for hiding this comment

IhorNehrutsa commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge left a comment

Choose a reason for hiding this comment

dpgeorge commented Jan 24, 2025

IhorNehrutsa commented Jan 24, 2025

IhorNehrutsa commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

DvdGiessen Jan 12, 2025 •

edited

Loading

IhorNehrutsa commented Jan 13, 2025 •

edited

Loading