esp32/network_ppp: Restructure to match extmod/network_ppp_lwip. #17138

DvdGiessen · 2025-04-16T17:30:30Z

Summary

The ESP32 PPP implementation predates the generic implementation in extmod. The new extmod implementation has a few advantages such as a better deinit procedure (the ESP32 implementation would not clean up properly and cause crashes if recreated) and using the UART IRQ functionality instead of running a task to read data from the UART.

This change restructures the ESP implementation to be much closer to the new extmod version, while also bringing a few tiny improvements from the ESP32 version to the extmod version. The diff between extmod/network_ppp_lwip.c and ports/esp32/network_ppp.c is now a small set of easy to review ESP32 port-specific changes.

Testing

I've been running these changes for months on a few custom boards (original ESP32 and ESP32S3) with three different modem chips to connect to the Internet via PPP. Those firmware versions do contain a bunch of other changes as well (for example #17135, which I highly recommend also gets considered if this PR gets merged since it fixes a broken behaviour in the UART that can now get triggered by this updated PPP version) so it's not a completely clean test. I'm fairly confident in the changes, and the approach makes sense, but I'd be happy to see other people giving it a go as well and letting me know if they spot anything.

Note for reviewers: Instead of reviewing the changes to the ESP32 version directly, it might be easier to review the differences to the extmod version first (they should make sense on their own) and then compare the diff between the extmod and ESP32 versions:

git diff --no-index extmod/network_ppp_lwip.c ports/esp32/network_ppp.c

diff --git a/extmod/network_ppp_lwip.c b/ports/esp32/network_ppp.c
index 2c3dac9201..8b700c98ef 100644
--- a/extmod/network_ppp_lwip.c
+++ b/ports/esp32/network_ppp.c
@@ -3,8 +3,11 @@
  *
  * The MIT License (MIT)
  *
+ * Copyright (c) 2018 "Eric Poulsen" <eric@zyxod.com>
  * Copyright (c) 2024 Damien P. George
  *
+ * Based on the ESP IDF example code which is Public Domain / CC0
+ *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
  * in the Software without restriction, including without limitation the rights
@@ -24,22 +27,25 @@
  * THE SOFTWARE.
  */
 
-// This file is intended to closely match ports/esp32/network_ppp.c. Changes can
+// This file is intended to closely match extmod/network_ppp_lwip.c. Changes can
 // and should probably be applied to both files. Compare them directly by using:
 // git diff --no-index extmod/network_ppp_lwip.c ports/esp32/network_ppp.c
 
 #include "py/runtime.h"
 #include "py/mphal.h"
 #include "py/stream.h"
-#include "extmod/modnetwork.h"
 
-#if MICROPY_PY_NETWORK_PPP_LWIP
+#if defined(CONFIG_ESP_NETIF_TCPIP_LWIP) && defined(CONFIG_LWIP_PPP_SUPPORT)
 
 #include "lwip/dns.h"
 #include "netif/ppp/ppp.h"
 #include "netif/ppp/pppapi.h"
 #include "netif/ppp/pppos.h"
 
+// Includes for port-specific changes compared to network_ppp_lwip.c
+#include "shared/netutils/netutils.h"
+#include "ppp_set_auth.h"
+
 // Enable this to see the serial data going between the PPP layer.
 #define PPP_TRACE_IN_OUT (0)
 
@@ -60,7 +66,7 @@ typedef struct _network_ppp_obj_t {
     struct netif netif;
 } network_ppp_obj_t;
 
-const mp_obj_type_t mp_network_ppp_lwip_type;
+const mp_obj_type_t esp_network_ppp_lwip_type;
 
 static mp_obj_t network_ppp___del__(mp_obj_t self_in);
 
@@ -122,12 +128,12 @@ static mp_obj_t network_ppp___del__(mp_obj_t self_in) {
             // Still connected over the stream.
             // Force the connection to close, with nocarrier=1.
             self->state = STATE_INACTIVE;
-            ppp_close(self->pcb, 1);
+            pppapi_close(self->pcb, 1);
         }
         network_ppp_stream_uart_irq_disable(self);
         // Free PPP PCB and reset state.
         self->state = STATE_INACTIVE;
-        ppp_free(self->pcb);
+        pppapi_free(self->pcb);
         self->pcb = NULL;
     }
     return mp_const_none;
@@ -220,6 +226,17 @@ static mp_obj_t network_ppp_config(size_t n_args, const mp_obj_t *args, mp_map_t
             val = self->stream;
             break;
         }
+        case MP_QSTR_ifname: {
+            if (self->pcb != NULL) {
+                struct netif *pppif = ppp_netif(self->pcb);
+                char ifname[NETIF_NAMESIZE + 1] = {0};
+                netif_index_to_name(netif_get_index(pppif), ifname);
+                if (ifname[0] != 0) {
+                    val = mp_obj_new_str_from_cstr((char *)ifname);
+                }
+            }
+            break;
+        }
         default:
             mp_raise_ValueError(MP_ERROR_TEXT("unknown config param"));
     }
@@ -238,7 +255,12 @@ static mp_obj_t network_ppp_status(mp_obj_t self_in) {
 }
 static MP_DEFINE_CONST_FUN_OBJ_1(network_ppp_status_obj, network_ppp_status);
 
-static u32_t network_ppp_output_callback(ppp_pcb *pcb, const void *data, u32_t len, void *ctx) {
+#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 4, 0)
+static u32_t network_ppp_output_callback(ppp_pcb *pcb, const void *data, u32_t len, void *ctx)
+#else
+static u32_t network_ppp_output_callback(ppp_pcb *pcb, u8_t *data, u32_t len, void *ctx)
+#endif
+{
     network_ppp_obj_t *self = ctx;
     #if PPP_TRACE_IN_OUT
     mp_printf(&mp_plat_print, "ppp_out(n=%u,data=", len);
@@ -258,20 +280,35 @@ static u32_t network_ppp_output_callback(ppp_pcb *pcb, const void *data, u32_t l
 }
 
 static mp_obj_t network_ppp_connect(size_t n_args, const mp_obj_t *args, mp_map_t *kw_args) {
-    enum { ARG_security, ARG_user, ARG_key };
+    enum { ARG_security, ARG_user, ARG_key, ARG_authmode, ARG_username, ARG_password };
     static const mp_arg_t allowed_args[] = {
-        { MP_QSTR_security, MP_ARG_KW_ONLY | MP_ARG_INT, {.u_int = PPPAUTHTYPE_NONE} },
+        { MP_QSTR_security, MP_ARG_KW_ONLY | MP_ARG_INT, {.u_int = -1} },
         { MP_QSTR_user, MP_ARG_KW_ONLY | MP_ARG_OBJ, {.u_rom_obj = MP_ROM_NONE} },
         { MP_QSTR_key, MP_ARG_KW_ONLY | MP_ARG_OBJ, {.u_rom_obj = MP_ROM_NONE} },
+        // Deprecated arguments for backwards compatibility
+        { MP_QSTR_authmode, MP_ARG_KW_ONLY | MP_ARG_INT, {.u_int = PPPAUTHTYPE_NONE} },
+        { MP_QSTR_username, MP_ARG_KW_ONLY | MP_ARG_OBJ, {.u_rom_obj = MP_ROM_NONE} },
+        { MP_QSTR_password, MP_ARG_KW_ONLY | MP_ARG_OBJ, {.u_rom_obj = MP_ROM_NONE} },
     };
 
     mp_arg_val_t parsed_args[MP_ARRAY_SIZE(allowed_args)];
     mp_arg_parse_all(n_args - 1, args + 1, kw_args, MP_ARRAY_SIZE(allowed_args), allowed_args, parsed_args);
 
+    // Use deprecated arguments as defaults
+    if (parsed_args[ARG_security].u_int == -1) {
+        parsed_args[ARG_security].u_int = parsed_args[ARG_authmode].u_int;
+    }
+    if (parsed_args[ARG_user].u_obj == mp_const_none) {
+        parsed_args[ARG_user].u_obj = parsed_args[ARG_username].u_obj;
+    }
+    if (parsed_args[ARG_key].u_obj == mp_const_none) {
+        parsed_args[ARG_key].u_obj = parsed_args[ARG_password].u_obj;
+    }
+
     network_ppp_obj_t *self = MP_OBJ_TO_PTR(args[0]);
 
     if (self->state == STATE_INACTIVE) {
-        self->pcb = pppos_create(&self->netif, network_ppp_output_callback, network_ppp_status_cb, self);
+        self->pcb = pppapi_pppos_create(&self->netif, network_ppp_output_callback, network_ppp_status_cb, self);
         if (self->pcb == NULL) {
             mp_raise_msg(&mp_type_OSError, MP_ERROR_TEXT("pppos_create failed"));
         }
@@ -296,14 +333,16 @@ static mp_obj_t network_ppp_connect(size_t n_args, const mp_obj_t *args, mp_map_
     if (parsed_args[ARG_security].u_int != PPPAUTHTYPE_NONE) {
         const char *user_str = mp_obj_str_get_str(parsed_args[ARG_user].u_obj);
         const char *key_str = mp_obj_str_get_str(parsed_args[ARG_key].u_obj);
-        ppp_set_auth(self->pcb, parsed_args[ARG_security].u_int, user_str, key_str);
+        pppapi_set_auth(self->pcb, parsed_args[ARG_security].u_int, user_str, key_str);
     }
 
-    ppp_set_default(self->pcb);
+    if (pppapi_set_default(self->pcb) != ERR_OK) {
+        mp_raise_msg(&mp_type_OSError, MP_ERROR_TEXT("ppp_set_default failed"));
+    }
 
     ppp_set_usepeerdns(self->pcb, true);
 
-    if (ppp_connect(self->pcb, 0) != ERR_OK) {
+    if (pppapi_connect(self->pcb, 0) != ERR_OK) {
         mp_raise_msg(&mp_type_OSError, MP_ERROR_TEXT("ppp_connect failed"));
     }
 
@@ -320,7 +359,7 @@ static mp_obj_t network_ppp_disconnect(mp_obj_t self_in) {
     network_ppp_obj_t *self = MP_OBJ_TO_PTR(self_in);
     if (self->state == STATE_CONNECTING || self->state == STATE_CONNECTED) {
         // Initiate close and wait for PPPERR_USER callback.
-        ppp_close(self->pcb, 0);
+        pppapi_close(self->pcb, 0);
     }
     return mp_const_none;
 }
@@ -334,16 +373,101 @@ static MP_DEFINE_CONST_FUN_OBJ_1(network_ppp_isconnected_obj, network_ppp_isconn
 
 static mp_obj_t network_ppp_ifconfig(size_t n_args, const mp_obj_t *args) {
     network_ppp_obj_t *self = MP_OBJ_TO_PTR(args[0]);
-    return mod_network_nic_ifconfig(&self->netif, n_args - 1, args + 1);
+    if (n_args == 1) {
+        // get
+        const ip_addr_t *dns;
+        if (self->pcb != NULL) {
+            dns = dns_getserver(0);
+            struct netif *pppif = ppp_netif(self->pcb);
+            mp_obj_t tuple[4] = {
+                netutils_format_ipv4_addr((uint8_t *)&pppif->ip_addr, NETUTILS_BIG),
+                netutils_format_ipv4_addr((uint8_t *)&pppif->gw, NETUTILS_BIG),
+                netutils_format_ipv4_addr((uint8_t *)&pppif->netmask, NETUTILS_BIG),
+                netutils_format_ipv4_addr((uint8_t *)dns, NETUTILS_BIG),
+            };
+            return mp_obj_new_tuple(4, tuple);
+        } else {
+            mp_obj_t tuple[4] = { mp_const_none, mp_const_none, mp_const_none, mp_const_none };
+            return mp_obj_new_tuple(4, tuple);
+        }
+    } else {
+        ip_addr_t dns;
+        mp_obj_t *items;
+        mp_obj_get_array_fixed_n(args[1], 4, &items);
+        #if CONFIG_LWIP_IPV6
+        netutils_parse_ipv4_addr(items[3], (uint8_t *)&dns.u_addr.ip4, NETUTILS_BIG);
+        #else
+        netutils_parse_ipv4_addr(items[3], (uint8_t *)&dns, NETUTILS_BIG);
+        #endif // CONFIG_LWIP_IPV6
+        dns_setserver(0, &dns);
+        return mp_const_none;
+    }
 }
 static MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(network_ppp_ifconfig_obj, 1, 2, network_ppp_ifconfig);
 
 static mp_obj_t network_ppp_ipconfig(size_t n_args, const mp_obj_t *args, mp_map_t *kwargs) {
     network_ppp_obj_t *self = MP_OBJ_TO_PTR(args[0]);
-    return mod_network_nic_ipconfig(&self->netif, n_args - 1, args + 1, kwargs);
+    if (kwargs->used == 0) {
+        if (self->pcb == NULL) {
+            mp_raise_ValueError(MP_ERROR_TEXT("PPP not active"));
+        }
+        struct netif *netif = ppp_netif(self->pcb);
+        // Get config value
+        if (n_args != 2) {
+            mp_raise_TypeError(MP_ERROR_TEXT("must query one param"));
+        }
+
+        switch (mp_obj_str_get_qstr(args[1])) {
+            case MP_QSTR_addr4: {
+                mp_obj_t tuple[2] = {
+                    netutils_format_ipv4_addr((uint8_t *)&netif->ip_addr, NETUTILS_BIG),
+                    netutils_format_ipv4_addr((uint8_t *)&netif->netmask, NETUTILS_BIG),
+                };
+                return mp_obj_new_tuple(2, tuple);
+            }
+            case MP_QSTR_gw4: {
+                return netutils_format_ipv4_addr((uint8_t *)&netif->gw, NETUTILS_BIG);
+            }
+            default: {
+                mp_raise_ValueError(MP_ERROR_TEXT("unexpected key"));
+                break;
+            }
+        }
+        return mp_const_none;
+    } else {
+        mp_raise_TypeError(MP_ERROR_TEXT("setting properties not supported"));
+    }
+    return mp_const_none;
 }
 static MP_DEFINE_CONST_FUN_OBJ_KW(network_ppp_ipconfig_obj, 1, network_ppp_ipconfig);
 
+static mp_obj_t network_ppp_active(size_t n_args, const mp_obj_t *args) {
+    network_ppp_obj_t *self = MP_OBJ_TO_PTR(args[0]);
+    if (n_args > 1) {
+        if (mp_obj_is_true(args[1])) {
+            if (self->state >= STATE_ACTIVE) {
+                return mp_const_true;
+            }
+
+            self->pcb = pppapi_pppos_create(&self->netif, network_ppp_output_callback, network_ppp_status_cb, self);
+            if (self->pcb == NULL) {
+                mp_raise_msg(&mp_type_OSError, MP_ERROR_TEXT("pppos_create failed"));
+            }
+            self->state = STATE_ACTIVE;
+
+            network_ppp_stream_uart_irq_enable(self);
+        } else {
+            if (self->state < STATE_ACTIVE) {
+                return mp_const_false;
+            }
+
+            network_ppp___del__(MP_OBJ_FROM_PTR(self));
+        }
+    }
+    return mp_obj_new_bool(self->state >= STATE_ACTIVE);
+}
+static MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(network_ppp_active_obj, 1, 2, network_ppp_active);
+
 static const mp_rom_map_elem_t network_ppp_locals_dict_table[] = {
     { MP_ROM_QSTR(MP_QSTR___del__), MP_ROM_PTR(&network_ppp___del___obj) },
     { MP_ROM_QSTR(MP_QSTR_config), MP_ROM_PTR(&network_ppp_config_obj) },
@@ -358,11 +482,17 @@ static const mp_rom_map_elem_t network_ppp_locals_dict_table[] = {
     { MP_ROM_QSTR(MP_QSTR_SEC_NONE), MP_ROM_INT(PPPAUTHTYPE_NONE) },
     { MP_ROM_QSTR(MP_QSTR_SEC_PAP), MP_ROM_INT(PPPAUTHTYPE_PAP) },
     { MP_ROM_QSTR(MP_QSTR_SEC_CHAP), MP_ROM_INT(PPPAUTHTYPE_CHAP) },
+
+    // Deprecated interface for backwards compatibility
+    { MP_ROM_QSTR(MP_QSTR_active), MP_ROM_PTR(&network_ppp_active_obj) },
+    { MP_ROM_QSTR(MP_QSTR_AUTH_NONE), MP_ROM_INT(PPPAUTHTYPE_NONE) },
+    { MP_ROM_QSTR(MP_QSTR_AUTH_PAP), MP_ROM_INT(PPPAUTHTYPE_PAP) },
+    { MP_ROM_QSTR(MP_QSTR_AUTH_CHAP), MP_ROM_INT(PPPAUTHTYPE_CHAP) },
 };
 static MP_DEFINE_CONST_DICT(network_ppp_locals_dict, network_ppp_locals_dict_table);
 
 MP_DEFINE_CONST_OBJ_TYPE(
-    mp_network_ppp_lwip_type,
+    esp_network_ppp_lwip_type,
     MP_QSTR_PPP,
     MP_TYPE_FLAG_NONE,
     make_new, network_ppp_make_new,

Trade-offs and Alternatives

I didn't fully replace the ESP32 version with the extmod version since there are distinct differences, and the ESP32 port does not use the lwIP version shipped by us but the modified version shipped as part of the ESP-IDF.

I've also aimed to keep the ESP32 version backwards compatible, thus it implements a few extra arguments and settings compared to the extmod version. Removing these deprecated arguments and porting the missing features over to the extmod version was out of scope for this PR, to keep it a bit easier to review.

We might want to add a message for future contributors to be aware that these two files are now very similar and that many changes could probably be applied to both.

ports/esp32/network_ppp.c

codecov · 2025-04-16T17:36:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.54%. Comparing base (2406582) to head (3b1e22c).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #17138   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files         169      169           
  Lines       21897    21897           
=======================================
  Hits        21579    21579           
  Misses        318      318

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2025-04-16T17:53:22Z

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

dpgeorge · 2025-05-14T02:36:28Z

We might want to add a message for future contributors to be aware that these two files are now very similar and that many changes could probably be applied to both.

Yes, I think that's a good idea.

Eventually would be good to have these two implementations share as much code as possible, but for now the PR here is a very good step in that direction.

The ESP32 PPP implementation predates the generic implementation in extmod. The new extmod implementation has a few advantages such as a better deinitialisation procedure (the ESP32 implemementation would not clean up properly and cause crashes if recreated) and using the UART IRQ functionality instead of running a task to read data from the UART. This change restructures the ESP implementation to be much closer to the new extmod version, while also bringing a few tiny improvements from the ESP32 version to the extmod version. The diff between extmod/network_ppp_lwip.c and ports/esp32/network_ppp.c is now a small set of easy to review ESP32 port-specific changes. Signed-off-by: Daniël van de Giessen <daniel@dvdgiessen.nl>

dpgeorge · 2025-05-15T02:04:57Z

using the UART IRQ functionality instead of running a task to read data from the UART.

That's a really neat improvement. At the very least it means one less FreeRTOS task, which is great!

dpgeorge

I haven't tested this, but reviewing the code it looks really good. I particularly like that:

it's much more maintainable moving forward, now that it more closely matches the extmod version
API now matches the docs
is backwards compatible (retains old keyword argument names, security constants, etc)
uses the UART IRQ instead of a separate polling task

Point (1) is particularly important, because PPP is quite hard to get right and modems can be very finicky. So it's great now that the code running on esp32 boards is also (indirectly) testing the extmod code, and vice versa.

As for testing this PR, the CI tests that this builds, and I trust your testing with actual PPP hardware.

Thank you very much!

DvdGiessen commented Apr 16, 2025

View reviewed changes

ports/esp32/network_ppp.c Show resolved Hide resolved

DvdGiessen force-pushed the esp32_ppp_rework_like_extmod branch from bc2be92 to 4434ade Compare April 16, 2025 17:42

DvdGiessen marked this pull request as ready for review April 16, 2025 17:55

DvdGiessen force-pushed the esp32_ppp_rework_like_extmod branch from 4434ade to cea5f2c Compare April 24, 2025 09:20

DvdGiessen mentioned this pull request Apr 27, 2025

ports/esp32: Updated network_ppp.c to match current documentation #17008

Closed

dpgeorge added the port-esp32 label May 7, 2025

DvdGiessen mentioned this pull request May 7, 2025

ports/esp32: Fix crash and improve timer interrupt allocation #17265

Merged

DvdGiessen force-pushed the esp32_ppp_rework_like_extmod branch 3 times, most recently from db139c8 to 8e60f20 Compare May 14, 2025 14:08

dpgeorge approved these changes May 15, 2025

View reviewed changes

dpgeorge force-pushed the esp32_ppp_rework_like_extmod branch from 8e60f20 to 3b1e22c Compare May 15, 2025 02:19

dpgeorge merged commit 3b1e22c into micropython:master May 15, 2025
94 of 96 checks passed

This was referenced May 15, 2025

ESP32: Guru Meditation some time after ppp.active(False) #17079

Open

PPP.active freezes MicroPython #15157

Open

DvdGiessen mentioned this pull request Jul 10, 2025

esp32/network_ppp: Bugfixes for deadlocks and crashes when disconnecting. #17656

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

esp32/network_ppp: Restructure to match extmod/network_ppp_lwip. #17138

esp32/network_ppp: Restructure to match extmod/network_ppp_lwip. #17138

Uh oh!

DvdGiessen commented Apr 16, 2025 •

edited

Loading

Uh oh!

Uh oh!

codecov bot commented Apr 16, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Apr 16, 2025

Uh oh!

dpgeorge commented May 14, 2025

Uh oh!

dpgeorge commented May 15, 2025

Uh oh!

dpgeorge left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

esp32/network_ppp: Restructure to match extmod/network_ppp_lwip. #17138

esp32/network_ppp: Restructure to match extmod/network_ppp_lwip. #17138

Uh oh!

Conversation

DvdGiessen commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Trade-offs and Alternatives

Uh oh!

Uh oh!

codecov bot commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Apr 16, 2025

Uh oh!

dpgeorge commented May 14, 2025

Uh oh!

dpgeorge commented May 15, 2025

Uh oh!

dpgeorge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DvdGiessen commented Apr 16, 2025 •

edited

Loading

codecov bot commented Apr 16, 2025 •

edited

Loading