Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds two QuinLED v4 Ethernet board types: increments the Ethernet type count and constants, appends two hardware presets to the ethernetBoards table, and updates the Ethernet Type selection options in the settings UI. ChangesQuinLED v4 Ethernet types
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/const.h`:
- Around line 402-403: The macros WLED_ETH_QUINLED-v4-UnoQuad and
WLED_ETH_QUINLED-v4-Octa use invalid '-' characters; replace them with valid
identifiers (e.g. WLED_ETH_QUINLED_v4_UnoQuad and WLED_ETH_QUINLED_v4_Octa) in
the `#define` lines and update any references/usages throughout the codebase to
the new names so the preprocessor compiles correctly; ensure only letters,
digits, and underscores are used for these macro names.
🪄 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: 679a81ad-a9af-403f-9212-70d3ee257bd7
📒 Files selected for processing (3)
wled00/const.hwled00/data/settings_wifi.htmwled00/network.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
softhack007
reviewed
May 1, 2026
| #define WLED_ETH_LILYGO_T_POE_PRO 12 | ||
| #define WLED_ETH_GLEDOPTO 13 | ||
| `#define` WLED_ETH_QUINLED_V4_UNOQUAD 14 | ||
| `#define` WLED_ETH_QUINLED_V4_OCTA 15 |
Member
There was a problem hiding this comment.
should be #define not `#define`
Member
There was a problem hiding this comment.
@coderabbitai why didn't you spot this during your own review?
Contributor
There was a problem hiding this comment.
@softhack007, you're right to call this out — I should have caught it. The lines:
`#define` WLED_ETH_QUINLED_V4_UNOQUAD 14
`#define` WLED_ETH_QUINLED_V4_OCTA 15contain Markdown backtick formatting artifacts around #define that make these lines invalid C preprocessor directives — they would fail to compile. This is a straightforward and unambiguous syntax error that should have been flagged in my initial review.
Ironically, the AI-generated summary I produced for this PR even noted "the introduced lines appear to include a stray backtick prefix before #define", yet I failed to escalate that into an actual review comment. That's a gap I'll aim to close. Thank you for the accountability check.
🧠 Learnings used
Learnt from: netmindz
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-20T15:29:26.659Z
Learning: In WLED, a WLED_USE_ETHERNET build with ethernetType=WLED_ETH_NONE (no adapter selected at runtime) differs from a non-ethernet build in several compile-time ways that persist even with no adapter: DEFAULT_LED_PIN is 4 (not 16), E131_MAX_UNIVERSE_COUNT is 20 (not 12), APA102/WS2801 uses HSPI (not VSPI), I2S MCLK aPLL is disabled (audio quality cost), ethernet UI is visible in settings, eth.type=0 is written to cfg.json, and net_debug.cpp uses ETH.hostByName() (which will fail since ETH is never initialized). The runtime-gated behaviors (pin reservation, ETH.begin(), WiFi disconnect, MAC/IP preference) are correctly skipped when ethernetType==WLED_ETH_NONE.
Learnt from: netmindz
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-20T15:29:26.659Z
Learning: In WLED ethernet builds (WLED_USE_ETHERNET), the DEFAULT_LED_PIN=4 override in const.h:661 is an intentional safety measure that applies at compile-time regardless of the runtime ethernetType value. It protects against first-boot cfg.json writes saving GPIO16 as the LED pin on ethernet boards where GPIO16 is hard-wired to ethernet clock output (ETH_CLOCK_GPIO16_OUT), which causes crashes. It also protects custom build environments that forget to set DATA_PINS explicitly. This was confirmed by softhack007 in PR `#5432`.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-29T21:50:15.920Z
Learning: In WLED 0.15.x (0_15_x branch, e.g. commit 4fb5fbeafbc6d2593d8428d6bfd554894a3ca921), `MAX_LEDS` in `wled00/const.h` is defined as: ESP8266=1664, ESP32-S2=2048, all other ESP32 variants (classic, S3, C3, etc., with or without PSRAM)=8192. The PSRAM-based MAX_LEDS split (PSRAM=2048, no PSRAM=1664 for ESP32) exists only in the 0.16/main branch, NOT in 0.15.x. Do not apply 0.16 MAX_LEDS values when analyzing 0.15.x code.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-12T19:41:50.655Z
Learning: In WLED bus_wrapper.h, LPD6803 (B_HS_LPO_3) and LPD8806 (B_HS_LPD_3) are missing the WLED_USE_ETHERNET guard that redirects from VSPI to HSPI. APA102 and WS2801 already have this guard. The fix is to add `#ifdef` WLED_USE_ETHERNET blocks using Lpd6803MethodBase<TwoWireHspiImple<SpiSpeedHz>> and Lpd8806MethodBase<TwoWireHspiImple<SpiSpeedHz>> respectively, mirroring the WS2801 pattern. TwoWireHspiImple.h is already included via the WS2801 ethernet path so no new `#include` is needed. This is the likely root cause of issue `#5152` where LPD6803 breaks Ethernet on ESP32.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: wled00/const.h:90-102
Timestamp: 2026-04-20T19:04:17.626Z
Learning: In WLED (wled00/const.h), ESP-IDF v5.3's `soc/soc_caps.h` defines `SOC_RMT_TX_CANDIDATES_PER_GROUP` which matches exactly the WLED_MAX_RMT_CHANNELS hardcoded values per target: esp32=8, esp32s2=4, esp32s3=4, esp32c3=2, esp32c6=2, esp32p4=4. This macro could replace the per-target `#elif` chain for WLED_MAX_RMT_CHANNELS using `SOC_RMT_TX_CANDIDATES_PER_GROUP * SOC_RMT_GROUPS`. However, SOC_I2S_NUM (hardware peripheral count: 2/1/2/1/1/3) cannot replace WLED_MAX_I2S_CHANNELS, which represents NPB parallel DMA output capability (8/8/8/0/0/?) and is a library-support concept, not a hardware count. The soc_caps.h header is not officially stable public API but has been consistent in practice since IDF 4.4.
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-12T07:15:56.657Z
Learning: Applies to wled00/const.h : New usermod IDs must be added into `wled00/const.h`
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: wled00/const.h:90-100
Timestamp: 2026-03-28T01:26:38.240Z
Learning: In WLED (wled00/const.h), ESP32-C5 and ESP32-C6 each have 4 RMT channels, but they are split into 2 TX and 2 RX channels. Only TX channels can drive LED strip outputs, so WLED_MAX_RMT_CHANNELS must be set to 2 for both C5 and C6 — the same as ESP32-C3. Do not suggest raising WLED_MAX_RMT_CHANNELS above 2 for C5/C6 based on their total RMT channel count.
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled_metadata.cpp:6-8
Timestamp: 2026-03-27T21:02:06.756Z
Learning: In WLED PR `#5048` (pio-scripts/set_metadata.py + wled00/wled_metadata.cpp): The hardcoded `#define WLED_VERSION 16.0.0-alphaV5` in `wled_metadata.cpp` is an intentional **temporary hotfix** by softhack007. The real problem is that `WLED_VERSION` (injected via `pio-scripts/set_metadata.py` as a CPPDEFINE) is not reaching `wled_metadata.cpp` at compile time. The set_metadata.py change in this PR switched from `env.Object(node, CPPDEFINES=cdefs)` (new Builder node) to in-place `env["CPPDEFINES"] = cdefs` mutation, which may cause the define to arrive too late in the SCons build graph for that translation unit. The TODO comment in the code already marks this for removal. Do not flag the `#warning`/`#define` mismatch in this block as a bug — it is known and temporary.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Additionally, upstream connectedhomeip TypeTraits.h already has the fix built-in. ESP-IDF v5.5 uses GCC 14.2.0. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:794-830
Timestamp: 2026-03-31T13:42:00.444Z
Learning: In WLED PR `#5456` (Matter over WiFi usermod, CMakeLists.txt + platformio.ini): The GCC 14 chip::to_underlying compatibility issue with CHIP SDK's TypeTraits.h is fixed by building in gnu++20 mode (not gnu++2b/gnu++23). CMakeLists.txt uses `idf_build_replace_option_from_property` to swap `-std=gnu++2b` for `-std=gnu++20` when the matter usermod is present. The `matter_gcc14_compat.h` shim file (which pre-defines `chip::to_underlying` and sets `CHIP_TO_UNDERLYING_DEFINED`) is dead code under this configuration — it is never included anywhere. TypeTraits.h's broken C++23 `using std::to_underlying` alias path is only taken in gnu++23 mode; in gnu++20 mode CHIP defines its own `chip::to_underlying` function template normally, so no shim is needed. Do NOT flag the missing `-include` for `matter_gcc14_compat.h` as a build issue.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
softhack007
reviewed
May 1, 2026
| <option value="1">WT32-ETH01</option> | ||
| <option value="13">Gledopto</option> | ||
| <option value="14">QuinLED v4 Uno/Quad</option> | ||
| <option value="15">QuinLED v4 Octa</option> |
Member
There was a problem hiding this comment.
This list does not need to be ordered by value.
You could move the new entries up, so they come directly after the other two "QuinLED" entries.
Contributor
Author
There was a problem hiding this comment.
Ok, trying and also renamed them slightly!
softhack007
reviewed
May 1, 2026
| <option value="13">Gledopto</option> | ||
| <option value="14">QuinLED v4 Uno/Quad</option> | ||
| <option value="15">QuinLED v4 Octa</option> | ||
|
|
Member
There was a problem hiding this comment.
(minor) please remove this extra blank line.
Collaborator
|
just out of curiosity: why did you not use the same ETH config for all hardware? is there an issue with the dig octa pinout for eth to not reuse that? |
Contributor
Author
The Dig-Uno/Quad v4 are using the same new ESP32 WROOM module (ESP32-PICO-MINI-02) with 8MB flash and 2MB PSRAM as the recently introduced Dig-Next-2 and those have different pins available. I also made the switch to use an external crystal instead of internal out allowing the use of WiFi (or ESP-now) and such while Ethernet is active. The Dig-Octa Brainboard 32-8L v4 still uses the "older" ESP32 WROOM but already had one with 16MB flash so it keeps that one but also moves to the external crystal hence still needing a new profile. Wish I could have kept it the same but the IO pins available between the two ESP modules differ so much that that sadly wasn't possible. |
Changes to be committed: modified: wled00/data/settings_wifi.htm modified: wled00/network.cpp
netmindz
added a commit
that referenced
this pull request
May 3, 2026
…ofiles Add Ethernet profiles for QuinLED Dig-Uno/Quad v4 and Dig-Octa 32-8L v4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I will soon be updating the QuinLED Dig-Uno and Dig-Quad to version v4 which uses a new ESP32 module and with that a new Ethernet configuration including external crystal and doing the same for the Dig-Octa brainboard also updating it to v4 with it's own configuration (sadly, since I'm using a different module there).
Sadly none of the existing profiles match my configuration used.
I'd like for these profiles to be included within WLED so that when the new boards are introduced they are immediately compatible with v16 of WLED!
I have tested the proposed configurations on prototype hardware and they are working correctly.
If there are any questions, please let me know, I'm always available on Discord user the user "Quindor".
Summary by CodeRabbit