Repository navigation
esp32 network: Add PPP over a UART - #2461
Conversation
|
Amp code with this promp: Not sure how hypothetical the teardown issue is, but the priority override seems nice to fix, your call: Review: ESP32 PPP over a UARTReviewed commit: RecommendationRequest changes. The PPP teardown can free an interface while lwIP still owns an active This review covers the commit against its first parent, including the C driver, Erlang An independent oracle review was requested and incorporated. In particular, the oracle Findings1. [P1] Do not destroy PPP after an expired graceful-close deadlineLocation: Trigger: Establish PPP, then stop the network while the peer is silent or has lost power
In the locally installed ESP-IDF v5.5.5:
Consequently, a pending lwIP termination timer can still reference the session and its Required fix: Keep the PPP session, netif, UART, and callback state alive until close is Regression check: Run an actual PPP peer, suppress its Terminate-Ack, stop the network, 2. [P1] Preserve automatic route selection instead of pinning PPP as defaultLocation: Trigger: Start STA or Ethernet together with PPP using the documented defaults, then
Thus PPP with priority 20 displaces Ethernet with priority 128 or STA with priority 100. Suggested direction: Reevaluate routes without a permanent override. Simply deleting Oracle caveat: An old queued success event may run after lwIP has already disconnected The following untested candidate checks the live link and refreshes routing atomically diff --git a/src/platforms/esp32/components/avm_builtins/network_driver.c b/src/platforms/esp32/components/avm_builtins/network_driver.c
--- a/src/platforms/esp32/components/avm_builtins/network_driver.c
+++ b/src/platforms/esp32/components/avm_builtins/network_driver.c
@@
+#ifdef HAVE_PPP
+static esp_err_t refresh_ppp_route(void *arg)
+{
+ esp_netif_t *netif = arg;
+ if (esp_netif_is_netif_up(netif)) {
+ esp_netif_action_connected(netif, NULL, 0, NULL);
+ }
+ return ESP_OK;
+}
+#endif
+
static void event_handler(void *arg, esp_event_base_t event_base, int32_t event_id, void *event_data)
@@
if (event_id == NETIF_PPP_ERRORNONE) {
ESP_LOGI(TAG, "PPP connected.");
if (data->ppp_glue != NULL) {
- esp_netif_set_default_netif(data->ppp_glue->base.netif);
+ esp_netif_tcpip_exec(refresh_ppp_route, data->ppp_glue->base.netif);
}Validate API availability and behavior on the supported SDK versions, including IPv6 and Regression check: Exercise PPP alone; STA/Ethernet before and after PPP; loss of the Small documentation suggestionExplain the UART ownership handoff after dialingLocation: The guide tells applications to dial using the same UART before starting PPP, but does not diff --git a/doc/src/network-programming-guide.md b/doc/src/network-programming-guide.md
--- a/doc/src/network-programming-guide.md
+++ b/doc/src/network-programming-guide.md
@@
configuration. The other end must already be ready for PPP: typically a cellular modem that the
application has powered on and dialed over the same UART (`ATD*99#` answered `CONNECT`) before
-calling `network:start/1`. The link negotiates its address and DNS servers. It can be used alone
+calling `network:start/1`. Close the dialing UART port with `uart:close/1` before starting PPP;
+the network driver installs and owns its own UART driver. Do not read from or write to that
+UART through another port while PPP is running.
+The link negotiates its address and DNS servers. It can be used alone
or next to `sta`, `ap` and `eth`; with its default `route_priority` of 20 it carries the trafficThis documents the existing ownership requirement; it does not prove that a close/reopen Validation and limits
|
network:start/1 takes {ppp, [{uart, N}, {tx, Pin}, {rx, Pin},
{speed, Baud}, {route_priority, P}]} and runs PPP on that UART with the
esp_netif PPP client, for a modem the application has dialed (it
answered CONNECT). Callbacks got_ip and disconnected.
lwIP sets the address before the link is up, so the address event finds
it down and the default netif is not updated: once the link is up the
driver runs esp_netif_action_connected on the tcpip thread, and the
interface with the highest route_priority that is up becomes the
default. Stopping closes the link (LCP terminate, so the modem hangs
up). If the peer does not answer within 3 s, network:stop/0 returns and
a task frees the link when lwIP reports it dead. Bytes in and out are
logged on close.
CONFIG_AVM_ENABLE_PPP, off by default, needs LWIP_PPP_SUPPORT and
selects LWIP_PPP_NOTIFY_PHASE_SUPPORT. The ESP32 build matrix gets a
PPP build, and the network guide a section.
Signed-off-by: Gabriel Mancini <gabriel.mancini@gmail.com>
2474f11 to
d7b3038
Compare
|
Thanks for the review, both points were real. Fixed in the last push: Teardown. Default route. I dropped The guide now says the application closes its UART before starting PPP. It also describes the default route rule and the background close. In CI, the size failure came from the PPP Tested on a T-Call (SIM800L, GPRS), with a test app:
|
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Follow-up review — PPP over a UARTReviewed head: Thanks for the fixes and hardware testing. Both previous P1 findings are addressed: the netif is no longer unconditionally destroyed after the three-second deadline, and routing no longer pins PPP as the default. The UART handoff documentation is also fixed, and the PPP-enabled ESP32/v5.5.5 CI job passed. An additional oracle review challenged the remaining lifetime concern and confirmed one conditional P2. This is not a regression introduced by the follow-up push; it also existed in the original PPP implementation. [P2] Drain queued PPP input before destroying its netifLocation: Trigger: Joining the RX task stops the producer; it does not drain input already submitted to lwIP. In ESP-IDF v5.5.5:
The possible ordering is RX queues input → RX signals stopped → teardown acquires the core mutex and frees the netif → TCP/IP processes queued input and dereferences the freed netif. Reaching PPP's DEAD phase does not drain that input queue. With core locking disabled, destruction is submitted behind previously queued input, so this particular interleaving does not apply. SDK evidence: Small suggested fixAfter joining RX, enqueue a real TCP/IP mailbox barrier before destroying the netif. The existing Do not substitute The following is a review-only candidate against this PR head, not an applied, compiled, or runtime-tested fix: diff --git a/src/platforms/esp32/components/avm_builtins/network_driver.c b/src/platforms/esp32/components/avm_builtins/network_driver.c
--- a/src/platforms/esp32/components/avm_builtins/network_driver.c
+++ b/src/platforms/esp32/components/avm_builtins/network_driver.c
@@ -67,6 +67,7 @@
#include <freertos/FreeRTOS.h>
#include <freertos/semphr.h>
#include <freertos/task.h>
+#include <lwip/tcpip.h>
#define HAVE_PPP 1
#endif
#pragma GCC diagnostic pop
@@ -1791,6 +1792,11 @@ error:
return false;
}
+static void ppp_rx_drained(void *arg)
+{
+ xSemaphoreGive((SemaphoreHandle_t) arg);
+}
+
// Frees what start_ppp set up, once the session is dead (or never started): lwIP keeps timers
// on a session until its dead phase, and the netif and its PPP control block must outlive them.
static void free_ppp(struct PPPGlue *glue)
@@ -1801,8 +1807,15 @@ static void free_ppp(struct PPPGlue *glue)
}
if (glue->running) {
ESP_LOGI(TAG, "PPP closed: %u bytes in, %u bytes out", (unsigned) glue->rx_bytes, (unsigned) glue->tx_bytes);
glue->stop = true;
xSemaphoreTake(glue->stopped, portMAX_DELAY);
+ // RX has stopped submitting input, but queued input still holds the netif.
+ // Use the mailbox even when esp_netif calls execute under the core lock.
+ if (tcpip_callback(ppp_rx_drained, glue->stopped) != ERR_OK) {
+ ESP_LOGE(TAG, "Failed to drain PPP input, left allocated");
+ return;
+ }
+ xSemaphoreTake(glue->stopped, portMAX_DELAY);
}
if (glue->stopped != NULL) {
vSemaphoreDelete(glue->stopped);
@@ -1827,9 +1840,9 @@ static void ppp_teardown_task(void *args)
{
struct PPPGlue *glue = args;
if (xSemaphoreTake(glue->died, pdMS_TO_TICKS(PPP_TEARDOWN_MS)) == pdTRUE) {
free_ppp(glue);
- ESP_LOGI(TAG, "PPP closed late, freed");
+ ESP_LOGI(TAG, "PPP late close teardown finished");
} else {
ESP_LOGE(TAG, "PPP did not close in %d ms, left allocated", PPP_TEARDOWN_MS);
}The failure return deliberately retains resources instead of freeing pointers potentially referenced by pending input. The final log adjustment avoids claiming the glue was freed if the barrier could not be allocated. The unconditional barrier is redundant but correct when core locking is disabled or input is synchronous; avoiding configuration guards keeps the lifetime invariant explicit. It adds one message allocation per started-session teardown. Posting can block on mailbox space, and waiting assumes a functioning TCP/IP task. This must run from the current teardown tasks, not from the TCP/IP task or while holding its core mutex. What otherwise looks correct
Validation and limitsThe oracle checked the relevant callback, queued-input, inline core-locking dispatch, PPP receive, and netif-dereference implementations in the SDK/lwIP revisions used by ESP-IDF v5.2.8, v5.3.6, v5.4.4, and v5.5.5. This is source-level evidence, not a hardware-reproduced crash. No new local firmware build or runtime test of the suggested patch was performed. The existing PPP-enabled CI job passed for the reviewed head; that does not exercise this teardown interleaving. Suggested regression check: enable core locking with input locking disabled, keep UART traffic arriving around the DEAD notification, and stop/restart repeatedly with heap poisoning enabled. Arrange pending RX messages before cleanup and verify they finish before destruction. Repeat with core locking disabled as a control, and cover barrier-allocation failure without early free. Recommendation: one targeted lifetime fix before approval; the two previous P1 findings can be considered addressed. |
|
This PR has been approved and merged locally, do not change. |
Adds PPP over a UART to the ESP32
networkmodule, for boards with a cellular modem on a UART (LilyGO T-Internet-COM, T-PCIE and T-Vending with a SIM7600 Mini PCIe module, T-Call with a SIM800L).network:start/1accepts{ppp, [...]}, alone or next tosta/ap/eth: the UART number, TX and RX pins,speedandroute_priority, and thegot_ipanddisconnectedcallbacks. The other end must already be ready for PPP: the application powers the modem and dials it on the same UART (ATD*99#answeredCONNECT) before starting the network, so the driver stays modem-agnostic. The link negotiates its address and DNS servers, and its netif becomes the default once it is up. With its defaultroute_priorityof 20 it carries the traffic only when Ethernet and the STA are down.network:stop/0closes the link with an LCP terminate, so the modem hangs up, and frees the UART.PPP is opt-in with
CONFIG_AVM_ENABLE_PPP, which needsCONFIG_LWIP_PPP_SUPPORT; otherwise apppentry returns{error, ppp_not_supported}.Built on top of #2419 (it shares
start_networkwith the Ethernet entry). The ESP32 build matrix gets a PPP build, and the network guide a section.Tested on hardware, with an application that dials the modem and falls back to WiFi: a SIM7600 on a T-Internet-COM and a T-PCIE (ESP32) and on a T-Vending (ESP32-S3), and a SIM800L (2G) on a T-Call (ESP32). A TLS session over the link for 11 hours overnight (no LCP errors, same address), link drops and redials,
network:stop/0with the link up and after the modem hung up, and OTA updates of the application over 2G (479 KB, with resumed downloads). No QEMU test: the suite has no peer for a PPP link on a UART.These changes are made under both the "Apache 2.0" and the "GNU Lesser General Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later