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

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.

Thanks, done.

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).

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
{
    ...

Done