Add independent Ethernet/WiFi IP config and primary network interface selection#5650
Add independent Ethernet/WiFi IP config and primary network interface selection#5650kelvtech-co-uk wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds Ethernet static IP/GW/SN state and persistence, UI to configure Ethernet IP and choose a primary interface, init/event wiring to apply static Ethernet addressing, and lwIP-based default-netif selection for ESP32 dual-interface setups. ChangesEthernet Static IP & Primary Interface Configuration
Sequence DiagramsequenceDiagram
participant Settings as Settings UI
participant SetHandler as handleSettingsSet()
participant Config as cfg.json
participant InitEth as initEthernet()
participant ETH as ETH Driver
participant WiFi as WiFi STA
participant SetPrimary as setPrimaryNetworkInterface()
participant lwIP as lwIP Stack
Settings->>SetHandler: POST EIP*,EGW*,ESN*,EPI
SetHandler->>SetHandler: parse to ethStaticIP/GW/SN and ethPrimaryInterface
SetHandler->>Config: write cfg.json
InitEth->>ETH: call ETH.config(static|DHCP)
ETH->>SetPrimary: ETH_CONNECTED / ETH_GOT_IP
WiFi->>SetPrimary: WIFI_STA_GOT_IP
SetPrimary->>lwIP: netif_set_default(selected)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/network.cpp`:
- Around line 314-353: The function setPrimaryNetworkInterface walks netif_list
and calls netif_set_default without holding the lwIP TCP/IP core lock, causing
thread-safety assertions under CONFIG_LWIP_CHECK_THREAD_SAFETY; wrap the
traversal and the netif_set_default call with the lwIP core lock (include
"lwip/tcpip.h" and call LOCK_TCPIP_CORE(); ... UNLOCK_TCPIP_CORE()) to protect
access to netif_list and netif_set_default, and ensure any early returns still
release the lock; additionally make the interface-name matching in
setPrimaryNetworkInterface more robust by falling back to checking netif_is_up()
and a non-zero IPv4 address if the 'en'/'st' prefix check on netif->name[0..1]
fails.
In `@wled00/set.cpp`:
- Around line 165-169: The code reads the wrong POST key ("EGI") when setting
ethPrimaryInterface, so ethPrimaryInterface always stays false; update the
request->arg call used to set ethPrimaryInterface to use the correct form key
"EPI" (i.e., change the argument passed to request->arg in the
ethPrimaryInterface assignment), keep the same toInt() == 1 check and leave
initEthernet() as-is so the corrected value persists and Ethernet initialization
runs with the intended primary-interface selection.
In `@wled00/wled.cpp`:
- Around line 889-901: There are duplicate assignments to interfacesInited and
wasConnected; remove the first pair (the assignments immediately before the `#if`
block) and keep the single assignment after the ARDUINO_ARCH_ESP32 &&
WLED_USE_ETHERNET conditional so interfacesInited and wasConnected are set once
(leave setPrimaryNetworkInterface() and the final interfacesInited = true;
wasConnected = true; intact).
In `@wled00/xml.cpp`:
- Around line 322-334: The current dual-IP check uses the config flag
ethStaticIP (symbol: ethStaticIP) which hides DHCP-assigned Ethernet addresses;
change the condition to use live interface IPs instead — e.g., test
ETH.localIP() and WiFi.localIP() (or their uint32_t equivalents) rather than
ethStaticIP/ethernetType so both active interfaces are detected; update the
branch that constructs the IP string (functions/variables: ETH.localIP(),
WiFi.localIP(), Network.localIP(), Network.isEthernet()) to rely on those
runtime checks and remove dependence on ethStaticIP.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25568589-10b6-4906-b56c-039e39bb4f83
📒 Files selected for processing (8)
wled00/cfg.cppwled00/data/settings_wifi.htmwled00/fcn_declare.hwled00/network.cppwled00/set.cppwled00/wled.cppwled00/wled.hwled00/xml.cpp
softhack007
added
the
AI
kelvtech-co-uk
force-pushed
the
feature/eth-dual-interface
branch
from
f9320d8 to
f021c81
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/set.cpp (1)
165-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix primary-interface POST key mismatch (
EGIvsEPI).Line 168 reads
EGI, but the form submitsEPI, so the primary-interface selection does not persist correctly.Suggested fix
- ethPrimaryInterface = (request->arg(F("EGI")).toInt() == 1); + ethPrimaryInterface = (request->arg(F("EPI")).toInt() == 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/set.cpp` around lines 165 - 169, The code reads the wrong POST key ("EGI") so the primary-interface selection doesn't persist; update the logic that sets ethPrimaryInterface (currently using request->arg(F("EGI")).toInt() == 1) to use the correct form key "EPI" and also guard with request->hasArg(F("EPI")) (or equivalent) to handle radio-button submission semantics before calling initEthernet(); adjust only the key and add the hasArg check in the ethPrimaryInterface assignment site.
🧹 Nitpick comments (1)
wled00/fcn_declare.h (1)
303-306: ⚡ Quick winRemove the duplicated
initEthernet()declaration.Line 303 already declares
initEthernet(), so the duplicate at Line 305 is unnecessary and adds maintenance drift risk.Suggested fix
bool initEthernet(); // result is informational `#if` defined(ARDUINO_ARCH_ESP32) && defined(WLED_USE_ETHERNET) -bool initEthernet(); // byClaude: sets lwIP primary network interface based on ethPrimaryInterface // by enabling wled to be dual-homed we need to be deterministic about which interface // wled uses for outgoing connections to minimise asyncronous routing issues. void setPrimaryNetworkInterface(); `#endif`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/fcn_declare.h` around lines 303 - 306, Remove the duplicate declaration of initEthernet(): keep a single prototype for the function (the one already declared at the top) and delete the second repeated line that appears with the comment "byClaude: sets lwIP primary network interface based on ethPrimaryInterface", ensuring only one bool initEthernet(); remains in fcn_declare.h to avoid redundancy and maintenance drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/wled.h`:
- Around line 393-402: Wrap the AI-generated network config declarations (the
block that defines ethStaticIP, ethStaticGW, ethStaticSN and
ethPrimaryInterface) with the required C++ comment delimiters, e.g. add a
starting comment like "// AI: below section was generated by an AI" immediately
before the WLED_GLOBAL ethStaticIP declaration and an ending comment like "//
AI: end" immediately after the WLED_GLOBAL ethPrimaryInterface line so the
entire block (ethStaticIP, ethStaticGW, ethStaticSN, ethPrimaryInterface) is
enclosed by the AI markers.
---
Duplicate comments:
In `@wled00/set.cpp`:
- Around line 165-169: The code reads the wrong POST key ("EGI") so the
primary-interface selection doesn't persist; update the logic that sets
ethPrimaryInterface (currently using request->arg(F("EGI")).toInt() == 1) to use
the correct form key "EPI" and also guard with request->hasArg(F("EPI")) (or
equivalent) to handle radio-button submission semantics before calling
initEthernet(); adjust only the key and add the hasArg check in the
ethPrimaryInterface assignment site.
---
Nitpick comments:
In `@wled00/fcn_declare.h`:
- Around line 303-306: Remove the duplicate declaration of initEthernet(): keep
a single prototype for the function (the one already declared at the top) and
delete the second repeated line that appears with the comment "byClaude: sets
lwIP primary network interface based on ethPrimaryInterface", ensuring only one
bool initEthernet(); remains in fcn_declare.h to avoid redundancy and
maintenance drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e368d689-67b4-4512-8b0c-8d79c30c8f59
📒 Files selected for processing (8)
wled00/cfg.cppwled00/data/settings_wifi.htmwled00/fcn_declare.hwled00/network.cppwled00/set.cppwled00/wled.cppwled00/wled.hwled00/xml.cpp
kelvtech-co-uk
force-pushed
the
feature/eth-dual-interface
branch
from
f021c81 to
9513309
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/xml.cpp (1)
324-331:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse live Ethernet state for the dual-IP branch.
Line 324 keys this off
ethStaticIP, so DHCP-assigned Ethernet sessions still fall back to the single-IP path even whenETH.localIP()is valid.Suggested fix
- if (ethernetType != WLED_ETH_NONE && (uint32_t)ethStaticIP != 0 && - WiFi.localIP() != (uint32_t)0) { - // both interfaces active - IPAddress ethIP = ETH.localIP(); - IPAddress wifiIP = WiFi.localIP(); + IPAddress ethIP = ETH.localIP(); + IPAddress wifiIP = WiFi.localIP(); + if (ethernetType != WLED_ETH_NONE && + ethIP != (uint32_t)0 && + wifiIP != (uint32_t)0) { + // both interfaces active snprintf_P(s, sizeof(s), PSTR("%d.%d.%d.%d (ETH) / %d.%d.%d.%d (WiFi)"), ethIP[0], ethIP[1], ethIP[2], ethIP[3], wifiIP[0], wifiIP[1], wifiIP[2], wifiIP[3]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/xml.cpp` around lines 324 - 331, The dual-IP branch currently gates on ethStaticIP which misses DHCP Ethernet addresses; change the condition to check the actual live Ethernet IP (use ETH.localIP() or compare ETH.localIP() != (uint32_t)0) instead of ethStaticIP so DHCP-assigned ETH sessions take the dual-IP path; update the if that references ethernetType, ethStaticIP and WiFi.localIP() to use ETH.localIP() availability and keep the same snprintf_P block that uses ETH.localIP() and WiFi.localIP().
🧹 Nitpick comments (1)
wled00/wled.cpp (1)
718-735: ⚡ Quick winWrap these AI-generated C++ blocks with the standard begin/end markers.
The inline
// AI:notes don't satisfy the repo convention for larger generated sections.As per coding guidelines:
Embed larger blocks of AI-generated code within // AI: below section was generated by an AI ... // AI: end comments.Also applies to: 890-896, 1001-1007
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/wled.cpp` around lines 718 - 735, Wrap the AI-generated C++ sections in wled.cpp with the repository's standard AI markers by adding a start comment (e.g., "// AI: below section was generated by an AI - begin") immediately before the block beginning at the if (multiWiFi[selectedWiFi].staticIP != 0U) that contains the WiFi.config(...) and DEBUG_PRINTF_P(...) lines, and an end comment (e.g., "// AI: end") immediately after that block; do the same for the other two AI blocks referenced around lines 890-896 and 1001-1007 so that each generated region that touches multiWiFi, WiFi.config, or DEBUG_PRINTF_P is clearly delimited with the standard begin/end AI comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/network.cpp`:
- Around line 324-361: setPrimaryNetworkInterface() currently only picks the
preferred prefix (eth "en" or wifi "st") and leaves the lwIP default unchanged
if that interface is down or has no IP; modify it so after acquiring the TCPIP
core lock (LOCK_TCPIP_CORE()) you first search netif_list for a ready interface
matching the preferred prefix (using netif_is_up() and checking
netif_iter->ip_addr.u_addr.ip4.addr != 0), and if none found do a second-pass
over netif_list to select any other interface that is up and has an IP as a
fallback, then call netif_set_default(target) when you have a valid target; keep
the existing DEBUG_PRINTF_P/DEBUG_PRINTLN messages and use ip_addr.u_addr.ip4
with ip4_addr1..4 to log the chosen address.
---
Duplicate comments:
In `@wled00/xml.cpp`:
- Around line 324-331: The dual-IP branch currently gates on ethStaticIP which
misses DHCP Ethernet addresses; change the condition to check the actual live
Ethernet IP (use ETH.localIP() or compare ETH.localIP() != (uint32_t)0) instead
of ethStaticIP so DHCP-assigned ETH sessions take the dual-IP path; update the
if that references ethernetType, ethStaticIP and WiFi.localIP() to use
ETH.localIP() availability and keep the same snprintf_P block that uses
ETH.localIP() and WiFi.localIP().
---
Nitpick comments:
In `@wled00/wled.cpp`:
- Around line 718-735: Wrap the AI-generated C++ sections in wled.cpp with the
repository's standard AI markers by adding a start comment (e.g., "// AI: below
section was generated by an AI - begin") immediately before the block beginning
at the if (multiWiFi[selectedWiFi].staticIP != 0U) that contains the
WiFi.config(...) and DEBUG_PRINTF_P(...) lines, and an end comment (e.g., "//
AI: end") immediately after that block; do the same for the other two AI blocks
referenced around lines 890-896 and 1001-1007 so that each generated region that
touches multiWiFi, WiFi.config, or DEBUG_PRINTF_P is clearly delimited with the
standard begin/end AI comments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5f32be6-ef47-4688-87b3-7df31626be8f
📒 Files selected for processing (8)
wled00/cfg.cppwled00/data/settings_wifi.htmwled00/fcn_declare.hwled00/network.cppwled00/set.cppwled00/wled.cppwled00/wled.hwled00/xml.cpp
… selection - Add separate static IP/gateway/subnet configuration for Ethernet interface independent of WiFi static IP configuration - Add primary network interface selection to control which interface lwIP uses for outbound connections (MQTT, NTP, internet access) - Implement setPrimaryNetworkInterface() using netif_set_default() to set lwIP default netif based on user selection - Keep both interfaces active simultaneously when Ethernet connects - Show both interface IPs in UI when both are active - Update WiFi & Network settings UI with Ethernet IP config section and Primary Network Interface selector - Ethernet IP config section hidden when Ethernet Type is None Resolves: ethernet static IP not applying independently of WiFi config Related: wled#5247 Known limitations: - mDNS resolves to primary interface IP only; per-interface mDNS hostname registration is a candidate for follow-on work - Dual interface operation has been validated on boards using ETH_CLOCK_GPIO0_IN (external clock source). Boards using ETH_CLOCK_GPIO17_OUT may experience instability when WiFi and Ethernet operate simultaneously due to ESP32 PLL instability described in wled#4703. A future enhancement could detect the board clock mode and conditionally disable WiFi on affected boards, preserving existing single-interface behaviour for those users.
kelvtech-co-uk
force-pushed
the
feature/eth-dual-interface
branch
from
9513309 to
015e2e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
wled00/wled.cpp (1)
718-737: ⚡ Quick winUse the repo’s exact AI fence comments.
These markers add extra text (
...,comments), so they no longer match the documented// AI: below section was generated by an AI/// AI: endformat.Suggested normalization
-// AI: below section was generated by an AI ... +// AI: below section was generated by an AI ... -// AI: end comments +// AI: endAs per coding guidelines "Embed larger blocks of AI-generated code within
// AI: below section was generated by an AI...// AI: endcomments".Also applies to: 892-903, 1004-1013
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/wled.cpp` around lines 718 - 737, The AI fence comments are not using the repo's exact markers and include extra text; locate the AI-generated blocks around the WiFi static IP code (identify by WiFi.config(...) usage and multiWiFi[selectedWiFi] references) and replace any altered/extra comment lines with the exact markers "// AI: below section was generated by an AI" at the start and "// AI: end" at the end (no extra ellipses or words); apply the same normalization to the other AI blocks noted (around the regions containing WiFi.config/multiWiFi at the other ranges).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/cfg.cpp`:
- Around line 138-153: Wrap the two AI-generated ethernet config blocks with the
repository's AI delimiters by adding the start marker comment before the block
that deserializes eth_ip/eth_gw/eth_sn and the end marker after the
CJSON(ethPrimaryInterface, ethernet[F("epi")]) line; specifically enclose the
code that references eth_ip, eth_gw, eth_sn, ethStaticIP, ethStaticGW,
ethStaticSN and the CJSON call for ethPrimaryInterface (ethernet[F("epi")])
between the comments "// AI: below section was generated by an AI" and "// AI:
end" so both the larger deserialization block and the primary interface line are
fully wrapped.
In `@wled00/network.cpp`:
- Around line 279-299: The diff contains large AI-generated blocks (e.g., the
ethernet static/DHCP block that calls ETH.config(ethStaticIP, ethStaticGW,
ethStaticSN, dnsAddress) and the corresponding DHCP branch using INADDR_NONE)
that are not wrapped with the required AI delimiters; ensure you surround each
such larger AI-generated section with the comment markers // AI: below section
was generated by an AI and // AI: end (the function
setPrimaryNetworkInterface() is already wrapped, so apply the same wrapper to
the ethernet static/DHCP block shown and the other large AI-generated block
around the code referenced near the later region that corresponds to the 551-575
change), placing the start marker immediately before the AI block and the end
marker immediately after it so all large AI-inserted sections follow the coding
guideline.
In `@wled00/set.cpp`:
- Around line 150-168: Wrap the new Ethernet POST parsing block (the code that
checks request->hasArg(F("EIP0")) and fills ethStaticIP, ethStaticGW,
ethStaticSN, and sets ethPrimaryInterface) with the required AI section
delimiters by adding a leading comment "// AI: below section was generated by an
AI" immediately before the existing block and a trailing comment "// AI: end"
immediately after it, preserving all existing logic and indentation so the
parsing of EIP#, EGW#, ESN# and the EPI radio-button handling remains unchanged.
---
Nitpick comments:
In `@wled00/wled.cpp`:
- Around line 718-737: The AI fence comments are not using the repo's exact
markers and include extra text; locate the AI-generated blocks around the WiFi
static IP code (identify by WiFi.config(...) usage and multiWiFi[selectedWiFi]
references) and replace any altered/extra comment lines with the exact markers
"// AI: below section was generated by an AI" at the start and "// AI: end" at
the end (no extra ellipses or words); apply the same normalization to the other
AI blocks noted (around the regions containing WiFi.config/multiWiFi at the
other ranges).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31dad564-0c87-47bb-a407-504dc4f3e459
📒 Files selected for processing (8)
wled00/cfg.cppwled00/data/settings_wifi.htmwled00/fcn_declare.hwled00/network.cppwled00/set.cppwled00/wled.cppwled00/wled.hwled00/xml.cpp
@kelvtech-co-uk one important rule - even when its non-technical - is to not https://github.com/wled/WLED?tab=contributing-ov-file#during-review I'll restart the bot for this PR to fix it - you may receive some duplicate comments from coderabbit. |
|
@coderabbitai full review please create a prioritized list of suggested improvements and bugfixes after your review (major items only). |
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== repo / branch ==="
git status --short --branch
echo
echo "=== changed files against base ==="
BASE_REF="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null || true)"
if [ -n "${BASE_REF}" ]; then
git diff --name-status "${BASE_REF}"...HEAD
else
git diff --name-status origin/main...HEAD || true
fi
echo
echo "=== unified diff for reviewed files ==="
if [ -n "${BASE_REF}" ]; then
git diff --unified=80 "${BASE_REF}"...HEAD -- \
wled00/cfg.cpp \
wled00/data/settings_wifi.htm \
wled00/fcn_declare.h \
wled00/network.cpp \
wled00/set.cpp \
wled00/wled.cpp \
wled00/wled.h \
wled00/xml.cpp
else
git diff --unified=80 origin/main...HEAD -- \
wled00/cfg.cpp \
wled00/data/settings_wifi.htm \
wled00/fcn_declare.h \
wled00/network.cpp \
wled00/set.cpp \
wled00/wled.cpp \
wled00/wled.h \
wled00/xml.cpp || true
fi
echo
echo "=== relevant symbol locations ==="
rg -n -C 5 'setPrimaryNetworkInterface|ethStaticIP|ethStaticGW|ethStaticSN|ethPrimaryInterface|ETH\.config|ETH_GOT_IP|WIFI_STA_GOT_IP|WiFi\.config|Network\.localIP|EIP0|EPI|toggleEthIP|ethip' wled00 || trueLength of output: 148514 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== repo / branch ==="
git status --short --branch
echo
echo "=== changed files against base ==="
BASE_REF="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null || true)"
if [ -n "${BASE_REF}" ]; then
git diff --name-status "${BASE_REF}"...HEAD
else
git diff --name-status origin/main...HEAD || true
fi
echo
echo "=== unified diff for reviewed files ==="
if [ -n "${BASE_REF}" ]; then
git diff --unified=80 "${BASE_REF}"...HEAD -- \
wled00/cfg.cpp \
wled00/data/settings_wifi.htm \
wled00/fcn_declare.h \
wled00/network.cpp \
wled00/set.cpp \
wled00/wled.cpp \
wled00/wled.h \
wled00/xml.cpp
else
git diff --unified=80 origin/main...HEAD -- \
wled00/cfg.cpp \
wled00/data/settings_wifi.htm \
wled00/fcn_declare.h \
wled00/network.cpp \
wled00/set.cpp \
wled00/wled.cpp \
wled00/wled.h \
wled00/xml.cpp || true
fi
echo
echo "=== relevant symbol locations ==="
rg -n -C 5 'setPrimaryNetworkInterface|ethStaticIP|ethStaticGW|ethStaticSN|ethPrimaryInterface|ETH\.config|ETH_GOT_IP|WIFI_STA_GOT_IP|WiFi\.config|Network\.localIP|EIP0|EPI|toggleEthIP|ethip' wled00 || trueLength of output: 148514 ✅ Actions performedFull review triggered. |
coderabbitai
Bot
added
the
connectivity
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/network.cpp (1)
190-195:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
initEthernet()one-shot guard blocks applying updated Ethernet config.After first success, this early return prevents reapplying changed Ethernet IP/GW/SN from settings saves, so users can persist values that won’t take effect until reboot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/network.cpp` around lines 190 - 195, The static one-shot guard successfullyConfiguredEthernet in initEthernet() prevents reapplying updated Ethernet IP/GW/SN from settings; remove or change this behavior so new settings take effect: either delete the early return and rely on initEthernet() being idempotent, or replace the boolean check with a comparison between the saved settings (IP, gateway, subnet) and the currently applied Ethernet values (e.g., Ethernet.localIP(), gatewayIP(), subnetMask()) and only skip re-init when they match; alternatively, when settings are saved, reset/clear successfullyConfiguredEthernet so initEthernet() will re-run. Ensure you update initEthernet() and any settings-save code paths so changed IP/GW/SN are applied without a reboot.
🧹 Nitpick comments (1)
wled00/xml.cpp (1)
335-340: 💤 Low valueUse
snprintf_Pfor consistency and safety.Lines 335 and 340 use
sprintf()while line 330 usessnprintf_P(). For consistency and to prevent potential buffer overflows in future modifications, usesnprintf_Pthroughout.♻️ Suggested fix
} else { IPAddress localIP = Network.localIP(); - sprintf(s, "%d.%d.%d.%d", localIP[0], localIP[1], localIP[2], localIP[3]); + snprintf_P(s, sizeof(s), PSTR("%d.%d.%d.%d"), localIP[0], localIP[1], localIP[2], localIP[3]); if (Network.isEthernet()) strcat_P(s, PSTR(" (Ethernet)")); } `#else` IPAddress localIP = Network.localIP(); - sprintf(s, "%d.%d.%d.%d", localIP[0], localIP[1], localIP[2], localIP[3]); + snprintf_P(s, sizeof(s), PSTR("%d.%d.%d.%d"), localIP[0], localIP[1], localIP[2], localIP[3]); `#endif`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/xml.cpp` around lines 335 - 340, Replace the unsafe/ inconsistent sprintf calls that format the IP into buffer s with snprintf_P to match the existing usage (and avoid overflow); specifically change the two occurrences that use sprintf(s, "%d.%d.%d.%d", localIP[0], ...) in xml.cpp to use snprintf_P(s, sizeof buffer, PSTR("%d.%d.%d.%d"), localIP[0], ...), and when appending the Ethernet label use strcat_P only after ensuring snprintf_P left space (or include the label in the snprintf_P format when Network.isEthernet() is true) so that formatting is done safely and consistently with the earlier snprintf_P usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/cfg.cpp`:
- Around line 135-155: The deserialization block is only guarded by
WLED_USE_ETHERNET but accesses globals and functions declared under
ARDUINO_ARCH_ESP32 && WLED_USE_ETHERNET (ethStaticIP, ethStaticGW, ethStaticSN,
ethPrimaryInterface, initEthernet), which can break non-ESP32 builds; wrap the
portions that reference those ESP32-only symbols in a combined compile guard
(e.g., `#if` defined(ARDUINO_ARCH_ESP32) && defined(WLED_USE_ETHERNET)) or split
the logic so generic JSON parsing of ethernet["type"] remains under
WLED_USE_ETHERNET while the code that reads eth_ip/eth_gw/eth_sn, assigns
ethStaticIP/ethStaticGW/ethStaticSN, sets ethPrimaryInterface, and calls
initEthernet is only compiled when ARDUINO_ARCH_ESP32 is defined, ensuring no
ESP32-only globals are referenced on other platforms.
In `@wled00/network.cpp`:
- Around line 322-336: The selection is fragile because it matches
netif_iter->name against hardcoded prefixes via targetName; instead, track and
use esp-netif handles or impl names provided by ESP-NETIF APIs rather than
parsing lwIP netif->name. Replace the prefix check around netif_iter (and any
use of targetName) by either (a) keeping the esp_netif_t returned when creating
the WiFi/ethernet interfaces and comparing netif_iter to
esp_netif_get_netif_impl_handle(...) (i.e., use the stored esp_netif_t handle
for the preferred interface) or (b) call
esp_netif_get_netif_impl_name(esp_netif_handle) to get the canonical impl name
and compare that, and then call netif_set_default() under LOCK_TCPIP_CORE() for
the chosen esp_netif/netif; remove reliance on literal "st"/"en" checks
(symbols: targetName, netif_iter, netif_list, LOCK_TCPIP_CORE,
netif_set_default, esp_netif_get_netif_impl_name, esp_netif_t).
---
Outside diff comments:
In `@wled00/network.cpp`:
- Around line 190-195: The static one-shot guard successfullyConfiguredEthernet
in initEthernet() prevents reapplying updated Ethernet IP/GW/SN from settings;
remove or change this behavior so new settings take effect: either delete the
early return and rely on initEthernet() being idempotent, or replace the boolean
check with a comparison between the saved settings (IP, gateway, subnet) and the
currently applied Ethernet values (e.g., Ethernet.localIP(), gatewayIP(),
subnetMask()) and only skip re-init when they match; alternatively, when
settings are saved, reset/clear successfullyConfiguredEthernet so initEthernet()
will re-run. Ensure you update initEthernet() and any settings-save code paths
so changed IP/GW/SN are applied without a reboot.
---
Nitpick comments:
In `@wled00/xml.cpp`:
- Around line 335-340: Replace the unsafe/ inconsistent sprintf calls that
format the IP into buffer s with snprintf_P to match the existing usage (and
avoid overflow); specifically change the two occurrences that use sprintf(s,
"%d.%d.%d.%d", localIP[0], ...) in xml.cpp to use snprintf_P(s, sizeof buffer,
PSTR("%d.%d.%d.%d"), localIP[0], ...), and when appending the Ethernet label use
strcat_P only after ensuring snprintf_P left space (or include the label in the
snprintf_P format when Network.isEthernet() is true) so that formatting is done
safely and consistently with the earlier snprintf_P usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88d30fab-1ff8-41b9-9d51-2f0a6ffb541f
📒 Files selected for processing (8)
wled00/cfg.cppwled00/data/settings_wifi.htmwled00/fcn_declare.hwled00/network.cppwled00/set.cppwled00/wled.cppwled00/wled.hwled00/xml.cpp
@coderabbitai clarification: |
|
Tip For best results, initiate chat on the files or code changes.
P2 (revised) — Review documentation quality within AI-generated blocks
(ᵔ◡ᵔ)∫ ✏️ Learnings added
|
|
Fixes pushed for 3 of the 4 issues flagged by CodeRabbit.
@coderabbitai review |
|
The remaining open item from the priority list is P1 — Triggering a full re-review now to pick up the new changes. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/network.cpp (1)
322-336:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid hardcoded lwIP netif name prefixes (
"en"/"st") for primary-interface selection.Relying on
netif->nameprefixes is fragile and can break across SDK/core variations. Please switch to a stable ESP-NETIF-backed mapping (or an internal abstraction that resolves the intended netif robustly) before callingnetif_set_default().For Arduino-ESP32 / ESP-IDF, is there a documented stable guarantee that lwIP netif names remain "en" for Ethernet and "st" for WiFi STA across versions? If not, what ESP-NETIF API should be used to map/select the correct interface for default routing?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/network.cpp` around lines 322 - 336, Current code picks the default netif by checking lwIP netif->name prefix via targetName and comparing netif_iter->name, which is fragile; instead, keep LOCK_TCPIP_CORE() but replace the name-prefix logic with an ESP-NETIF-backed mapping: for each netif_iter call esp_netif_get_handle_from_netif(netif_iter) to obtain the esp_netif handle and then use esp_netif_get_desc() or esp_netif_get_ifkey() (or other esp_netif type/role query) to determine whether the interface is Ethernet or WiFi STA, set isPreferred based on that result, and only then call netif_set_default(netif_iter); remove reliance on targetName and string-index checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/set.cpp`:
- Around line 166-193: The code only calls setPrimaryNetworkInterface() inside
the ethIPChanged branch so toggling the EPI radio alone doesn't apply
immediately; modify the logic to also call setPrimaryNetworkInterface() when
ethPrimaryInterface has changed even if ethIPChanged is false—i.e., compare the
new ethPrimaryInterface to the previously active primary network state (read the
current primary interface or cached value) and invoke
setPrimaryNetworkInterface() when that boolean differs; keep existing
ETH.config(...) behavior inside the ethIPChanged branch so IP reconfiguration
still only happens when ethIPChanged is true.
---
Duplicate comments:
In `@wled00/network.cpp`:
- Around line 322-336: Current code picks the default netif by checking lwIP
netif->name prefix via targetName and comparing netif_iter->name, which is
fragile; instead, keep LOCK_TCPIP_CORE() but replace the name-prefix logic with
an ESP-NETIF-backed mapping: for each netif_iter call
esp_netif_get_handle_from_netif(netif_iter) to obtain the esp_netif handle and
then use esp_netif_get_desc() or esp_netif_get_ifkey() (or other esp_netif
type/role query) to determine whether the interface is Ethernet or WiFi STA, set
isPreferred based on that result, and only then call
netif_set_default(netif_iter); remove reliance on targetName and string-index
checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7411c529-2de6-4e6c-9b34-09fe28fd7f36
📒 Files selected for processing (8)
wled00/cfg.cppwled00/data/settings_wifi.htmwled00/fcn_declare.hwled00/network.cppwled00/set.cppwled00/wled.cppwled00/wled.hwled00/xml.cpp
coderabbitai
Bot
added
the
hardware
|
@softhack007 I've draft code changes to address the last priority item that CodeRabbit has flagged but I'm stuck hunting a bug from testing on my gledopto so it might be a couple more days. Do I need to be concerned about a release window you work to i.e. ideally get this PR final by X date? |
|
I don't know whether I'm Claude's puppet or he's mine but sharing verbaitm our current stubling block in the hope someone might have relevant experience to share... During testing of setPrimaryNetworkInterface() we observed different netif_is_up() behaviour between our two test boards: WT32-ETH01 — netif_is_up() correctly returns true when ethernet is connected and has a valid IP. The function works as expected and setPrimaryNetworkInterface() correctly switches the default netif to ethernet. Gledopto GL-C-618WL — netif_is_up() consistently returned false (up=0) even when ethernet was physically connected and passing traffic (Hyperion/E1.31 ambilight working correctly over the same interface). We were unable to determine conclusively whether this is a hardware-specific behaviour of the Gledopto's LAN8720 driver, a timing issue in our test setup, or an artefact of our unusual network configuration described below. Our Gledopto test environment involves a non-standard stub network topology — an isolated 192.168.10.0/24 subnet between the Gledopto and a CoreELEC media player, with ConnMan blacklisted from managing the ethernet interface and IP assignment handled manually via ifconfig as part of a systemd ExecStartPre script. This causes the ethernet interface to flap briefly during CoreELEC's boot sequence, and our UDP debug logging (which requires WiFi to be up) made it impossible to observe a clean stable ethernet state. We believe this fringe case is clouding the netif_is_up() question. The Gledopto's inaccessibility at 192.168.10.2 from other subnets is a routing/asymmetric path issue specific to this network topology rather than a bug in our code — the same address was accessible when running the original Gledopto firmware with no WiFi and a direct gateway configured. We are deferring the Gledopto-specific investigation and leaving netif_is_up() unchanged. If anyone has experience with netif_is_up() behaviour on other ESP32 ethernet boards we'd welcome input. |
… setPrimaryNetworkInterface()
|
The following changes have been made since the last review:
Expect at least 1 further commit for this PR adding a UI indicator showing active vs configured primary network interface. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Look at the current setPrimaryNetworkInterface implementation in network.cpp
rg -n -C 5 'setPrimaryNetworkInterface|esp_netif_next|esp_netif_get_desc|esp_netif_defaults|netif_set_default|LOCK_TCPIP|UNLOCK_TCPIP' wled00/network.cppLength of output: 7199
What's working well:
One minor advisory for future-proofing: Triggering a full re-review to pick up the latest changes. ✅ Actions performedReview triggered.
|
…nterface with fallback warning PNI selection is only applicable when both interfaces have a gateway configured. Without gateways on both interfaces, outbound traffic routing is unaffected by the PNI setting regardless of which interface is selected.
|
Okay I think this is enough for this PR. @coderabbitai review |
|
✅ Actions performedReview triggered.
|
Solved... In single interface ethernet builds (WiFi disabled), lwIP's default netif is always ethernet — replies to inbound connections naturally return via the same interface. In dual interface configurations (WiFi + Ethernet), WLED may use WiFi as the default netif depending on PNI configuration and interface availability. This means that if your ethernet interface is on an isolated subnet and you want it reachable from other subnets via static routing, you must configure a gateway IP on the ethernet interface pointing to your router — leaving it as 0.0.0.0 will cause reply packets to route via WiFi instead, making the ethernet IP unreachable from remote subnets even with correct static routes in place. In my case a Gledopto GL-C-618WL on an isolated 192.168.10.0/24 stub network became reachable at 192.168.10.2 from my main LAN only after configuring 192.168.10.1 (the stub network router) as the ethernet gateway. With 0.0.0.0 the device was unreachable despite correct static routing on the Unifi gateway. |
This is the expected behaviour of an IP stack: it uses the only route it knows about to route reply packets back to the source address, which leaves only the wifi if you configure no routes on the ethernet. IP packets do not contain the addresses of the routers that passed them through -- the device has no idea the packet was routed by "192.168.10.1" on its way to your WLED device, all it knows is that it's supposed to reply to "192.168.x.100" or whatever your client IP is. So it is not possible to "reply by the interface you received a packet on" unless you tell your device that there's a router on that subnet that can reach the source network. In the general case, you can of course also add explicit static routes, informing the stack that "IP ranges in the 192.168.0.0/16 block can be reached by sending packets to 192.168.10.1" without saying "any address can be reached by sending packets to 192.168.10.1" (ie. setting a default route aka "gateway" on that interface) -- though honestly I think that supplying interfaces to such advanced networking features is beyond the scope of WLED. |
Thanks, I was caught out by this not being the case with v0.15.4. I have a static route on my router for the 192.168.10.0/24 range sending it to my coreelec player, this is then onward routing that subnet to the directly connected gledopto. With only ethernet enabled and statically configured with no gateway I could browse to WLEDs ethernet stub address from remote subnets. This wasnt the case with my code changes and I stupidly lost time to an obvious check i should have done upfront.
100% agee, static routing or supporting topologies like mine are niche and shouldn't be a focus for the project. Caveat that Id love this PR to be merged 🤞🏻 |
3 participants
Caution
The majority of this code was generated using claude.ai, I don't have the skills or experience in langages to do this myself unfortunately. However I'm keen to learn so feedback welcomed on how I can improve all of this.
Motivation
I needed a low latency network connection between my Gledopto GL-C-618WL and my media player running Hyperion. However I was unable to provide a wired LAN connection from my network switch due to both devices location. I hacked a solution by modifying my media players CoreELEC network configuration and directly cabling the devices. In this configuration WLED was only useable whilst my media player was turned on as it became the router between my main LAN and the stub network I hacked into CoreELEC. I wanted a better solution and modifying WLED's firmware was easier than modifying my house to run a cable.
Core Changes
Resolves
Issue #5247 - ethernet static IP not applying independently of WiFi config
Related
Issue #4703 - ESP-NOW + Ethernet GPIO17 PLL instability (referenced in known limitations)
File Changes
wled00/wled.h— new global variableswled00/cfg.cpp— serialisation/deserialisation of new ethernet IP configwled00/set.cpp— form POST handler for new UI fieldswled00/xml.cpp— s.js population of new UI fields, dual IP displaywled00/network.cpp— ETH.config() with new variables, setPrimaryNetworkInterface(), WiFi kept active on ETH connectwled00/wled.cpp— WiFi config() update, setPrimaryNetworkInterface() call in handleConnection()wled00/fcn_declare.h— declaration of setPrimaryNetworkInterface()wled00/data/settings_wifi.htm— Ethernet IP Configuration section, Primary Network Interface selectorKnown limitations
Testing
Tested on:
Test scenarios covered:
WLED_USE_ETHERNETguard)Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Behavior