[gabrielmancini]

Adds Ethernet to the ESP32 network module, for boards with a LAN8720-class PHY on the EMAC (Dingtian DT-R002, LilyGO T-Internet-COM).

network:start/1 accepts {eth, [...]}, alone or next to sta/ap: the MDIO and RMII clock pins, PHY power and reset, phy_addr, route_priority, dhcp_hostname, and the started, connected, disconnected and got_ip callbacks. network:wait_for_eth/0,1,2 and network:eth_status/0 mirror their STA counterparts, and mDNS binds to the Ethernet address when there is one. With both up, Ethernet carries the default route unless route_priority says otherwise.

Ethernet is opt-in with CONFIG_AVM_ENABLE_ETHERNET, on ESP-IDF 5.4 or newer; otherwise an eth entry returns {error, eth_not_supported}.

Commits:

1. Move the WiFi start of start_network into start_wifi (no behaviour change).
2. The Ethernet driver, with test_eth and test_eth_teardown in the QEMU suite.
3. Ethernet as the preferred default route.

Tested in QEMU (esp32) at each commit, and on a DT-R002 and a T-Internet-COM (link, DHCP, cable pull/plug, eth with sta).

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

[bettio]

The PR is good but it requires some improvements.

The biggest missing thing are tests, CI already runs an OpenCores Ethernet MAC on QEMU here for test_net, test_socket and test_ssl. Please make the new path testable on it: under CONFIG_ETH_USE_OPENETH, have start_eth create the MAC with esp_eth_mac_new_openeth and the PHY with esp_eth_phy_new_dp83848 at address 1, as eth_start() does, instead of esp_eth_mac_new_esp32. Then add a test_eth.erl sim test, gated like the other QEMU cases, that checks: network:wait_for_eth/1 returns {ok, IpInfo}; the callbacks arrive in order started, connected, got_ip; network:stop/0 followed by a second wait_for_eth succeeds (exercises stop_eth); and a run that returns without network:stop/0, as test_wifi_teardown does, so the driver destroy path is covered. Set autonego_timeout_ms to 100 as the harness does to keep it fast, and add CONFIG_AVM_ENABLE_ETHERNET=y to src/platforms/esp32/test/sdkconfig.defaults once the option exists.

Also about commit 185b311 (esp32 network: Add Ethernet (EMAC + RMII PHY)):

The extraction of start_wifi from start_network is a pure move mixed into the feature, so the start_network hunk reads as a move plus edits and cannot be checked mechanically. Please split it into its own commit ahead of this one (esp32 network: Refactor start_network into start_wifi, no behaviour change), as the git guide asks: Refactor first, then Feature. The body of this commit is good as it is.

See also my other comments

[gabrielmancini]

Thanks for the thorough review! Pushed a new version (force-push, rebased on release-0.7), now three commits:

1. esp32 network: Refactor start_network into start_wifi — pure move, no behaviour change (git diff --color-moved / --histogram shows it as a move).
2. esp32 network: Add Ethernet (EMAC + RMII PHY)
3. esp32 network: Prefer Ethernet for default route

Point by point:

• Kconfig: AVM_ENABLE_ETHERNET, default n, HAVE_ETH derives from it; #error below ESP-IDF 5.4. It depends on AVM_ENABLE_NETWORK_PORT_DRIVER && (ETH_USE_ESP32_EMAC || ETH_USE_OPENETH) — I added ETH_USE_OPENETH so the QEMU suite can enable it on every QEMU target, not only esp32. Happy to narrow it to EMAC only if you prefer. esp_eth stays in PRIV_REQUIRES.
• CI: new esp32 / v5.4.4 matrix entry appending CONFIG_AVM_ENABLE_ETHERNET=y to sdkconfig.defaults.in.
• QEMU tests: under CONFIG_ETH_USE_OPENETH, start_eth creates the OpenCores MAC and a DP83848 at address 1. test_eth.erl checks invalid configs return badarg, callbacks arrive in order started, connected, got_ip, eth_status/0, and network:stop/0 followed by wait_for_eth/0; test_eth_teardown.erl returns without network:stop/0 and runs twice, like test_wifi_teardown. CONFIG_AVM_ENABLE_ETHERNET=y added to the test sdkconfig.defaults. Locally (esp32 QEMU, IDF 5.5.2) both pass; test_socket fails here the same way on plain release-0.7, so it's my network, not this change.
• ESP32-P4: defaults come from ETH_ESP32_EMAC_DEFAULT_CONFIG(); the GPIO16/17 PSRAM logic is now under CONFIG_IDF_TARGET_ESP32.
• Chips without EMAC / option off: an eth entry, alone or next to sta/ap, returns {error, eth_not_supported}.
• Validation: power, reset, mdc, mdio must be valid output GPIOs, phy_addr -1 or 0..31, rmii_clock checked per chip (classic ESP32: in on 0, out on 0/16/17); otherwise {error, badarg}, before touching any pin. GPIO return codes are checked.
• Handles moved into struct ClientData, with the power pin.
• Error path: start_eth calls stop_eth() on failure; stop_eth resets the power pin. If esp_eth_driver_uninstall fails, it logs and keeps the handle.
• autonego_timeout_ms = 100; the docs say network:start/1 blocks until the PHY answers on SMI, not for a cable.
• power_pin → power, new reset option mapped to phy_config.reset_gpio_num.
• Type: {rmii_clock, {in | out, non_neg_integer()}}, chip constraints in docs and C.
• Docs: opt-in, ESP32 and ESP32-P4, IDF 5.4+, WiFi must stay enabled in sdkconfig; rmii_clock default documented as the sdkconfig choice; started is now "the interface is up, link state unknown".
• network.erl: eth_ip_info and eth_state (inactive | started | connected | disconnected) in the state, eth_status/0, wait_for_eth/0,1,2 plus an "Ethernet Convenience Functions" section; mDNS binds to the Ethernet address when there is one (and moves back to the STA's on eth_disconnected).
• CHANGELOG: says where it's available and how to turn it on.
• Nit on the route comment: rewritten.

@UncleGrumpy on interface priority: commit 3 adds {route_priority, 0..255} to the eth entry (default 128, over the STA's 100). Since STA and AP have fixed priorities (100 and 10), that's enough to put Ethernet either above or below WiFi. A route_priority for sta/ap too could come in a follow-up if you think it's worth it.

Not hardware-tested yet: the driver code is the same our boards run on our fork, but this new version will only be tested on real hardware (Dingtian DT-R002 and LilyGO T-Internet-COM) on Monday. I'll report the results here.

[UncleGrumpy]

Thank you! That’s great for now, we can always add interface priorities for WiFi later, in case someone wants to prefer AP over STA, but that seems like a rare case.

[gabrielmancini]

CI found real problems besides the Docker Hub rate limit; pushed a fix commit on top (happy to squash):

• IDF 5.2/5.3: the test sdkconfig turns CONFIG_AVM_ENABLE_ETHERNET on for every version and the driver hit its #error. Below 5.4 the option now has no effect (Ethernet compiled out, eth_not_supported), and the Ethernet tests are not built.
• ESP32-S3: GPIO_IS_VALID_GPIO/GPIO_IS_VALID_OUTPUT_GPIO shift 1ULL by the pin number, undefined from 64 up; GPIO99 passed as GPIO35. Pins are now bounded by GPIO_PIN_COUNT first, and the badarg cases use pins invalid on every chip.
• JIT-only builds: avm_network is not precompiled into esp32boot, so network cannot load; the Ethernet tests are skipped there.
• ESP32-C3: without SMP, messages sent from a task arrive in reverse order, so eth_connected came before eth_started. That is in libAtomVM, not in the driver: fixed in Fix order of messages sent from tasks #2423. Until it is merged the C3 job will still fail test_eth.

Checked locally in QEMU with the memory-check build (IDF 5.5.5): esp32, esp32s3 and esp32c3 (the last with #2423) pass test_eth and test_eth_teardown; the esp32 JIT build skips them; an IDF 5.3 build with the option on compiles.

The hardware test moves to Tuesday; I'll report here.

[github-advanced-security]

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:

• The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
• Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
• You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

[gabrielmancini]

Pushed 00961d8 for the ESP32-S3 USB CDC job: test_eth_teardown got the address from wait_for_eth/0 but eth_status/0 still said started. The STA handler sets sta_state to connected when it gets an IP; the Ethernet one now does the same, so a late eth_started (task messages can still arrive out of order until #2423) no longer leaves it at started.

In QEMU (memory-check build, IDF 5.5.5): esp32s3 passes test_eth and test_eth_teardown; esp32c3 without #2423 now passes test_eth_teardown, while test_eth still needs #2423 there, since it checks the order of the callbacks. The macOS job timed out in test_estdlib, which happened on other branches too.

Hardware test today; I'll report here.

[gabrielmancini]

Hardware test of 00961d8 (ESP-IDF 5.5.5, CONFIG_AVM_ENABLE_ETHERNET=y, plain ESP32) on two boards:

• Dingtian DT-R002: [{rmii_clock, {out, 17}}, {power, 0}, {phy_addr, 0}]
• LilyGO T-Internet-COM (LAN8720): [{rmii_clock, {out, 0}}, {power, 4}, {phy_addr, 0}]

Both with mdc 23 and mdio 18. The same test app ran on each, and all four steps passed on both:

1. network:start/1 with only eth: started, connected and got_ip arrive in that order, eth_status/0 returns connected, and a TCP connection to an outside host gets its local address from Ethernet. Then network:stop/0.
2. wait_for_eth/2, TCP, then network:stop/0, ten times: an address every time. Free heap after each stop drifts by about 20 bytes per cycle (DT-R002: 191712 to 191536 bytes over ten restarts).
3. Cable out: disconnected, and eth_status/0 returns disconnected. Cable back in: connected, got_ip, then TCP works again.
4. eth and sta together: with the cable in, TCP goes out through Ethernet. With the cable out, it goes through STA. With the cable back in, it goes through Ethernet again (default route_priority).

Each network:stop/0 logs Failed to remove multicast filter for IPv4 (from emac_esp32_rm_mac_filter). That comes from ESP-IDF, not from the driver: when the netif is added, lwIP joins the IGMP all-hosts group before esp_netif has its back-pointer. netif_igmp_mac_filter_cb then returns ERR_VAL without logging, so the filter is never added. When the netif is destroyed, the removal does not find the filter. It does no harm: the next start gets an address.

[gabrielmancini]

The two red jobs are the ones expected until #2423 is merged: on esp32c3 (IDF 5.4.4 and 5.5.5) test_eth checks the order of the started, connected and got_ip callbacks, and without SMP messages sent from a task arrive reversed (see my comment above). With #2423 applied it passes in QEMU; test_eth_teardown already passes there without it.

[bettio]

Few minor fixes are required.

[gabrielmancini]

Thanks! The three fixes are in 3e9b784, details in each thread.

About the CI run on e8b5bd7: test_eth failed once in the esp32 / v5.4.4 job (the one that also got ethernet: 'ON' and libsodium), about 30 ms after ETHERNET_EVENT_CONNECTED, so not a timeout. The same job passed on the previous head 3c58f24 (that push was only a rebase), the other 14 esp-idf jobs passed, and locally (QEMU, memory-check build, ESP-IDF v5.5.5) it passed 3 times in a row on esp32 and once on esp32c3 with these changes. I couldn't reproduce it; if it shows up again on this run I'll dig into which message the test got there.

[gabrielmancini]

Hardware test of 3e9b784 on a LilyGO T-Internet-COM (ESP32-WROVER-E, LAN8720, [{rmii_clock, {out, 0}}, {power, 4}, {phy_addr, 0}], mdc 23, mdio 18; ESP-IDF v5.5.5, CONFIG_AVM_ENABLE_ETHERNET=y), with the same test app as the earlier run, all four steps pass:

1. eth alone: started, connected and got_ip in order (address after 5.7 s), eth_status/0 returns connected, a TCP connection to an outside host gets its local address from Ethernet, then network:stop/0.
2. wait_for_eth/2, TCP and network:stop/0, ten times: an address every time; free heap after each stop goes from 191728 to 191560 bytes over the ten cycles.
3. Cable out: disconnected, eth_status/0 returns disconnected. Cable back in: connected, got_ip, TCP works again.
4. eth and sta together: TCP goes out through Ethernet, through STA with the cable out, and through Ethernet again with it back in.

No resets or watchdog. Each network:stop/0 still logs the ESP-IDF Failed to remove multicast filter for IPv4 explained in the earlier report.

[petermm]

late here, few AMP code review things, I suggest punting the mdns issue, and fix it later..

Review: AtomVM PR #2419

• PR: esp32 network: Add Ethernet (EMAC + RMII PHY)
• Reviewed head: 3e9b7847
• Base: release-0.7
• Method: Oracle review of the full diff and surrounding contracts, followed by inspection of the reported local code paths.
• Validation limits: These are source-level findings. No local hardware reproductions or packet captures were performed. Suggested patches below have not been applied or tested. The current ESP-IDF CI jobs pass, but do not cover these scenarios.

1. P2: sta_connect/1 silently removes Ethernet callbacks

Location: libs/avm_network/src/network.erl:1237–1263, with the resulting configuration stored at lines 830–838 and consumed by the Ethernet event handlers at lines 904–923.

Trigger: Start Ethernet alongside managed STA, then call network:sta_connect/1 with new WiFi credentials.

Problem: update_config/2 rebuilds the configuration with only STA, AP, mDNS and SNTP. The eth entry is discarded. Ethernet remains running, but subsequent connected, disconnected and got_ip callbacks silently stop firing because their configuration lookup returns undefined. The configuration is replaced even if the native WiFi connection attempt subsequently fails.

Suggested fix: Preserve the existing Ethernet configuration, as is already done for AP. This must not imply that sta_connect/1 reconfigures Ethernet hardware.

diff --git a/libs/avm_network/src/network.erl b/libs/avm_network/src/network.erl
@@
         AP =
             case proplists:get_value(ap, OldConfig) of
                 undefined -> [];
                 Ap -> {ap, Ap}
             end,
+        ETH =
+            case proplists:get_value(eth, OldConfig) of
+                undefined -> [];
+                Eth -> {eth, Eth}
+            end,
         MDNS =
@@
-        lists:flatten([STA, AP, MDNS, SNTP])
+        lists:flatten([STA, AP, ETH, MDNS, SNTP])

Regression coverage: Start managed STA and Ethernet with PID callbacks, change STA credentials through the public API, then exercise an Ethernet disconnect/reconnect and verify its callbacks still arrive. Cover a failed STA connection attempt as well.

2. P2: mDNS multicast replies can leave through the wrong interface

Location: libs/avm_network/src/network.erl:1141–1145, interacting with libs/avm_network/src/mdns.erl:79–92 and 313–315.

Trigger: Enable mDNS with Ethernet and STA on separate networks. Set Ethernet's {route_priority, 50}, below STA's 100. Send an mDNS query requesting a multicast response through Ethernet.

Problem: The new interface-selection logic prefers Ethernet whenever it has an address, regardless of route priority. The responder joins the multicast group on Ethernet and advertises the Ethernet address, but binds to INADDR_ANY and never selects an outgoing multicast interface. Multicast responses therefore follow the default route through WiFi and do not reach the Ethernet requester.

The oracle checked this against ESP-IDF v5.4.4's pinned lwIP:

• IP_ADD_MEMBERSHIP does not set IP_MULTICAST_IF.
• UDP transmission falls back to ordinary routing without an interface override.

Suggested fix: Explicitly select the outgoing multicast interface to match the responder's selected interface. Preserve the intended Ethernet preference rather than making it depend on the default route.

AtomVM currently exposes multicast membership but not IP_MULTICAST_IF. The following is an illustrative consumer-side diff, not a standalone fix. It requires implementing and documenting the corresponding socket option in the native socket backend and Erlang socket API first; the final option name and value representation should follow those APIs' conventions.

diff --git a/libs/avm_network/src/mdns.erl b/libs/avm_network/src/mdns.erl
@@
     {ok, Socket} = socket:open(inet, dgram, udp),
     ok = socket:setopt(Socket, {socket, reuseaddr}, true),
+    ok = socket:setopt(Socket, {ip, multicast_if}, Interface),
     ok = socket:setopt(Socket, {ip, add_membership}, #{
         multiaddr => ?MDNS_MULTICAST_ADDR, interface => Interface
     }),

Regression coverage: With STA as the default route and Ethernet selected for mDNS, capture a multicast query and response on Ethernet and verify the response stays on Ethernet and advertises its address. Also check the default Ethernet-preferred configuration and fallback to STA after Ethernet disconnects. This finding is source-supported, not packet-capture reproduced.

3. P2: wait_for_eth/2 hides startup failures as timeouts

Location: libs/avm_network/src/network.erl:389–401, interacting with handle_continue/2 at lines 814–820 and wait_for_ip/1 at lines 1276–1286.

Trigger: Call network:wait_for_eth([{phy_addr, 32}], 15000), or use wait_for_eth on a build without Ethernet support.

Problem: start/1 acknowledges the gen_server before its start_port continuation completes. The native driver rejects the configuration and the server exits with {start_port_failed, Reason}, but the helper neither monitors the server nor receives that failure. It waits the full timeout and returns {error, timeout}, hiding badarg, eth_not_supported, or a hardware error.

The existing STA helper has the same limitation; this PR copies it into the new Ethernet API.

Suggested fix: Start the convenience function's server with gen_server:start_monitor/4, which establishes the monitor atomically. Monitoring only after start/1 returns can lose the original exit reason. Handle the monitor in the wait loop and flush it on completion. The following localized suggestion leaves the existing STA helper unchanged.

diff --git a/libs/avm_network/src/network.erl b/libs/avm_network/src/network.erl
@@
-    case start([{eth, NewEthConfig}]) of
-        {ok, _Pid} ->
-            wait_for_ip(Timeout);
+    case gen_server:start_monitor({local, ?MODULE}, ?MODULE, [{eth, NewEthConfig}], []) of
+        {ok, {Pid, Monitor}} ->
+            try
+                wait_for_eth_ip(Timeout, Pid, Monitor)
+            after
+                erlang:demonitor(Monitor, [flush])
+            end;
         Error ->
             Error
     end.
@@
 %% @private
+wait_for_eth_ip(Timeout, Pid, Monitor) ->
+    receive
+        connected ->
+            wait_for_eth_ip(Timeout, Pid, Monitor);
+        {ok, _IpInfo} = Started ->
+            Started;
+        disconnected ->
+            {error, disconnected};
+        {'DOWN', Monitor, process, Pid, {start_port_failed, Reason}} ->
+            {error, Reason};
+        {'DOWN', Monitor, process, Pid, Reason} ->
+            {error, Reason}
+    after Timeout ->
+        {error, timeout}
+    end.
+
+%% @private
 wait_for_ip(Timeout) ->

Regression coverage: Through wait_for_eth/2, assert that invalid PHY addresses promptly return {error, badarg}, unsupported builds return {error, eth_not_supported}, and successful startup still returns the assigned address. Verify that completing the helper leaves no monitor or stale DOWN message. This suggestion preserves the existing timeout-reset-on-connected behavior rather than changing timeout semantics as part of this fix.

[gabrielmancini]

Thanks @petermm, all three checked against the code:

1. sta_connect/1 dropping eth: right, update_config/2 rebuilt the config without it, so a running Ethernet lost its callbacks. Fixed in 4da38ed: the eth entry is kept as it is (it doesn't touch the hardware). No test for this one: sta_connect/1 needs STA, which the QEMU harness doesn't have.
2. mDNS multicast interface: agreed to punt. It needs IP_MULTICAST_IF in the socket API first; I'll open an issue once this is in, with your notes.
3. wait_for_eth/2 hiding startup failures: fixed in 4da38ed as you suggested, with gen_server:start_monitor/4 and the monitor flushed on the way out. test_eth now checks that wait_for_eth([{phy_addr, 32}], 5000) returns {error, badarg} and leaves no DOWN behind. The STA helper keeps its current behavior here.

QEMU (memory-check build, ESP-IDF v5.5.5): esp32 and esp32c3 suites pass (test_eth included), except test_socket, which needs Internet hosts and fails here on plain release-0.7 too.

[gabrielmancini]

Rebased on release-0.7 (the conflict was the CHANGELOG only; the code is the reviewed one). Review comments from 10-16 are answered and no thread is open: could you take another look?

[bettio]

We are good at 99%, so mostly there, mostly some cleanup in comments, and let's cleanup history.

Briefly we should cleanup branch history as described in GIT_WORKFLOW_AND_HISTORY_STYLE_GUIDE.md:

• Four fix-up commits patch the feature commit 1ff89c9 within the same PR: f795569, ab94087, 2003816, f6c6384.
• The feature commit alone does not build. 1ff89c9 enables the Ethernet option in the test sdkconfig while the driver stops at #error below IDF 5.4, so the test app fails on 5.2 and 5.3 until f795569. That breaks bisect.
• Subject lines over the 50-character limit: f6c6384 at 68, 2003816 at 57, dfce29e at 53. One body line of 73 columns in f6c6384.
• Also I suggest to cleanup the PR description on GitHub, we use it as merge-commit body. I suggest to keep it shorter and it should work as a recap for the PR. So it should say what the PR does, briefly how, etc...

[gabrielmancini]

Thanks @bettio! History cleaned up and rebased on release-0.7:

• The four fix-ups are squashed into the Ethernet commit, so each commit builds: the IDF 5.2/5.3 guard is in the same commit that enables the option in the test sdkconfig. The route_priority test case moved to the route commit, where the option appears.
• Three commits now: the start_wifi move, the driver, and the default route. Subjects are 50 characters or less, bodies 72 columns.
• The PR description is now a short recap, for the merge commit.

The final tree is the same as before apart from the rebase; QEMU (esp32) passes at the driver commit and at the head.

[gabrielmancini]

Thanks @bettio! Removed the six comments, each from the commit that added it, so the history stays three commits that build:

• network.erl: the eth_rmii_clock_config() and eth_config_property() notes; the Ethernet table in the network programming guide has the same information.
• network_driver.c: the IDF 5.4 note above the #if, and the one above start_wifi.
• test_eth.erl: the two narration comments.

Comments only, the code is unchanged.

[bettio]

Approved. Do not change this PR: it has been already merged locally.