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 c10b627ce6..738d06e7a0 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
@@ -27,15 +30,18 @@
 #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)
 
@@ -56,7 +62,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);
 
@@ -118,12 +124,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;
@@ -216,6 +222,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"));
     }
@@ -234,7 +251,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);
@@ -254,20 +276,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"));
         }
@@ -292,14 +329,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"));
     }
 
@@ -316,7 +355,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;
 }
@@ -330,16 +369,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) },
@@ -354,11 +478,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.

DvdGiessen · 2025-04-16T17:31:41Z

ports/esp32/network_ppp.c

+            val = self->stream;
+            break;
+        }
+        case MP_QSTR_ifname: {


I haven't yet ported over the SO_BINDTODEVICE support from #12062 to work on the other lwIP implementations. So this is indeed something that is still only supported on the ESP32.

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

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.54%. Comparing base (0b72962) to head (cea5f2c).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #17138   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files         169      169           
  Lines       21889    21889           
=======================================
  Hits        21570    21570           
  Misses        319      319

☔ 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

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>

DvdGiessen commented Apr 16, 2025

View reviewed changes

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DvdGiessen commented Apr 16, 2025 •

edited

Loading

DvdGiessen Apr 16, 2025

codecov bot commented Apr 16, 2025 •

edited

Loading

github-actions bot commented Apr 16, 2025

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

Are you sure you want to change the base?

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

Conversation

DvdGiessen commented Apr 16, 2025 • edited Loading

Summary

Testing

Trade-offs and Alternatives

DvdGiessen Apr 16, 2025

Choose a reason for hiding this comment

codecov bot commented Apr 16, 2025 • edited Loading

Codecov Report

github-actions bot commented Apr 16, 2025

DvdGiessen commented Apr 16, 2025 •

edited

Loading

codecov bot commented Apr 16, 2025 •

edited

Loading