W5500 Support for ALL boards #320
Conversation
📝 WalkthroughWalkthroughAdds W5500 (SPI) Ethernet support and two ESP32‑S3 board entries; extends EthernetSettings with per-board SPI/RMII pin fields; conditionally exposes ETHClass2; implements dynamic SPI pin allocation/deallocation, deferred Ethernet init, UI/serialization updates, extensive pre-allocation pin validation, and ESP32‑PICO variant detection. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration
participant Init as Init/Setup
participant PinMgr as Pin Manager
participant ETH as ETH (ETHClass2 / ETH)
participant Log as Logger
rect rgba(100,150,200,0.5)
Note over Init,ETH: W5500 (SPI) flow
Init->>Config: read eth_type, spi-use & W5500 pins
Config-->>Init: W5500 config (MOSI,MISO,SCLK,CS,INT,RST,use)
Init->>PinMgr: allocate SPI pins (MOSI,MISO,SCLK,CS,INT,RST)
PinMgr-->>Init: allocated / failure
Init->>ETH: ETH.begin(SPI_host, pins...)
ETH-->>Init: success / failure
alt Success
Init->>Log: log W5500 success + MAC
Init->>Init: mark Ethernet configured
else Failure
Init->>PinMgr: deallocate pins
Init->>Log: log W5500 failure
end
end
rect rgba(200,100,100,0.5)
Note over Init,ETH: RMII flow
Init->>Config: read RMII config & clock mode
Config-->>Init: RMII pin mapping
Init->>PinMgr: allocate RMII pins (MDC/MDIO/POWER + reserved)
PinMgr-->>Init: allocated / failure
Init->>ETH: ETH.begin(RMII params...)
ETH-->>Init: success / failure (link/speed/duplex/MAC)
alt Success
Init->>Log: log RMII success (link/speed/duplex/MAC)
Init->>Init: mark Ethernet configured
else Failure
Init->>PinMgr: deallocate pins
Init->>Log: log RMII failure
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Checks will all fail because the exiting build environment is not prepped. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
13-24: Fix array type mismatch foresp32_nonconfigurable_ethernet_pins.
The non‑RMII definition uses an empty initializer with unspecified size, which conflicts with the fixed‑size extern declaration and can break compilation. Make the definition match the declared size (or guard the declaration in the header).🛠️ Suggested fix
`#ifdef` CONFIG_ETH_PHY_INTERFACE_RMII const managed_pin_type esp32_nonconfigurable_ethernet_pins[WLED_ETH_RSVD_PINS_COUNT] = { { 21, true }, // RMII EMAC TX EN == When high, clocks the data on TXD0 and TXD1 to transmitter { 19, true }, // RMII EMAC TXD0 == First bit of transmitted data { 22, true }, // RMII EMAC TXD1 == Second bit of transmitted data { 25, false }, // RMII EMAC RXD0 == First bit of received data { 26, false }, // RMII EMAC RXD1 == Second bit of received data { 27, true }, // RMII EMAC CRS_DV == Carrier Sense and RX Data Valid }; `#else` -const managed_pin_type esp32_nonconfigurable_ethernet_pins[] = {}; +const managed_pin_type esp32_nonconfigurable_ethernet_pins[WLED_ETH_RSVD_PINS_COUNT] = {}; `#endif`
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 28-30: Remove the unused "inliner" dependency declaration from
package.json and also remove its entry from package-lock.json; verify that
tools/cdata.js imports only "web-resource-inliner" (not "inliner") and then run
npm install (or npm ci) to regenerate the lockfile so package-lock.json no
longer contains "inliner". Ensure package.json still lists
"web-resource-inliner" and that no other files import or require "inliner"
before committing.
🧹 Nitpick comments (3)
wled00/src/dependencies/network/Network.h (1)
5-7: Include placement outside header guard may cause issues.The
ETHClass2.hinclude andextern ETHClass2 ETHdeclaration are placed before the#ifndef Network_hinclude guard (lines 10-11). This means every translation unit that includesNetwork.hwill re-process these lines. WhileETHClass2.hlikely has its own guards, theexterndeclaration placement is unconventional and could cause confusion.Consider moving these inside the include guard for consistency:
Suggested restructure
+#ifndef Network_h +#define Network_h + `#ifdef` ESP8266 `#include` <ESP8266WiFi.h> `#else` // ESP32 `#include` <WiFi.h> - // `#include` <ETH.h> `#include` "ETHClass2.h" extern ETHClass2 ETH; `#endif` -#ifndef Network_h -#define Network_h - class NetworkClasswled00/data/settings_wifi.htm (1)
214-216: Indentation uses spaces instead of tabs.The new options use spaces for indentation, but per coding guidelines, web UI files under
wled00/data/should use tabs for indentation. The existing options in this file use tabs.As per coding guidelines, after editing web UI files, run
npm run buildto regenerate embedded headers.Fix indentation to use tabs
- <option value="15">TTGO T-ETH-Lite S3 (W5500) ☾</option> <!-- WLEDMM --> - <option value="16">WaveShare ESP32-S3-ETH (W5500) ☾</option> <!-- WLEDMM --> - <option value="17">W5500 Generic ☾</option> <!-- WLEDMM --> + <option value="15">TTGO T-ETH-Lite S3 (W5500) ☾</option> <!-- WLEDMM --> + <option value="16">WaveShare ESP32-S3-ETH (W5500) ☾</option> <!-- WLEDMM --> + <option value="17">W5500 Generic ☾</option> <!-- WLEDMM -->wled00/wled.h (1)
356-356: Global ETH instance declaration follows WLED pattern.The
WLED_GLOBAL ETHClass2 ETH;declaration follows the established pattern for WLED global variables. This works correctly with theextern ETHClass2 ETH;declaration inNetwork.h.Consider grouping this with other network-related globals (near line 645 with
udpConnectedetc.) for better organization, though this is a minor nitpick.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonwled00/const.hwled00/data/settings_wifi.htmwled00/network.cppwled00/src/dependencies/network/Network.hwled00/wled.cppwled00/wled.hwled00/wled_ethernet.h
🧰 Additional context used
📓 Path-based instructions (3)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/const.hwled00/wled.cppwled00/src/dependencies/network/Network.hwled00/network.cppwled00/wled.hwled00/wled_ethernet.h
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**/*.{htm,html,css,js}: Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
After editing web UI files, run npm run build to regenerate embedded headers
Manually validate web UI changes via local HTTP server and browser
Files:
wled00/data/settings_wifi.htm
wled00/data/**/*.{htm,html,js,css}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Check the browser console for JavaScript errors after web UI changes
Files:
wled00/data/settings_wifi.htm
🧠 Learnings (9)
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR `#270`, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/const.h
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
wled00/const.hwled00/data/settings_wifi.htmwled00/wled.hwled00/wled_ethernet.h
📚 Learning: 2025-12-21T22:26:03.261Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 293
File: wled00/udp.cpp:668-669
Timestamp: 2025-12-21T22:26:03.261Z
Learning: In MoonModules/WLED-MM on 32-bit ESP32 platforms, changing pixel index variables to `int` type to align with the `setPixelColor(int, ...)` API signature is intentional and safe. Maximum pixel addresses are bounded by UINT16_MAX, and `setPixelColor()` performs internal overflow checks.
Applied to files:
wled00/const.hwled00/wled.h
📚 Learning: 2025-12-28T09:48:59.612Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-12-28T09:48:59.612Z
Learning: In QEMU mode with WLED_QEMU flag, Network.cpp must check tcpip_adapter directly instead of using the ETH object, because ETH.begin() is skipped to avoid hardware register crashes. Without this, Network.isConnected() always returns false, preventing initInterfaces() and server.begin() from being called.
Applied to files:
wled00/wled.cppwled00/src/dependencies/network/Network.hwled00/wled.h
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/wled.cppwled00/wled.h
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : After editing web UI files, run npm run build to regenerate embedded headers
Applied to files:
package.json
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/**/*.{cpp,h} : Use spaces (2 per level) for C++ firmware source and headers in wled00
Applied to files:
wled00/wled.h
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
wled00/wled.h
📚 Learning: 2026-01-07T23:30:25.779Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 308
File: platformio.ini:560-560
Timestamp: 2026-01-07T23:30:25.779Z
Learning: In platformio.ini, ESP32-S3 environments intentionally have WLED_USE_SHARED_RMT commented out (keeping RMTHI driver enabled) for testing purposes, while ESP32-S2 has it enabled (using shared RMT driver) to avoid boot loops. This inconsistency is temporary while investigating why RMTHI causes interrupt watchdog resets on S3. Once the S3 issue is understood, both S2 and S3 will use the same driver flags.
Applied to files:
wled00/wled.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: wled_build / Builds (esp32S3_WROOM-2_M)
- GitHub Check: wled_build / Builds (esp32c3dev_4MB_M)
- GitHub Check: wled_build / Builds (abc_wled_controller_v43_V4_S)
- GitHub Check: wled_build / Builds (esp32S3_8MB_S)
- GitHub Check: wled_build / Builds (esp8266pro_16MB_M)
- GitHub Check: wled_build / Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: wled_build / Builds (esp32s2_PSRAM_M)
- GitHub Check: wled_build / Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: wled_build / Builds (esp32s2_PSRAM_S)
- GitHub Check: wled_build / Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: wled_build / Builds (esp32_4MB_PSRAM_S)
- GitHub Check: wled_build / Builds (esp32dev_compat)
- GitHub Check: wled_build / Builds (esp32_4MB_M_eth)
- GitHub Check: wled_build / Builds (esp32_16MB_V4_M_debug)
- GitHub Check: wled_build / Builds (esp32_4MB_V4_M)
🔇 Additional comments (8)
wled00/src/dependencies/network/Network.h (1)
5-7: ETHClass2 integration looks correct for ESP32 path.The replacement of
ETH.hwithETHClass2.hand the extern declaration aligns with the correspondingWLED_GLOBAL ETHClass2 ETHdefinition inwled.h. This follows WLED's established pattern for global variables.Based on learnings, verify that QEMU mode (with
WLED_QEMUflag) still works correctly, as the learning notes that ETH.begin() is skipped in QEMU to avoid hardware register crashes.wled00/const.h (2)
312-312: LGTM! Ethernet type count updated correctly.The count is correctly updated from 15 to 18 to accommodate the three new W5500-based Ethernet types. The comment accurately reflects the additions.
329-331: New Ethernet type constants are well-defined.The three new constants (15, 16, 17) are sequential and properly follow the existing
WLED_ETH_OLIMEX_GTW(14). The naming convention is consistent with existing entries. These values align with the UI options added insettings_wifi.htm.wled00/wled.h (1)
111-112: ETHClass2 integration is consistent across headers.The replacement of
ETH.hwithETHClass2.hmatches the change inNetwork.h. Keeping the commented-out original include is helpful for future reference. As per coding guidelines, 2-space indentation is used correctly.wled00/wled_ethernet.h (1)
21-58: Struct extension for SPI pins looks solid.
The added pin fields and type adjustments align with the new W5500 path and keep RMII compatibility intact.wled00/network.cpp (2)
26-306: Board pin table updates look consistent.
The added SPI pin fields and new W5500 board entries integrate cleanly with the expanded settings structure.
372-392: Ethernet link diagnostics are a nice improvement.
The added link/speed/duplex logging plus the default event log should help with field debugging.wled00/wled.cpp (1)
1051-1135: Dual‑path Ethernet init is clean and well‑guarded.
Pin allocation, failure handling, and success logging look consistent across W5500 and RMII branches.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Ok, I can fix that. It's just that https://github.com/troyhacks/ETHClass2 is missing in lib_deps ? |
Yes, that and raising the platform package version. |
|
Even works on the ESP32-C3... which also surprised me at 64x64 (4096 pixels) over Art-Net at over 45 FPS with Particle Effects even. (This did require me to use the standard libs and not Tasmora as is currently the package for the C3. And some swearing and many failed attempts to figure out how to get this C3 to boot at all - even before the W5500 code. 😁) |
|
Ooo, CodeRabbit called me a pro. 🏆 |
… just to print the new IP to the console.
There was a problem hiding this comment.
Actionable comments posted: 0
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)
13-24: Fix array size mismatch in non-RMII fallback declaration.The empty array
esp32_nonconfigurable_ethernet_pins[]in the non-RMII case will cause out-of-bounds access. Code inxml.cpp:244andcfg.cpp:749unconditionally loop overWLED_ETH_RSVD_PINS_COUNT(6 iterations) when ethernet is enabled, regardless of the RMII configuration. IfWLED_USE_ETHERNETis enabled butCONFIG_ETH_PHY_INTERFACE_RMIIis not, these loops will access indices 0–5 of a zero-sized array.Change the non-RMII fallback to match the declared size:
Fix
`#else` -const managed_pin_type esp32_nonconfigurable_ethernet_pins[] = {}; +const managed_pin_type esp32_nonconfigurable_ethernet_pins[WLED_ETH_RSVD_PINS_COUNT] = {}; `#endif`
|
We can't merge until the builds are green |
So how do we change this? Using a never version overall of the V4 IDF or some other configuration? |
netmindz
left a comment
netmindz
left a comment
There was a problem hiding this comment.
Failing builds need to be addressed prior to merge
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@platformio.ini`:
- Around line 462-463: The platform_packages upgrade for the esp32_idf_V4
profile (platform_packages = platformio/framework-arduinoespressif32 @
3.20017.241212+sha.dcc1105b) is newer than the V4 baseline and must be validated
by building a representative environment; run a full build of at least one
common environment such as esp32_4MB_V4_M (and optionally one S3/S2/C3 variant)
using the updated platform/platform_packages settings to confirm there are no
compile or linker regressions, report any failures, and only merge once the
build(s) succeed or adjust the platform_packages back to a known-good version.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/wled.h`:
- Around line 359-363: Network.h currently includes "ETHClass2.h" and declares
extern ETHClass2 ETH without the same IDF-version guard used in wled.h, causing
use of ETHClass2 when it's unavailable; wrap both the `#include` "ETHClass2.h" and
the declaration extern ETHClass2 ETH inside the identical conditional `#if`
ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 5) (and matching `#endif`) so
Network.h only references ETHClass2 for IDF >= 4.4.5 exactly as wled.h does.
Flash Usage for
|
| Environment | Flash | Used | Total |
|---|---|---|---|
esp32dev_compat |
93.3% | 1467293 | 1572864 |
esp32_4MB_V4_HUB75_forum |
86.2% | 1638521 | 1900544 |
esp32_16MB_V4_M |
49.5% | 1555733 | 3145728 |
esp32_16MB_V4_M_debug |
51.7% | 1625897 | 3145728 |
esp32S3_8MB_PSRAM_M |
68.9% | 1445329 | 2097152 |
adafruit_matrixportal_esp32s3 |
72.9% | 1528385 | 2097152 |
athom_music_esp32_4MB_V4_M |
88.2% | 1503077 | 1703936 |
esp32c3mini_dio_4MB_M |
84.4% | 1438320 | 1703936 |
esp32_4MB_M_eth |
87.2% | 1486609 | 1703936 |
abc_wled_controller_v43_V4_S |
81.2% | 1383925 | 1703936 |
WaveShare_ESP32-S3-ETH |
47.5% | 1494897 | 3145728 |
esp8266_4MB_M |
94.9% | 991479 | 1044464 |
esp32s2_PSRAM_S |
88.5% | 1391766 | 1572864 |
esp32_16MB_V4_S |
52.0% | 1636097 | 3145728 |
esp32_4MB_V4_S_HUB75 |
85.1% | 1338933 | 1572864 |
esp32s2_PSRAM_M |
87.6% | 1492810 | 1703936 |
esp32c3dev_2MB_M |
90.7% | 1425816 | 1572864 |
esp8266_2MB_S |
87.7% | 915815 | 1044464 |
esp32_4MB_V4_S_eth |
81.2% | 1382769 | 1703936 |
esp32c3dev_4MB_M |
84.5% | 1440480 | 1703936 |
esp32S3_WROOM-2_M |
52.9% | 1663605 | 3145728 |
esp8266_4MB_S |
89.5% | 934771 | 1044464 |
esp32S3_8MB_S |
73.3% | 1537657 | 2097152 |
esp32_4MB_PSRAM_S |
81.5% | 1388317 | 1703936 |
esp32_4MB_V4_M |
92.4% | 1574757 | 1703936 |
esp8266pro_16MB_M |
96.3% | 1005351 | 1044464 |
esp32_pico_4MB_V4_S |
79.7% | 1357217 | 1703936 |
esp01_1MB_S |
98.7% | 880871 | 892912 |
esp32S3_16MB_PSRAM_M_HUB75 |
53.0% | 1667857 | 3145728 |
|
@softhack007 and @netmindz - this is "Troymota" which is my attempt to rebuild Arduino-ESP32 2.0.15 based on IDF v4.4.7 with W5500 enabled and less cruft than the regular Espressif build. I used it here anywhere there was a v4 platform, enabled Ethernet and PSRAM for all builds where they might have PSRAM. Clearly testing required - and I don't have every board - but it seems to work. |
Flash Usage for
|
| Environment | Flash | Used | Total |
|---|---|---|---|
esp32_4MB_V4_HUB75_forum |
86.2% | 1638989 | 1900544 |
esp32dev_compat |
93.3% | 1468189 | 1572864 |
esp32_4MB_M_eth |
87.4% | 1488505 | 1703936 |
esp32_16MB_V4_S |
52.0% | 1636501 | 3145728 |
esp32_4MB_V4_M |
92.5% | 1576261 | 1703936 |
esp32_4MB_V4_S_HUB75 |
85.2% | 1339509 | 1572864 |
esp32_16MB_V4_M |
49.5% | 1557065 | 3145728 |
esp32S3_8MB_PSRAM_M |
68.9% | 1445489 | 2097152 |
esp32S3_4MB_PSRAM_S |
71.2% | 1353417 | 1900544 |
esp32_pico_4MB_V4_S |
79.7% | 1357757 | 1703936 |
esp32_4MB_V4_S_eth |
81.2% | 1383321 | 1703936 |
esp32S3_8MB_S |
73.3% | 1537585 | 2097152 |
esp32S3_4MB_PSRAM_M |
87.6% | 1492549 | 1703936 |
esp32_16MB_V4_M_debug |
51.8% | 1628905 | 3145728 |
esp32S3_WROOM-2_M |
52.9% | 1663877 | 3145728 |
esp8266_2MB_S |
87.7% | 915735 | 1044464 |
esp32c3dev_2MB_M |
90.7% | 1426048 | 1572864 |
esp8266pro_16MB_M |
96.3% | 1006103 | 1044464 |
esp32S3_16MB_PSRAM_M_HUB75 |
53.0% | 1668125 | 3145728 |
esp8266_4MB_S |
89.5% | 934691 | 1044464 |
WaveShare_ESP32-S3-ETH |
47.5% | 1495745 | 3145728 |
esp32S3_4MB_S |
76.8% | 1309265 | 1703936 |
esp32_4MB_PSRAM_S |
81.5% | 1388901 | 1703936 |
adafruit_matrixportal_esp32s3 |
72.9% | 1528421 | 2097152 |
esp32s2_PSRAM_M |
87.7% | 1493658 | 1703936 |
esp32s2_PSRAM_S |
88.5% | 1391902 | 1572864 |
esp8266_4MB_M |
95.0% | 991959 | 1044464 |
esp32c3mini_dio_4MB_M |
84.4% | 1438560 | 1703936 |
abc_wled_controller_v43_V4_S |
81.3% | 1384469 | 1703936 |
|
Marking this as "ready for review" There's also some cherry-pickable commits in here that fix the whole Pico chipset pin stuff so we don't need to rely on a "pico" build - it detects the chip and adjusts accordingly. This was due to the new Pico-V3 in the Quinled Dig-Next-2 box which was showing up with incorrect pins. Now you can just flash regular ESP32 code and it does the right things automatically. So far I've tested this on:
All builds in the platformio.ini file have PSRAM and WLED_USE_ETHERNET enabled as long as they're V4 builds. And they're all using "Troymota" 😉 - firmware stats from a few runs are above. "Troymota" also means I can crank out adjustments/tests now if needed. There's also some entirely unchecked AI code for old usermods that just went and assigned pins without checking - this caused some issues during testing when the board was expecting to use those pins for Ethernet (set on boot) and then something else grabbed them and le crash. (I mean, I checked them visually.... they aren't common ones I'd ever use.)
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
usermods/sd_card/usermod_sd_card.h (1)
176-193: Avoid unsigned wrap when invalid pins become-1.
oldPin*areuint8_t, so when a pin is invalidated to-1, the old value wraps to255, causing false change detection and repeated reinit/log spam. Make the snapshots signed to keep comparisons consistent.🔧 Suggested fix
- uint8_t oldPinSourceSelect = configPinSourceSelect; - uint8_t oldPinSourceClock = configPinSourceClock; - uint8_t oldPinPoci = configPinPoci; - uint8_t oldPinPico = configPinPico; + int8_t oldPinSourceSelect = configPinSourceSelect; + int8_t oldPinSourceClock = configPinSourceClock; + int8_t oldPinPoci = configPinPoci; + int8_t oldPinPico = configPinPico;platformio.ini (1)
257-268: Add ETHClass2 to esp32_legacy lib_ignore.
ETHClass2 is now in global lib_deps but esp32_legacy environments do not ignore it. The code has guards requiringESP_IDF_VERSION >= 4.4.5, and esp32_legacy uses framework 1.0.6.4 which likely predates this requirement. Either test that esp32dev_compat and related legacy builds compile successfully, or add ETHClass2 to the esp32_legacy lib_ignore (similar to how it's handled for ESP8266 at line 331).
🤖 Fix all issues with AI agents
In `@usermods/Temperature/usermod_temperature.h`:
- Around line 7-18: The current Ethernet branch unconditionally undefines
TEMPERATURE_PIN which removes any user or build-flag overrides; change the
WLED_USE_ETHERNET branch to only define a default of -1 when TEMPERATURE_PIN is
not already defined (remove the unconditional `#undef` and use `#ifndef`
TEMPERATURE_PIN / `#define` TEMPERATURE_PIN -1 / `#endif`), keeping the existing
non-Ethernet defaults and preserving explicit overrides; reference
TEMPERATURE_PIN and WLED_USE_ETHERNET when locating the block to update.
In `@wled00/cfg.cpp`:
- Around line 355-381: The INT pin is being declared as an output in the
PinManagerPinType array (spi[6]) which can prevent assignment of input-only
GPIOs; change the entry for spi_int to mark it as an input (i.e., set the
boolean flag for the spi_int element to input) so the PinManager allocates it
correctly, leaving other pins (spi_mosi, spi_miso, spi_sclk, spi_cs, spi_rst)
unchanged; update the spi array initializer referenced in cfg.cpp
(PinManagerPinType spi[6] ...) so pinManager.allocateMultiplePins(...) gets the
corrected spi_int configuration.
In `@wled00/data/settings_wifi.htm`:
- Around line 214-215: Replace the space indentation before the two option
elements for "TTGO T-ETH-Lite S3 (W5500) ☾" (value="15") and "WaveShare
ESP32-S3-ETH (W5500) ☾" (value="16") with a single tab to match the rest of
settings_wifi.htm, save the file, then run npm run build to regenerate the
embedded web UI headers and validate the page in-browser (check console for
errors).
In `@wled00/pin_manager.cpp`:
- Around line 57-66: Replace the runtime psramFound() check with a chip feature
flag check: call esp_chip_info() and test the features bitmask for
CHIP_FEATURE_EMB_PSRAM to detect embedded PSRAM reliably; use that result to set
cachedVariant to ESP32PicoVariant::PicoV3_02 (instead of using psramFound()),
leaving the existing flashSizeBytes-based branch for PicoV3, and ensure
references to psramFound() are removed or bypassed so GPIO9/10 are not
misclassified when build-time PSRAM support is disabled.
In `@wled00/set.cpp`:
- Around line 668-699: The non‑W5500 branch calls
pinManager.allocateMultiplePins(spi, 6, PinOwner::HW_SPI) but ignores its return
value; update the code to capture and check the allocation result, and on
failure log/debug the error, deallocate any partially allocated pins with
pinManager.deallocateMultiplePins(...), set spi_use_for_w5500 = false and avoid
proceeding to use SPI/W5500 (same failure handling as the other branches);
ensure you reference allocateMultiplePins, deallocateMultiplePins,
spi_use_for_w5500 and use_spi_for_w5500 when implementing the check and
handling.
- Around line 599-626: The assignment use_spi_for_w5500 = p->value() wrongly
relies on String->bool conversion; instead parse the parameter content
explicitly (e.g., check p->value() equals "1" or
equalsIgnoreCase("true"/"yes"/"on") or use p->value().toInt() > 0) so
use_spi_for_w5500 is set correctly; update the loop handling AsyncWebParameter*
p (the block that currently contains use_spi_for_w5500 = p->value()) to perform
an explicit string-to-bool conversion and keep logging via USER_PRINTF as
before.
In `@wled00/wled.h`:
- Around line 884-888: The declaration under the USE_HW_SPI_FOR_W5500 branch
uses the Windows-specific type BOOL which breaks ESP32 builds; change that
declaration to use the standard C++ bool for spi_use_for_w5500 (keep WLED_GLOBAL
and the _INIT(USE_HW_SPI_FOR_W5500) initializer the same) so both branches use
the same portable type (replace BOOL with bool in the branch that currently
declares BOOL spi_use_for_w5500 _INIT(...)).
🧹 Nitpick comments (6)
wled00/wled.cpp (4)
1072-1074: Redundant assignment -spi_use_for_w5500is already truthy here.The
elsebranch is entered whenif (spi_use_for_w5500)is true (line 1063), sospi_use_for_w5500 = false;contradicts the entry condition. If this is intentional fallback logic, the condition should be restructured.🔧 Consider clarifying the intent
If this is meant to handle the case where
spi_use_for_w5500is true butethernetType == 0, the logic should be:if (spi_use_for_w5500 && ethernetType == 0) { // W5500 via user-configured SPI pins ... } else if (ethernetType > 0) { // Board-specific Ethernet ... }Otherwise, remove the redundant assignment.
1090-1105: Remove commented-out debug code.This large block of commented-out ownership verification code adds noise. If needed for future debugging, consider using a
#if 0block with a comment explaining its purpose, or remove it entirely.
1117-1117: Use project logging macros for consistency.
Serial.printlnbypasses the project's debug/user output system. UseUSER_PRINTLNorDEBUG_PRINTLNfor consistent logging behavior.♻️ Proposed fix
} else { - Serial.println("ETH initialized W5500!"); + USER_PRINTLN(F("ETH initialized W5500!")); spi_use_for_w5500 = true; }
1174-1176: Silent failure when RMII not configured inside W5500 block.When
CONFIG_ETH_SPI_ETHERNET_W5500is defined butCONFIG_ETH_PHY_INTERFACE_RMIIis not, and a non-W5500 board is selected, this returnsfalsewithout any logging. Consider adding a debug message to help diagnose misconfiguration.🔧 Proposed fix
`#else` + DEBUG_PRINTLN(F("initE: RMII not available for this board type")); return false; `#endif`platformio.ini (2)
475-487: Deduplicate repeated-D BOARD_HAS_PSRAM/-D WLED_USE_ETHERNET.
Both defines appear twice in the samebuild_flagsblock; removing the duplicates avoids redefinition noise.♻️ Suggested cleanup
build_flags = -g -DARDUINO_ARCH_ESP32 -DESP32 @@ - -D BOARD_HAS_PSRAM - -D WLED_USE_ETHERNET @@ - -D BOARD_HAS_PSRAM - -D WLED_USE_ETHERNET + -D BOARD_HAS_PSRAM + -D WLED_USE_ETHERNET
3334-3342: Consider matchingbuild_unflagsto inherited pin defines.
-D LEDPINmay not remove-D LEDPIN=14(and similar), which can leave macro‑redefinition warnings. Using exact values (or-U) avoids that.🔧 Example using exact matches
-build_unflags = -D LEDPIN -D BTNPIN -D RLYPIN -D I2S_SDPIN -D I2S_CKPIN -D I2S_WSPIN -D ARDUINO_TTGO_T7_S3 +build_unflags = -D LEDPIN=14 -D BTNPIN=0 -D RLYPIN=15 -D I2S_SDPIN=10 -D I2S_CKPIN=11 -D I2S_WSPIN=12 -D ARDUINO_TTGO_T7_S3
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wled00/pin_manager.cpp (1)
487-491: Replace the VLA indeallocateMultiplePinsto use a fixed upper bound.Line 489 uses a VLA (
uint8_t pins[arrayElementCount];) wherearrayElementCountis a dynamic parameter. While the parameter is bounded at compile-time asbyte(max 255 bytes), using a fixed-size array aligned withWLED_NUM_PINSis the preferred pattern on embedded platforms.🛠️ Safe option using a fixed upper bound
bool PinManagerClass::deallocateMultiplePins(const managed_pin_type * mptArray, byte arrayElementCount, PinOwner tag) { - uint8_t pins[arrayElementCount]; - for (int i=0; i<arrayElementCount; i++) pins[i] = mptArray[i].pin; + if (arrayElementCount > WLED_NUM_PINS) return false; + uint8_t pins[WLED_NUM_PINS]; + for (int i=0; i<arrayElementCount; i++) pins[i] = mptArray[i].pin; return deallocateMultiplePins(pins, arrayElementCount, tag); }wled00/set.cpp (1)
599-698: Allocate SPI pins and setspi_use_for_w5500only after successful init.In the W5500 branch,
spi_use_for_w5500is set andinitEthernet()is called without reserving pins, and the flag remains true even if init fails. BecauseinitEthernet()skips pin allocation when the flag is true, this can leave pins unreserved and the UI/state out of sync. Reserve the pins first and only set the flag on success; revert and deallocate on failure.Suggested fix
- if (hw_mosi_pin >= 0 && hw_sclk_pin >= 0 && use_spi_for_w5500 == true) { - spi_use_for_w5500 = use_spi_for_w5500; - USER_PRINTLN("Trying to start W5500 SPI Ethernet"); - WLED::instance().initEthernet(); - } else if (hw_mosi_pin >= 0 && hw_sclk_pin >= 0 && use_spi_for_w5500 == false) { + if (hw_mosi_pin >= 0 && hw_sclk_pin >= 0 && use_spi_for_w5500 == true) { + if (!pinManager.allocateMultiplePins(spi, 6, PinOwner::HW_SPI)) { + DEBUG_PRINTLN(F("Could not allocate SPI pins for W5500.")); + spi_use_for_w5500 = false; + } else if (WLED::instance().initEthernet()) { + spi_use_for_w5500 = true; + } else { + uint8_t spiPins[6] = { + (uint8_t)hw_mosi_pin, (uint8_t)hw_miso_pin, (uint8_t)hw_sclk_pin, + (uint8_t)hw_cs_pin, (uint8_t)hw_int_pin, (uint8_t)hw_rst_pin + }; + pinManager.deallocateMultiplePins(spiPins, 6, PinOwner::HW_SPI); + spi_use_for_w5500 = false; + } + } else if (hw_mosi_pin >= 0 && hw_sclk_pin >= 0 && use_spi_for_w5500 == false) { pinManager.allocateMultiplePins(spi, 6, PinOwner::HW_SPI); spi_use_for_w5500 = false; } else {wled00/cfg.cpp (1)
32-37: Defer Ethernet init while the JSON lock is held (and avoid double init).
deserializeConfig()still callsinitEthernet()while the JSON buffer lock is held, even though the new comment warns against it. With the post‑lock call added later, W5500 can also initialize twice. Gate the early call on!fromFSand perform the init after releasing the lock for all Ethernet types.Suggested fix
`#ifdef` WLED_USE_ETHERNET JsonObject ethernet = doc[F("eth")]; CJSON(ethernetType, ethernet["type"]); // NOTE: Ethernet configuration takes priority over other use of pins - WLED::instance().initEthernet(); + if (!fromFS) WLED::instance().initEthernet(); `#endif` ... -#if defined(WLED_USE_ETHERNET) && defined(CONFIG_ETH_SPI_ETHERNET_W5500) - if (spi_use_for_w5500 && spi_mosi >= 0 && spi_sclk >= 0) { - WLED::instance().initEthernet(); - } -#endif +#ifdef WLED_USE_ETHERNET + if (ethernetType != WLED_ETH_NONE) { + WLED::instance().initEthernet(); + } +#endifAlso applies to: 396-399, 719-725
🤖 Fix all issues with AI agents
In `@platformio.ini`:
- Around line 448-450: Remove the global -D BOARD_HAS_PSRAM define from the
common/shared build flag blocks and only define BOARD_HAS_PSRAM inside
env-specific sections that target PSRAM-capable chips (e.g., the envs that
already explicitly set it); search for every occurrence of the literal
BOARD_HAS_PSRAM in the platformio.ini and move/delete those entries in the
shared flag groups so non‑PSRAM ESP32/S2/S3/C3 builds no longer get the define.
- Around line 3334-3342: The build_unflags entries in env:WaveShare_ESP32-S3-ETH
(build_unflags) are not removing inherited -D macros because PlatformIO matches
exact flag text; update build_unflags to use the exact parent defines (e.g., -D
LEDPIN=<parent_value>, -D BTNPIN=<parent_value>, -D RLYPIN=<parent_value>, -D
I2S_SDPIN=<parent_value>, -D I2S_CKPIN=<parent_value>, -D
I2S_WSPIN=<parent_value>) that appear in env:esp32S3_16MB_PSRAM_M_HUB75 so they
are properly removed, then keep only the intended -D LEDPIN=-1 etc. in
build_flags (or remove duplicates) to avoid duplicate macro definitions and
warnings.
In `@usermods/PWM_fan/usermod_PWM_fan.h`:
- Around line 121-140: In initPWMfan(), when you early-disable the usermod due
to pwmPin being invalid/allocated (the branches that set enabled = false and
return), ensure you release any tacho reservation created earlier by
initTacho(): if tachoPin is valid (e.g., >= 0) and
pinManager.isPinAllocated(tachoPin) then call pinManager.deallocatePin(tachoPin)
and reset tachoPin to -1 before returning; apply this to every early-return path
(checks around pwmPin, pinManager.isPinOk, pinManager.isPinAllocated, and failed
allocatePin) so the tacho pin is not leaked when the usermod is disabled.
In
`@usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display_ALT.h`:
- Around line 1650-1656: The loop that sanitizes ioPin (ioPin[],
pinManager.isPinOk()) currently resets invalid pin values to -1 which masks the
original bad config and prevents setup() from producing a clear error; instead
preserve the original invalid value (do not assign -1) and set a visible error
indicator (e.g. populate errorMessage or a dedicated invalidPinIndex field)
and/or call DEBUG_PRINTF with a clear config error so setup() can detect and
fail with the original pin value; update the block that calls
pinManager.isPinOk(), DEBUG_PRINTF and any code that reads ioPin[] in setup() to
check errorMessage/invalidPinIndex and return a descriptive init failure rather
than a generic one.
In `@wled00/const.h`:
- Around line 613-621: The comments for HW_PIN_CSSPI, HW_PIN_INTSPI, and
HW_PIN_RSTSPI are incorrect copies referencing "MISO/19"; update the comment
text for each `#define` so it accurately describes the intended W5500 pin function
(CS, INT, RST respectively) and that -1 is the default when not defined (e.g.,
"W5500 CS pin, use -1 if not defined"), keeping the existing `#define` logic in
const.h and ensuring the symbols HW_PIN_CSSPI, HW_PIN_INTSPI, and HW_PIN_RSTSPI
are the ones updated.
- Line 312: The WLED_NUM_ETH_TYPES macro is off-by-one and the comment is
incorrect: change the definition of WLED_NUM_ETH_TYPES (in const.h) from 18 to
17 so it matches Ethernet types 0–16, and update the trailing comment to state
that types range 0–16, that Olimex ESP32-Gateway (type 14) is not new, and that
two W5500 types (15 and 16) were added; keep the change limited to the
WLED_NUM_ETH_TYPES macro and its comment.
In `@wled00/wled.cpp`:
- Around line 1013-1039: The guard for already-configured Ethernet
(successfullyConfiguredEthernet) must be enforced for the global-SPI W5500 path
too: move the check that returns false when successfullyConfiguredEthernet is
true so it executes before the CONFIG_ETH_SPI_ETHERNET_W5500 conditional (or
otherwise ensure it runs unconditionally for all branches that handle
ethernetType), i.e. place the successfullyConfiguredEthernet check at the top of
the initE/ethernet initialization sequence (before evaluating spi_use_for_w5500
and ethernetType) so repeated calls cannot re-init when
successfullyConfiguredEthernet is true; keep the existing early returns for
WLED_ETH_NONE and invalid ethernetType and continue to load es =
ethernetBoards[ethernetType] as before.
In `@wled00/wled.h`:
- Around line 884-888: Replace the undefined BOOL with the standard bool for the
global variable declaration so both branches use the same type: change the
second branch that declares WLED_GLOBAL BOOL spi_use_for_w5500
_INIT(USE_HW_SPI_FOR_W5500); to use bool instead of BOOL, keeping the
WLED_GLOBAL and _INIT(...) wrappers intact and preserving the conditional
compilation around USE_HW_SPI_FOR_W5500.
In `@wled00/xml.cpp`:
- Around line 903-925: The code emits oappendi(HW_PIN_CSSPI),
oappendi(HW_PIN_INTSPI) and oappendi(HW_PIN_RSTSPI) unguarded which will break
builds when those HW_PIN_*SPI macros are undefined; wrap each oappendi(...) call
(and the corresponding xOpt(...) alternate entries) with `#ifdef/`#endif checks
for HW_PIN_CSSPI, HW_PIN_INTSPI and HW_PIN_RSTSPI respectively (or provide a
defined fallback constant) so references to those symbols only compile when
defined; adjust the blocks around the oappendi(...) and xOpt(...) lines in the
CONFIG_ETH_SPI_ETHERNET_W5500 section to conditionally compile per symbol
(functions/methods: oappendi, xOpt; macros: HW_PIN_CSSPI, HW_PIN_INTSPI,
HW_PIN_RSTSPI).
🧹 Nitpick comments (3)
wled00/wled_ethernet.h (1)
64-65: Avoid zero-length extern array when RSVD count is 0.With
WLED_ETH_RSVD_PINS_COUNTset to 0 in the non‑RMII branch, the extern array becomes a zero-length array (non‑standard and can break stricter toolchains). Consider an unsized extern here and keep the actual size only at the definition site.♻️ Suggested adjustment
-extern managed_pin_type esp32_nonconfigurable_ethernet_pins[WLED_ETH_RSVD_PINS_COUNT]; +extern managed_pin_type esp32_nonconfigurable_ethernet_pins[];usermods/Temperature/usermod_temperature.h (2)
7-18: Preserve explicitTEMPERATURE_PINoverrides when Ethernet is enabled.The current
#undefforcesTEMPERATURE_PINto-1even if a user set it via build flags. Consider only setting the default when the symbol isn’t already defined.♻️ Suggested adjustment
-#ifndef WLED_USE_ETHERNET - `#ifndef` TEMPERATURE_PIN - `#ifdef` ARDUINO_ARCH_ESP32 - `#define` TEMPERATURE_PIN 18 - `#else` //ESP8266 boards - `#define` TEMPERATURE_PIN 14 - `#endif` - `#endif` -#else - `#undef` TEMPERATURE_PIN - `#define` TEMPERATURE_PIN -1 -#endif +#ifdef WLED_USE_ETHERNET + `#ifndef` TEMPERATURE_PIN + `#define` TEMPERATURE_PIN -1 + `#endif` +#else + `#ifndef` TEMPERATURE_PIN + `#ifdef` ARDUINO_ARCH_ESP32 + `#define` TEMPERATURE_PIN 18 + `#else` //ESP8266 boards + `#define` TEMPERATURE_PIN 14 + `#endif` + `#endif` +#endif
214-223: Use output-capable validation for the OneWire data pin.
temperaturePinis actively driven, so the pre-checks should useisPinOk(..., true)to reject input-only pins earlier.♻️ Suggested adjustment
- } else if (!pinManager.isPinOk(temperaturePin, false)) { + } else if (!pinManager.isPinOk(temperaturePin, true)) { DEBUG_PRINTLN(F("Temperature: pin not valid for this board.")); temperaturePin = -1; @@ - if (newTemperaturePin >= 0 && !pinManager.isPinOk(newTemperaturePin, false)) { + if (newTemperaturePin >= 0 && !pinManager.isPinOk(newTemperaturePin, true)) { DEBUG_PRINTLN(F("Temperature: configured pin is not valid for this board.")); newTemperaturePin = -1; }Also applies to: 396-399
usermods/usermod_v2_four_line_display_ALT/usermod_v2_four_line_display_ALT.h
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@wled00/cfg.cpp`:
- Around line 396-399: deserializeConfig() currently calls initEthernet() even
when fromFS==true which can initialize W5500 under the JSON lock and cause
double init; update deserializeConfig() to skip calling initEthernet() when
fromFS is true (i.e., only call initEthernet() when fromFS == false) or remove
the early initEthernet() call entirely and ensure all Ethernet initialization
happens in the post-lock path (the block that runs after deserializeConfig()
completes); use the fromFS parameter and the initEthernet() symbol to guard or
relocate the init to the existing post-lock initialization code to prevent
reentrancy and duplicate initialization.
In `@wled00/set.cpp`:
- Around line 673-694: The code calls WLED::instance().initEthernet() while the
JSON settings lock is held (inside the settings handler), which can cause
reentrancy/serialization issues; instead, set spi_use_for_w5500 (and any related
state) but do NOT call initEthernet() there—instead set a small pending flag
(e.g., pending_init_ethernet or reuse an existing variable) when
use_spi_for_w5500 is true and pin allocation succeeds, and invoke
WLED::instance().initEthernet() only after releaseJSONBufferLock() has been
called (or from the code path that runs after the settings lock is released) so
the init runs outside the JSON buffer lock; update the block around
spi_use_for_w5500/use_spi_for_w5500 and remove the direct initEthernet() call,
and ensure any deferred flag is checked and consumed later to call
initEthernet().
In `@wled00/wled.cpp`:
- Around line 1089-1103: The code conditionally allocates pins when
spi_use_for_w5500 == false but always deallocates pinsToAllocate on ETH.begin()
failure; change this so deallocation only occurs if allocation happened—e.g.,
introduce a bool (pinsAllocated) set true after
pinManager.allocateMultiplePins(pinsToAllocate, 6, PinOwner::Ethernet) succeeds
(or check spi_use_for_w5500) and on ETH.begin() failure call
pinManager.deallocatePin(...) for items in pinsToAllocate only when
pinsAllocated is true (or when spi_use_for_w5500 == false), referencing
spi_use_for_w5500, pinsToAllocate, pinManager.allocateMultiplePins, ETH.begin,
and pinManager.deallocatePin.
In `@wled00/wled.h`:
- Around line 115-119: The include of "ETHClass2.h" is guarded only by
CONFIG_ETH_SPI_ETHERNET_W5500 but the global declaration later also requires
ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4,4,5); update the include guard at the
top (the block that currently checks CONFIG_ETH_SPI_ETHERNET_W5500) to require
both CONFIG_ETH_SPI_ETHERNET_W5500 and ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4,
4, 5) so that the ETHClass2.h include matches the same condition used for the
global declaration and avoids pulling in the header when ESP-IDF is too old.
🧹 Nitpick comments (1)
usermods/sd_card/usermod_sd_card.h (1)
188-192: Good early validation; consider adding debug feedback.Sanitizing invalid pins early is a good defensive practice. However, when a user-configured pin is silently changed to -1, they may not understand why SD card initialization fails (especially without debug output enabled).
Consider adding a debug message when a pin is rejected:
💡 Optional: Add debug logging for rejected pins
// Early validation - reject obviously invalid pins -if (!pinManager.isPinOk(configPinSourceSelect, true)) configPinSourceSelect = -1; -if (!pinManager.isPinOk(configPinSourceClock, true)) configPinSourceClock = -1; -if (!pinManager.isPinOk(configPinPoci, false)) configPinPoci = -1; -if (!pinManager.isPinOk(configPinPico, true)) configPinPico = -1; +if (!pinManager.isPinOk(configPinSourceSelect, true)) { DEBUG_PRINTF("[%s] SS pin %d invalid\n", _name, configPinSourceSelect); configPinSourceSelect = -1; } +if (!pinManager.isPinOk(configPinSourceClock, true)) { DEBUG_PRINTF("[%s] CLK pin %d invalid\n", _name, configPinSourceClock); configPinSourceClock = -1; } +if (!pinManager.isPinOk(configPinPoci, false)) { DEBUG_PRINTF("[%s] POCI pin %d invalid\n", _name, configPinPoci); configPinPoci = -1; } +if (!pinManager.isPinOk(configPinPico, true)) { DEBUG_PRINTF("[%s] PICO pin %d invalid\n", _name, configPinPico); configPinPico = -1; }
Flash Usage for
|
| Environment | Flash | Used | Total |
|---|---|---|---|
WaveShare_ESP32-S3-ETH |
47.5% | 1495569 | 3145728 |
abc_wled_controller_v43_V4_S |
81.2% | 1384397 | 1703936 |
esp01_1MB_S |
98.6% | 880727 | 892912 |
esp32S3_16MB_PSRAM_M_HUB75 |
53.0% | 1667837 | 3145728 |
esp32S3_4MB_PSRAM_M |
87.6% | 1492357 | 1703936 |
esp32S3_4MB_PSRAM_S |
71.2% | 1353345 | 1900544 |
esp32S3_4MB_S |
76.8% | 1309201 | 1703936 |
esp32S3_8MB_PSRAM_M |
68.9% | 1445289 | 2097152 |
esp32S3_8MB_S |
73.3% | 1537613 | 2097152 |
esp32S3_WROOM-2_M |
52.9% | 1663585 | 3145728 |
esp32_16MB_V4_M |
49.5% | 1556877 | 3145728 |
esp32_16MB_V4_S |
52.0% | 1636717 | 3145728 |
esp32_4MB_M_eth |
87.4% | 1488409 | 1703936 |
esp32_4MB_PSRAM_S |
81.5% | 1388829 | 1703936 |
esp32_4MB_V4_HUB75_forum |
86.3% | 1639253 | 1900544 |
esp32_4MB_V4_M |
92.5% | 1576109 | 1703936 |
esp32_4MB_V4_S_HUB75 |
85.2% | 1339417 | 1572864 |
esp32_4MB_V4_S_eth |
81.2% | 1383225 | 1703936 |
esp32_pico_4MB_V4_S |
79.7% | 1357593 | 1703936 |
esp32c3dev_2MB_M |
90.7% | 1425828 | 1572864 |
esp32c3dev_4MB_M |
84.5% | 1440500 | 1703936 |
esp32c3mini_dio_4MB_M |
84.4% | 1438340 | 1703936 |
esp32dev_compat |
93.3% | 1468113 | 1572864 |
esp32s2_PSRAM_M |
87.6% | 1493474 | 1703936 |
esp32s2_PSRAM_S |
88.5% | 1391698 | 1572864 |
esp8266_2MB_S |
87.7% | 915607 | 1044464 |
esp8266_4MB_M |
95.0% | 991847 | 1044464 |
esp8266_4MB_S |
89.5% | 934563 | 1044464 |
esp8266pro_16MB_M |
96.3% | 1005991 | 1044464 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/wled.cpp`:
- Around line 1014-1232: Move the ethernetType validation and bounds check up so
they run unconditionally (i.e., check ethernetType == WLED_ETH_NONE and
ethernetType >= WLED_NUM_ETH_TYPES before any CONFIG_ETH_SPI_ETHERNET_W5500
logic), assign es = ethernetBoards[ethernetType] immediately after that, and
change the global SPI override (the spi_use_for_w5500 branch that forces
es.eth_type = ETH_PHY_W5500 and pin assignments) to only apply when the selected
board is actually a W5500 board — e.g., guard that override with a check like
ethernetBoards[ethernetType].eth_type == ETH_PHY_W5500 (or es.eth_type ==
ETH_PHY_W5500) so UI/config choices for non-W5500 types aren’t overridden by
spi_use_for_w5500.
…s enabled but we don't actually have PSRAM
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/pin_manager.cpp`:
- Around line 240-243: Split the combined conditional so OPI flash GPIO
reservation is unconditional at compile-time while octal PSRAM remains
runtime-checked: when CONFIG_ESPTOOLPY_FLASHMODE_OPI is set, always reserve gpio
33–37 (remove hasPsramRuntime() from that branch); keep the
CONFIG_SPIRAM_MODE_OCT branch using hasPsramRuntime() to reserve PSRAM pins at
runtime. Apply the same change to the other pin_manager.cpp occurrence and
replace the psramFound() check in xml.cpp with the correct logic (use the
CONFIG_ESPTOOLPY_FLASHMODE_OPI unconditional reservation and keep
hasPsramRuntime() for SPIRAM) so gpio, hasPsramRuntime(), psramFound(),
CONFIG_ESPTOOLPY_FLASHMODE_OPI and CONFIG_SPIRAM_MODE_OCT are updated
consistently.
| #if CONFIG_ESPTOOLPY_FLASHMODE_OPI || CONFIG_SPIRAM_MODE_OCT | ||
| // Use runtime check - only show as reserved if PSRAM is actually present | ||
| if (gpio > 32 && gpio < 38 && hasPsramRuntime()) return (F("(reserved) Octal PSRAM or Octal Flash")); | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the pin_manager.cpp file at the specified lines
wc -l wled00/pin_manager.cppRepository: MoonModules/WLED-MM
Length of output: 89
🏁 Script executed:
# Read the code around line 240-243
sed -n '235,250p' wled00/pin_manager.cppRepository: MoonModules/WLED-MM
Length of output: 1006
🏁 Script executed:
# Read the code around line 893-897
sed -n '888,905p' wled00/pin_manager.cppRepository: MoonModules/WLED-MM
Length of output: 1306
🏁 Script executed:
# Search for all references to GPIO 33-37 or octal/OPI in pin_manager.cpp
rg -n "(gpio.*3[3-7]|OPI|octal|FLASHMODE)" wled00/pin_manager.cppRepository: MoonModules/WLED-MM
Length of output: 568
🏁 Script executed:
# Search for hasPsramRuntime usage in the codebase
rg -n "hasPsramRuntime" wled00/Repository: MoonModules/WLED-MM
Length of output: 524
🏁 Script executed:
# Search for all OPI or octal flash references
rg -n "FLASHMODE_OPI|SPIRAM_MODE_OCT" wled00/Repository: MoonModules/WLED-MM
Length of output: 1102
🏁 Script executed:
# Check if there are UI strings mentioning these pins
rg -n "Octal|octal.*PSRAM|octal.*Flash" wled00/Repository: MoonModules/WLED-MM
Length of output: 988
🌐 Web query:
ESP32-S3 GPIO33-37 octal SPI flash OPI mode pin reservation requirements
💡 Result:
On ESP32‑S3, when Octal SPI flash (OPI mode) is used, GPIO33–GPIO37 are not available as general‑purpose GPIOs because they become part of the memory bus (SPI0/1 “MSPI” signals) required to fetch code/data from flash. Espressif explicitly marks these as “not recommended for other uses” in Octal mode. [1]
What GPIO33–GPIO37 become in Octal SPI / OPI
Per the ESP32‑S3 datasheet pin mapping table for Octal SPI/OPI: [2]
- GPIO33 → DQ4 (SPIIO4)
- GPIO34 → DQ5 (SPIIO5)
- GPIO35 → DQ6 (SPIIO6)
- GPIO36 → DQ7 (SPIIO7)
- GPIO37 → DQS/DM (SPIDQS)
Practical “reservation” requirement
- If your design/board is configured for Octal Flash or Octal PSRAM, treat GPIO33–GPIO37 as reserved and do not connect them to other peripherals (I2C, LEDs, buttons, etc.), or you can break boot/flash access. [1]
- In Octal mode, the flash/PSRAM bus also consumes the usual SPI0/1 pins (GPIO26–GPIO32), so those are likewise not recommended for other uses. [1]
Sources:
[1] ESP-IDF GPIO notes for ESP32-S3 (SPI0/1 and Octal flash/PSRAM using GPIO33–GPIO37) (docs.espressif.com)
[2] ESP32-S3 Series Datasheet v2.1, Table 2‑14 pin mapping (GPIO33–GPIO37 = DQ4–DQ7, DQS/DM in Octal SPI/OPI) (documentation.espressif.com)
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/stable/esp32s3/api-reference/peripherals/gpio.html?utm_source=openai
- 2: https://documentation.espressif.com/esp32-s3_datasheet_en.html?utm_source=openai
Separate OPI flash GPIO reservation from octal PSRAM detection.
When CONFIG_ESPTOOLPY_FLASHMODE_OPI is set, GPIO33–37 are unconditionally tied to octal flash (DQ4–DQ7/DQS) as a hardware requirement. Gating their reservation on hasPsramRuntime() is incorrect—it allows these pins to appear usable when PSRAM is absent, risking conflicts with flash lines in OPI builds. CONFIG_SPIRAM_MODE_OCT is separate and its runtime check is appropriate.
Affects lines 240–243 and 893–897 in pin_manager.cpp, and also xml.cpp:238 which uses psramFound() instead of hasPsramRuntime() with the same flaw.
🛠️ Suggested fix
- `#if` CONFIG_ESPTOOLPY_FLASHMODE_OPI || CONFIG_SPIRAM_MODE_OCT
- // Use runtime check - only show as reserved if PSRAM is actually present
- if (gpio > 32 && gpio < 38 && hasPsramRuntime()) return (F("(reserved) Octal PSRAM or Octal Flash"));
- `#endif`
+ `#if` CONFIG_ESPTOOLPY_FLASHMODE_OPI
+ if (gpio > 32 && gpio < 38) return (F("(reserved) Octal Flash"));
+ `#elif` CONFIG_SPIRAM_MODE_OCT
+ if (gpio > 32 && gpio < 38 && hasPsramRuntime()) return (F("(reserved) Octal PSRAM"));
+ `#endif`- `#if` CONFIG_SPIRAM_MODE_OCT || CONFIG_ESPTOOLPY_FLASHMODE_OPI
- // 33 to 37: not available if using octal SPI Flash or octal PSRAM
- // Use runtime check to allow these pins when PSRAM isn't actually present
- if (gpio > 32 && gpio < 38 && hasPsramRuntime()) return false;
- `#endif`
+ `#if` CONFIG_ESPTOOLPY_FLASHMODE_OPI
+ if (gpio > 32 && gpio < 38) return false;
+ `#elif` CONFIG_SPIRAM_MODE_OCT
+ if (gpio > 32 && gpio < 38 && hasPsramRuntime()) return false;
+ `#endif`🤖 Prompt for AI Agents
In `@wled00/pin_manager.cpp` around lines 240 - 243, Split the combined
conditional so OPI flash GPIO reservation is unconditional at compile-time while
octal PSRAM remains runtime-checked: when CONFIG_ESPTOOLPY_FLASHMODE_OPI is set,
always reserve gpio 33–37 (remove hasPsramRuntime() from that branch); keep the
CONFIG_SPIRAM_MODE_OCT branch using hasPsramRuntime() to reserve PSRAM pins at
runtime. Apply the same change to the other pin_manager.cpp occurrence and
replace the psramFound() check in xml.cpp with the correct logic (use the
CONFIG_ESPTOOLPY_FLASHMODE_OPI unconditional reservation and keep
hasPsramRuntime() for SPIRAM) so gpio, hasPsramRuntime(), psramFound(),
CONFIG_ESPTOOLPY_FLASHMODE_OPI and CONFIG_SPIRAM_MODE_OCT are updated
consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/wled.cpp`:
- Around line 1228-1229: The debug message in initE that prints via
DEBUG_PRINTLN when returning false is misleading because it assumes RMII
absence; update the message (in the initE function where WLED_USE_ETHERNET is
not defined) to a generic statement such as "Ethernet not enabled/supported for
this build (no RMII/W5500 support)" or similar to cover W5500 and other
non-Ethernet builds, keeping the DEBUG_PRINTLN call and the return false
unchanged.
🧹 Nitpick comments (1)
wled00/wled.cpp (1)
1070-1163: Indentation inconsistency in W5500/RMII conditional blocks.The code within the
#ifdef CONFIG_ETH_SPI_ETHERNET_W5500block uses inconsistent indentation. Per coding guidelines, wled00 C++ files should use 2-space indentation. Some lines appear to use different spacing.Example of inconsistent indentation
Lines 1076-1083 and 1108-1120 should consistently use 2-space indentation for nested blocks. Consider running a formatter or manually aligning the indentation throughout
initEthernet().
| DEBUG_PRINTLN(F("initE: RMII not available for this board type")); | ||
| return false; // Ethernet not enabled for build |
There was a problem hiding this comment.
Debug message is slightly misleading.
The message "RMII not available for this board type" is printed when WLED_USE_ETHERNET is not defined, which could include W5500 builds or any build without Ethernet support, not just RMII unavailability.
Suggested improvement
- DEBUG_PRINTLN(F("initE: RMII not available for this board type"));
+ DEBUG_PRINTLN(F("initE: Ethernet not enabled for this build"));🤖 Prompt for AI Agents
In `@wled00/wled.cpp` around lines 1228 - 1229, The debug message in initE that
prints via DEBUG_PRINTLN when returning false is misleading because it assumes
RMII absence; update the message (in the initE function where WLED_USE_ETHERNET
is not defined) to a generic statement such as "Ethernet not enabled/supported
for this build (no RMII/W5500 support)" or similar to cover W5500 and other
non-Ethernet builds, keeping the DEBUG_PRINTLN call and the return false
unchanged.
3 participants
This should allow anyone to add Ethernet to any board.
Some notes:
W5500 is SPI and that means it's slower than RMII - which is only on the original ESP32 and the ESP32-P4... but it's still highly functional. There's also things that can we tweaked in the IDF (when building Ardino-ESP32) as well as at WLED compile-time to potentially get more performance out of these. (Which I can now do... with "Troymota", ha)
The "W5500 Lite" (the smaller DIP-style module on the left) will absolutely require additional 3.3v power. It seems I'm not alone in saying that nobody has ever run one off the 3.3v rail of an ESP32. It doesn't need a lot, it's just very picky. The older "blue W5500" boards (right) seem to be much more tolerant and can run off an ESP32 rail, but you might want to add a big cap to absorb the surges and avoid brownouts.
Research says also just having a dedicated regulator will help as well - the Lite version just doesn't do itself any favors. I have used a breadboard power supply like this to give enough juice for the W5500 Lite to work:
platformio/framework-arduinoespressif32 has to be any version ABOVE 3.20009.0 (I tested the next higher and the latest version and both worked).
This may not strictly be needed if someone wanted to figure out the one missing function and "backport" it somehow, but the easy method is to just use a slightly newer version.(No, it needs to be at least IDF v4.4.5 - I tried Espressif's own workaround and it doesn't work.... around.)ETH.h is replaced with a backport LilyGo did from IDFv5 which I split out from their meta-library to: https://github.com/troyhacks/ETHClass2
I expanded the ethernet board struct to handle all 3 types of Ethernet - two RMII types and the W5500 - and it will likely handle ones we haven't used yet that Espressif supports. I think it's clever, @softhack007 may disagree. 😊
I think our move to IDFv4 means I don't need to use my modified AsyncUDP library anymore! Huzzah! It's sending tons of AsyncUDP data just fine. If we move to IDFv5 at some point, this PR will be mostly moot as support for all of these devices are in there natively.
There's some "saving the pins in the config file" logic I didn't touch - nothing uses them, as far as I know. If I'm wrong, I can dig into that. I assume most people pick the one that works from the drop-down and move on with their life.I fixed this too.I also checked to make sure it still works on the original ESP32 RMII Ethernet devices, and it does:
...and on the existing S3 boards I have from LilyGo and WaveShare, which are now user-selectable instead of compile-time flags!
...and here it is on an original ESP32 doing W5500 Ethernet. This is based on some random board I had lying around, so pins could be changed. We do have global SPI defines we can do, but I didn't implement those - but the W5500 needs 3 more pins (interrupt, CS/SS, and reset). My TTGO "Koala" board seems to be marked as having "hardware" SPI pins at 12,13,14 for MISO, MOSI, and SCLK.
And proof of life of W5500 on a reggo ESP32:
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.