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 2 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:
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 User as User / Web UI
participant Form as Settings Form
participant SetHandler as handleSettingsSet()
participant Config as cfg.json
participant InitEth as initEthernet()
participant Event as Network Events
participant SetPrimary as setPrimaryNetworkInterface()
participant lwIP as lwIP Stack
User->>Form: Enter Ethernet static IP & select primary
User->>Form: Submit settings
Form->>SetHandler: POST EIP*, EGW*, ESN*, EPI
SetHandler->>SetHandler: Parse octets into ethStaticIP/GW/SN
SetHandler->>Config: Serialize to cfg.json
InitEth->>InitEth: Read ethStaticIP, call ETH.config()
Event->>SetPrimary: ETH_CONNECTED / ETH_GOT_IP / WIFI_STA_GOT_IP
SetPrimary->>SetPrimary: Check ethPrimaryInterface, find en/st netif
SetPrimary->>lwIP: netif_set_default(selected)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
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
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
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.
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
| // AI: read ethernet static IP configuration from form POST. | ||
| // Each IP address is submitted as four separate octet fields (0-255), | ||
| // reassembled here into IPAddress objects matching the same pattern | ||
| // used for WiFi static IP fields (IP{n}0-3, GW{n}0-3, SN{n}0-3). | ||
| if (request->hasArg(F("EIP0"))) { | ||
| for (int i = 0; i < 4; i++) { | ||
| char eip[6], egw[6], esn[6]; | ||
| snprintf_P(eip, sizeof(eip), PSTR("EIP%d"), i); | ||
| snprintf_P(egw, sizeof(egw), PSTR("EGW%d"), i); | ||
| snprintf_P(esn, sizeof(esn), PSTR("ESN%d"), i); | ||
| ethStaticIP[i] = request->arg(eip).toInt(); | ||
| ethStaticGW[i] = request->arg(egw).toInt(); | ||
| ethStaticSN[i] = request->arg(esn).toInt(); | ||
| } | ||
| } | ||
| // AI: read primary network interface selection. | ||
| // PNI field value 0 = WiFi is primary interface, 1 = Ethernet is primary interface. | ||
| // Radio buttons only submit when selected so use hasArg with value check. | ||
| ethPrimaryInterface = (request->arg(F("EPI")).toInt() == 1); |
There was a problem hiding this comment.
Add required AI section delimiters around this generated C++ block.
The new ethernet POST parsing block is a larger AI-generated section and should be wrapped with the required begin/end AI markers.
Suggested fix
+ // AI: below section was generated by an AI
// AI: read ethernet static IP configuration from form POST.
// Each IP address is submitted as four separate octet fields (0-255),
// reassembled here into IPAddress objects matching the same pattern
// used for WiFi static IP fields (IP{n}0-3, GW{n}0-3, SN{n}0-3).
if (request->hasArg(F("EIP0"))) {
for (int i = 0; i < 4; i++) {
char eip[6], egw[6], esn[6];
snprintf_P(eip, sizeof(eip), PSTR("EIP%d"), i);
snprintf_P(egw, sizeof(egw), PSTR("EGW%d"), i);
snprintf_P(esn, sizeof(esn), PSTR("ESN%d"), i);
ethStaticIP[i] = request->arg(eip).toInt();
ethStaticGW[i] = request->arg(egw).toInt();
ethStaticSN[i] = request->arg(esn).toInt();
}
}
// AI: read primary network interface selection.
// PNI field value 0 = WiFi is primary interface, 1 = Ethernet is primary interface.
// Radio buttons only submit when selected so use hasArg with value check.
ethPrimaryInterface = (request->arg(F("EPI")).toInt() == 1);
+ // AI: endAs per coding guidelines "Embed larger blocks of AI-generated code within // AI: below section was generated by an AI ... // AI: end comments".
🤖 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 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.
@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. |
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
| #ifdef WLED_USE_ETHERNET | ||
| JsonObject ethernet = doc[F("eth")]; | ||
| CJSON(ethernetType, ethernet["type"]); | ||
| // NOTE: Ethernet configuration takes priority over other use of pins | ||
| // AI: deserialize ethernet static IP configuration. | ||
| // Each address is stored as a 4-element JSON array, one byte per octet, | ||
| // matching the same pattern used for WiFi static IP (nw.ins[n].ip/gw/sn). | ||
| JsonArray eth_ip = ethernet[F("eip")]; | ||
| JsonArray eth_gw = ethernet[F("egw")]; | ||
| JsonArray eth_sn = ethernet[F("esn")]; | ||
| if (!eth_ip.isNull()) { | ||
| for (size_t i = 0; i < 4; i++) { | ||
| CJSON(ethStaticIP[i], eth_ip[i]); | ||
| CJSON(ethStaticGW[i], eth_gw[i]); | ||
| CJSON(ethStaticSN[i], eth_sn[i]); | ||
| } | ||
| } | ||
| // AI: deserialize primary network interface selection. | ||
| // false = WiFi is default gateway (default), true = Ethernet is default gateway. | ||
| CJSON(ethPrimaryInterface, ethernet[F("epi")]); | ||
| initEthernet(); | ||
| #endif |
There was a problem hiding this comment.
Unify Ethernet compile guards in deserialization path.
This block is guarded only by WLED_USE_ETHERNET but references globals that are declared only under ARDUINO_ARCH_ESP32 && WLED_USE_ETHERNET. That can fail non-ESP32 builds when WLED_USE_ETHERNET is set.
Suggested fix
-#ifdef WLED_USE_ETHERNET
+#if defined(ARDUINO_ARCH_ESP32) && defined(WLED_USE_ETHERNET)
JsonObject ethernet = doc[F("eth")];
CJSON(ethernetType, ethernet["type"]);
...
initEthernet();
`#endif`As per coding guidelines "Verify feature-flag names and ensure WLED_ENABLE_* and WLED_DISABLE_* flags exactly match the documented list to avoid silent build variations".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #ifdef WLED_USE_ETHERNET | |
| JsonObject ethernet = doc[F("eth")]; | |
| CJSON(ethernetType, ethernet["type"]); | |
| // NOTE: Ethernet configuration takes priority over other use of pins | |
| // AI: deserialize ethernet static IP configuration. | |
| // Each address is stored as a 4-element JSON array, one byte per octet, | |
| // matching the same pattern used for WiFi static IP (nw.ins[n].ip/gw/sn). | |
| JsonArray eth_ip = ethernet[F("eip")]; | |
| JsonArray eth_gw = ethernet[F("egw")]; | |
| JsonArray eth_sn = ethernet[F("esn")]; | |
| if (!eth_ip.isNull()) { | |
| for (size_t i = 0; i < 4; i++) { | |
| CJSON(ethStaticIP[i], eth_ip[i]); | |
| CJSON(ethStaticGW[i], eth_gw[i]); | |
| CJSON(ethStaticSN[i], eth_sn[i]); | |
| } | |
| } | |
| // AI: deserialize primary network interface selection. | |
| // false = WiFi is default gateway (default), true = Ethernet is default gateway. | |
| CJSON(ethPrimaryInterface, ethernet[F("epi")]); | |
| initEthernet(); | |
| #endif | |
| `#if` defined(ARDUINO_ARCH_ESP32) && defined(WLED_USE_ETHERNET) | |
| JsonObject ethernet = doc[F("eth")]; | |
| CJSON(ethernetType, ethernet["type"]); | |
| // AI: deserialize ethernet static IP configuration. | |
| // Each address is stored as a 4-element JSON array, one byte per octet, | |
| // matching the same pattern used for WiFi static IP (nw.ins[n].ip/gw/sn). | |
| JsonArray eth_ip = ethernet[F("eip")]; | |
| JsonArray eth_gw = ethernet[F("egw")]; | |
| JsonArray eth_sn = ethernet[F("esn")]; | |
| if (!eth_ip.isNull()) { | |
| for (size_t i = 0; i < 4; i++) { | |
| CJSON(ethStaticIP[i], eth_ip[i]); | |
| CJSON(ethStaticGW[i], eth_gw[i]); | |
| CJSON(ethStaticSN[i], eth_sn[i]); | |
| } | |
| } | |
| // AI: deserialize primary network interface selection. | |
| // false = WiFi is default gateway (default), true = Ethernet is default gateway. | |
| CJSON(ethPrimaryInterface, ethernet[F("epi")]); | |
| initEthernet(); | |
| `#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/cfg.cpp` around lines 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.
| // AI: interface name prefixes in arduino-esp32 IDF V4 (Tasmota platform): | ||
| // 'en' = ethernet, 'st' = WiFi STA. Validated on IDF 4.4.8. | ||
| const char *targetName = ethPrimaryInterface ? | ||
| "en" : | ||
| "st"; | ||
|
|
||
| // AI: acquire lwIP TCP/IP core lock before accessing netif_list | ||
| // and calling netif_set_default() to avoid thread-safety assertions | ||
| LOCK_TCPIP_CORE(); | ||
|
|
||
| for (netif_iter = netif_list; netif_iter != NULL; netif_iter = netif_iter->next) { | ||
| if (!netif_is_up(netif_iter) || netif_iter->ip_addr.u_addr.ip4.addr == 0) continue; | ||
| const bool isPreferred = (netif_iter->name[0] == targetName[0] && | ||
| netif_iter->name[1] == targetName[1]); | ||
| if (isPreferred) { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Arduino-ESP32/ESP-IDF, is there a documented guarantee that lwIP netif names use "st" for WiFi STA and "en" for Ethernet across supported versions? If not, what stable API should be used to identify the default WiFi/Ethernet netif?
💡 Result:
No—there is no documented, stable guarantee in ESP-IDF/Arduino-ESP32 that the underlying lwIP netif name will always use exactly “st” for WiFi STA and “en” for Ethernet across supported versions. In ESP-IDF, the recommended stable abstraction is ESP-NETIF: it creates and manages network interfaces and exposes handles for “default” WiFi (station/AP) and Ethernet interfaces. The ESP-NETIF API explicitly provides helpers for default WiFi STA/Ethernet initialization and returns an esp_netif_t handle, rather than requiring you to parse lwIP netif->name. For example, ESP-NETIF default Wi-Fi initialization is provided by functions that return an esp_netif handle (e.g., esp_netif_create_default_wifi_sta) and similarly Ethernet examples exist; the programming guide describes these as the convenient/default initialization paths. [1][2] If you still need to identify which underlying lwIP netif corresponds to an esp_netif_t, ESP-NETIF provides a stable API to query the “net interface name” used by the underlying TCP/IP stack implementation (lwIP): esp_netif_get_netif_impl_name(esp_netif_t *esp_netif, char *name). This is the API intended to read the implementation-specific name, not to rely on a fixed naming convention like “st”/“en”. [3] Stable approach to identify the default WiFi/Ethernet netif: 1) Use the ESP-NETIF default-interface creation APIs and keep the returned esp_netif_t handles. 2) If needed for compatibility/debugging, call esp_netif_get_netif_impl_name(handle, buf) to get the underlying lwIP name. Evidence: ESP-IDF’s ESP-NETIF documentation states that it “provides some convenient methods of creating the most common ones, such as Wi‑Fi and Ethernet” and that these APIs return an esp_netif handle configured with default settings. [1] Separately, the esp_netif API explicitly defines esp_netif_get_netif_impl_name as “Get net interface name from network stack implementation,” clarifying that the name comes from the underlying TCP/IP stack and is therefore implementation-/stack-dependent. [3] In contrast, the lwIP netif naming scheme (netif->name is only two characters plus index/num in classic lwIP) is an implementation detail of lwIP itself and is not presented by ESP-NETIF docs as a contractual guarantee to remain “st”/“en” forever. (So depending on netif->name for semantic identity is not a documented contract in ESP-IDF/ESP-NETIF.) [4]
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/network/esp_netif.html
- 2: https://docs.espressif.com/projects/esp-idf/en/release-v5.2/esp32/api-reference/network/esp_netif.html
- 3: https://docs.espressif.com/projects/esp-idf/en/v4.4.4/esp32/api-reference/network/esp_netif.html
- 4: https://github.com/pycom/esp-idf-2.0/blob/master/components/lwip/core/netif.c
Fix fragile WiFi/Ethernet selection by removing hardcoded lwIP netif->name prefixes ("st"/"en")
wled00/network.cpp (lines 322-336) matches preferred interfaces by checking netif_iter->name against "st"/"en", but ESP-IDF/ESP-NETIF do not document a stable guarantee that lwIP will use those exact prefixes across versions/implementations—so primary selection can silently degrade to the wrong/fallback interface. Use ESP-NETIF’s default-interface creation APIs and keep the returned esp_netif_t handles (or map to the implementation-specific name via esp_netif_get_netif_impl_name()), instead of parsing netif->name.
🤖 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, 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).
@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
|
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