Fix LED buffer size calculation #4928
Conversation
- may fix wled#4614 - forces 1st 8 buses to be I2S if parallel I2S is used, followed by RMT
- improve UI calculation
WalkthroughAdds I2S-aware LED buffer accounting and shared I2S DMA estimation, adjusts per-target bus data-size logic, removes default no-FS memory checks during automatic bus creation, and updates the web UI memory estimator with pixel-buffer overhead and parallel-I2S hardware logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/bus_manager.cpp (1)
906-907: Build breaker: missing semicolonu++ is missing a semicolon; this won’t compile.
Apply this diff:
- u++ + u++;
🧹 Nitpick comments (5)
wled00/bus_wrapper.h (2)
1363-1363: Duplicate assignment to offsetoffset is set twice for num < 8 on ESP32. Remove the duplicate.
- if (num < 8) offset = 1; // 8 I2S followed by 8 RMT - if (num < 8) offset = 1; // 8 I2S followed by 8 RMT + if (num < 8) offset = 1; // 8 I2S followed by 8 RMT
554-567: Spelling nit: “parallel”Several comments say “paralell/parellel”. Consider fixing for readability in the touched blocks.
Also applies to: 652-667, 748-762, 870-884, 1083-1096
wled00/data/settings_leds.htm (1)
203-206: 8266 DMA detection should use numeric comparevalue is a string; use strict compare on a parsed int to avoid edge cases.
- if (is8266() && d.Sf["L0"+n].value == 3) { //8266 DMA uses 5x the mem + if (is8266() && parseInt(d.Sf["L0"+n].value, 10) === 3) { // 8266 DMA (GPIO3) uses 5x the memwled00/FX_fcn.cpp (1)
1203-1206: Minor: add newline to OOM debugHelps readability in logs when a bus is skipped.
- DEBUG_PRINTF_P(PSTR("Out of LED memory! Bus %d (%d) #%u not created."), (int)bus.type, (int)bus.count, digitalCount); + DEBUG_PRINTF_P(PSTR("Out of LED memory! Bus %d (%d) #%u not created.\n"), (int)bus.type, (int)bus.count, digitalCount);wled00/bus_manager.cpp (1)
788-805: I2S common buffer calc: add 4‑byte alignment and small robustness tweaks
- DMA is “aligned to 4 bytes” (per comments in bus_wrapper.h). Align i2sCommonSize up to 4 to avoid minor undercount.
- Optional: hoist stepFactor outside the loop and consider size_t for sizes (non-blocking).
Suggested minimal fix within this block:
- unsigned i2sCommonSize = stepFactor * bus->getLength() * bus->getNumberOfChannels() * (bus->is16bit()+1); + unsigned i2sCommonSize = stepFactor * bus->getLength() * bus->getNumberOfChannels() * (bus->is16bit() ? 2u : 1u); + // 4-byte alignment for DMA + i2sCommonSize = (i2sCommonSize + 3u) & ~3u; if (i2sCommonSize > maxI2S) maxI2S = i2sCommonSize;Non-blocking nits:
- Define stepFactor once above the loop to avoid multiple constexprs.
- Consider size_t for size/maxI2S to future-proof larger configurations.
Please confirm that, in parallel I2S mode, the set of buses contributing to the shared DMA indeed corresponds to the first up-to-8 one‑wire digital buses as iterated here (ordering vs. RMT/I2S mapping). If ordering differs, we may need to adjust which buses are considered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wled00/FX_fcn.cpp(1 hunks)wled00/bus_manager.cpp(3 hunks)wled00/bus_wrapper.h(3 hunks)wled00/cfg.cpp(1 hunks)wled00/data/settings_leds.htm(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware files
Files:
wled00/cfg.cppwled00/FX_fcn.cppwled00/bus_manager.cppwled00/bus_wrapper.h
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
🧠 Learnings (5)
📓 Common learnings
Learnt from: mval-sg
PR: wled/WLED#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.
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#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.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-09-01T10:26:17.959Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-09-02T01:45:58.047Z
Learnt from: willmmiles
PR: wled/WLED#4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:31-36
Timestamp: 2025-09-02T01:45:58.047Z
Learning: For the NeoEsp32RmtHI driver, RISC-V ESP32-C3 support is currently disabled via bus_wrapper.h rather than compile-time guards, as the maintainer willmmiles is working on resolving underlying nested interrupt issues and prefers to centralize the workaround in one location.
Applied to files:
wled00/bus_wrapper.h
🧬 Code graph analysis (2)
wled00/FX_fcn.cpp (3)
wled00/bus_manager.h (7)
isDigital(141-142)isDigital(180-180)is2Pin(181-181)hasRGB(163-165)hasWhite(166-172)hasCCT(173-178)is16bit(185-185)wled00/bus_manager.cpp (8)
add(82-87)add(82-82)add(807-828)add(807-807)memUsage(765-776)memUsage(765-765)memUsage(779-805)memUsage(779-779)wled00/bus_wrapper.h (1)
memUsage(1209-1276)
wled00/bus_manager.cpp (2)
wled00/bus_manager.h (1)
BusDigital(234-285)wled00/bus_wrapper.h (3)
getDataSize(1111-1207)memUsage(1209-1276)isParallelI2S1Output(338-338)
⏰ 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). (16)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (10)
wled00/bus_wrapper.h (2)
472-485: ESP32 channel remap logic — verify off-by-one for parallel I2SLooks sane, but please confirm channel mapping when parallel I2S is enabled on classic ESP32: subtracting 1 and then skipping 7 (when channel > 6) matches intended RMT/I2S ordering after 0.15.0-b3. A quick bench test with 9+ outputs would be good.
1215-1273: memUsage split (RMT vs I2S) aligns with FX/bus_managerThe exclusion of I2S DMA here (handled separately as maxI2S) and double-buffering for RMT look consistent with BusManager::memUsage() and WS2812FX::finalizeInit().
Please double-check 16‑bit and 5‑channel multipliers against NeoPixelBus behavior for UCS/SM16825 on ESP32.
wled00/data/settings_leds.htm (4)
196-198: Include skipped LEDs in len — good catchAccounting for SLn in memory calc is correct.
215-217: Pixel buffer overhead (global + segment) — OKlen*8 per bus matches 4B global + 4B segment buffer per LED under typical segment layouts.
If segments won’t cover the full LED range, consider noting this is a conservative estimate.
269-270: Numeric n in getMem call — OKAvoids implicit string math.
307-308: ParseInt for bus index — OKPrevents accidental string concatenation issues.
wled00/cfg.cpp (1)
323-323: Use of emplace_back for default bus config — OKDirect in-place construction with doInitBusses=true fits the new finalizeInit gating.
Confirm no other caller relies on the removed interim mem accounting in this path.
wled00/FX_fcn.cpp (2)
1177-1207: I2S-aware memory gating — looks correctAccumulating mem (front/back) and maxI2S (DMA cadence) then gating on mem+maxI2S <= MAX_LED_MEMORY matches BusManager::memUsage().
Please sanity-check on S2/S3 with and without parallel I2S (8+ outputs) to ensure usesI2S selection is correct per target.
1209-1209: Debug print consistencyYou print mem+maxI2S vs BusManager::memUsage(); they should match post‑creation. Good for cross‑check.
wled00/bus_manager.cpp (1)
769-771: Comment alignment with implementation looks goodBusConfig::memUsage() delegates to PolyBus::memUsage() and notes the shared I2S DMA isn’t included. This matches the intended split between per-bus and common buffers.
|
@blazoncek I see you pushed an update to your branch. Do I need to cherry pick that as well or is it cosmetic? |
|
@willmmiles I pulled these changes into my "heap handlint tests" branch. I can get ESP8266 to work with 800 LEDs still but it is very(!) unstable, frequently refusing to fully load the UI (info->reboot still works), at lucky moments there is enough heap to run a PS effect, but its not really useable. On the positive side: at least it does not lock-up. without a reboot, heap is fragmented and nothing works right. |
Hit 2 flies with a single strike. 😉 BTW add transition buffer to calculation as transitions are enabled by default. |
lucky strike ;)
I would argue that those are not strictly required, allocation will fail and transition will not happen. If we want the value to represent "will work without memory issues" I agree. ESP8266 will show the warning at around 220LEDs if transition buffer is added to the calculation, stability issues start appearing above 400 but out of memory for FX buffer in transitions can happen at 200 already, depending on heap fragmentation. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wled00/data/settings_leds.htm (1)
207-214: Call isS3() and fix the TODO typos; else branch also misses ()Using isS3 instead of isS3() makes parallelI2S true on non‑ESP32 targets and skews mem calc; also repeat in Line 210. Fix typos in the TODO.
Apply:
- let parallelI2S = d.Sf.PR.checked && (is32() || isS2() || isS3) && !isD2P(t); + let parallelI2S = d.Sf.PR.checked && (is32() || isS2() || isS3()) && !isD2P(t); @@ - } else if ((is32() || isS2() || isS3) && n > (parallelI2S ? 7 : 0)) { + } else if ((is32() || isS2() || isS3()) && n > (parallelI2S ? 7 : 0)) { @@ - dbl = len * ch * 3; // DMA buffer for parallel I2S (TODO: ony the bus with largst LED count should be used) + dbl = len * ch * 3; // DMA buffer for parallel I2S (TODO: only the bus with the largest LED count should be used)
🧹 Nitpick comments (3)
wled00/data/settings_leds.htm (3)
199-200: Align pbfr with PR intent: 2 mandatory buffers; add transition buffer only if enabledCurrent pbfr = len*12 always counts a second segment buffer for transitions. PR states two mandatory buffers (global + segment). Make the extra buffer conditional on transitions (TD > 0).
Apply:
- let pbfr = len * 12; // pixel buffers: global buffer + 2*segment buffer (one is required, one for transitions) + const transEnabled = parseInt(d.Sf.TD.value, 10) > 0; // 0 ms disables transitions + let pbfr = len * (8 + (transEnabled ? 4 : 0)); // 2 mandatory 32-bit buffers (+1 for transitions)Also applies to: 216-216
196-197: parseInt radix and NaN guardBe explicit with base 10 and guard SL against NaN to avoid propagating NaN into mem calc.
Apply:
- let len = parseInt(d.Sf["LC"+n].value); - len += parseInt(d.Sf["SL"+n].value); // skipped LEDs are allocated too + let len = parseInt(d.Sf["LC"+n].value, 10); + len += parseInt(d.Sf["SL"+n].value, 10) || 0; // skipped LEDs are allocated too
204-206: Avoid loose equality for GPIO pin checkCoerce to number and use strict equality.
Apply:
- if (is8266() && d.Sf["L0"+n].value == 3) { //8266 DMA uses 5x the mem + if (is8266() && parseInt(d.Sf["L0"+n].value, 10) === 3) { // 8266 DMA uses 5x the mem
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/settings_leds.htm(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
🧠 Learnings (3)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: mval-sg
PR: wled/WLED#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.
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#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.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/data/settings_leds.htm
|
Yeah adding a recommendation to reboot sounds like a good addition @DedeHai |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/data/settings_leds.htm (1)
207-214: Fix bus-id comparisons and avoid double-counting I2S DMA buffer
- n is a chrID ('0'..'9','A'..), so numeric comparisons like n > 7 and n < 8 are wrong for 'A'.. (NaN), undercounting memory on buses ≥10.
- The parallel I2S DMA buffer is counted for every n<8 bus, overcounting by up to 8x. Count it once.
- let parallelI2S = d.Sf.PR.checked && (is32() || isS2() || isS3()) && !isD2P(t); - if (isC3() || (isS3() && !parallelI2S)) { + let parallelI2S = d.Sf.PR.checked && (is32() || isS2() || isS3()) && !isD2P(t); + const ni = parseInt(n, 36) || 0; // '0'..'9','A'.. -> 0..35 + if (isC3() || (isS3() && !parallelI2S)) { mul = 2; // ESP32 RMT uses double buffer - } else if ((is32() || isS2() || isS3()) && n > (parallelI2S ? 7 : 0)) { + } else if ((is32() || isS2() || isS3()) && ni > (parallelI2S ? 7 : 0)) { mul = 2; // ESP32 RMT uses double buffer - } else if ((parallelI2S && n < 8) || (n == 0 && is32())) { // I2S uses extra DMA buffer - dbl = len * ch * 3; // DMA buffer for parallel I2S (TODO: only the bus with largest LED count should be used) + } else if ((parallelI2S && ni === 0) || (ni === 0 && is32())) { // I2S uses extra DMA buffer (count once) + dbl = len * ch * 3; // TODO: size by the largest parallel bus, not bus 0 }Notes:
- Using ni fixes the numeric thresholds for 'A'.. buses.
- Counting DMA once avoids severe overestimation. For full accuracy, compute the largest (len*ch) across the 8 parallel buses once in UI() and remove per-bus dbl from getMem. I can provide that refactor if desired.
Also: nice catch on calling isS3() here—previous review concern resolved.
🧹 Nitpick comments (3)
wled00/data/settings_leds.htm (3)
196-200: Harden numeric parsing and clarify buffer commentGuard against NaN when fields are empty and clarify that 3x 32‑bit pixel buffers are included.
- let len = parseInt(d.Sf["LC"+n].value); - len += parseInt(d.Sf["SL"+n].value); // skipped LEDs are allocated too + let len = parseInt(d.Sf["LC"+n].value, 10) || 0; + let skip = parseInt(d.Sf["SL"+n].value, 10) || 0; // skipped LEDs are allocated too + len += skip; - let pbfr = len * 12; // pixel buffers: global buffer + 2*segment buffer (one is required, one for transitions) + let pbfr = len * 12; // 3x 32-bit pixel buffers: 1x global + 2x segment (transition default)
204-206: Use strict numeric comparison for 8266 DMA pin checkAvoid implicit coercion on the pin value.
- if (is8266() && d.Sf["L0"+n].value == 3) { //8266 DMA uses 5x the mem + if (is8266() && parseInt(d.Sf["L0"+n].value, 10) === 3) { // 8266 DMA uses 5x the mem mul = 5; }
384-391: Optional: add reboot hint when memory usage is highPer discussion, show a gentle reboot recommendation when usage is high to mitigate heap fragmentation issues on ESP8266.
gId('dbar').style.background = `linear-gradient(90deg, ${bquot > 60 ? (bquot > 90 ? "red":"orange"):"#ccc"} 0 ${bquot}%, #444 ${bquot}% 100%)`; gId('ledwarning').style.display = (maxLC > Math.min(maxPB,800) || bquot > 80) ? 'inline':'none'; gId('ledwarning').style.color = (maxLC > Math.max(maxPB,800) || bquot > 100) ? 'red':'orange'; - gId('wreason').innerHTML = (bquot > 80) ? "80% of max LED memory" +(bquot>100 ? ` (<b>WARNING: using over ${maxM}B!</b>)` : "") : "800 LEDs per output"; + const rebootHint = (bquot > 50) ? " Consider rebooting after adding outputs to reduce heap fragmentation." : ""; + gId('wreason').innerHTML = (bquot > 80) + ? "80% of max LED memory" + (bquot>100 ? ` (<b>WARNING: using over ${maxM}B!</b>)` : "") + rebootHint + : "800 LEDs per output";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/settings_leds.htm(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
🧠 Learnings (5)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: mval-sg
PR: wled/WLED#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.
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#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.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-09-01T10:26:17.959Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-09-04T06:34:21.724Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-04T06:34:21.724Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web headers (wled00/html_*.h)
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/data/settings_leds.htm
| var n = s.name.substring(2); | ||
| var t = parseInt(s.value); | ||
| memu += getMem(t, parseInt(n)); // calc memory | ||
| memu += getMem(t, n); // calc memory |
There was a problem hiding this comment.
n is used both as character and a number in getMem().
I prefer to have number instead of character.
There was a problem hiding this comment.
but isnt n passed as 0-9 and A-Z ? can 'A' be parsed into int?
There was a problem hiding this comment.
You are correct, but the solution presented here is not correct. n has to be converted.
There was a problem hiding this comment.
I see you added a fix to your fork, do you mind adding that to this PR too? I am struggling a bit with JS.
There was a problem hiding this comment.
I will once I get back from my trip.
There was a problem hiding this comment.
I think I managed to extract it. At least it worked in tests but please check.
wled00/data/settings_leds.htm
Outdated
| var nList = d.Sf.querySelectorAll("#mLC input[name^=L]"); | ||
| nList.forEach((LC,i)=>{ | ||
| let nm = LC.name.substring(0,2); // field name | ||
| let n = parseInt(LC.name.substring(2)); // bus number |
|
I am no longer too suer about this PR being a good idea "as is" for 0.16. At bootup, free heap is plenty, LED size is accepted and buffers are created, after wifi connection heap runs out and UI completely breaks down on ESP8266. This may need some adustment. The calculation is fine however for 0.15.2 porting. |
It means somebody modified cfg.json by uploading it or editing it using /edit endpoint. |
EDIT: or if someone updates from 0.15
curently, calculation for UI and bus creation differ. What I do not like is to limit it to the 4k including all buffers, that will just bring back #4614. ESP8266 can handle up to about 800 LEDs, maybe a bit less if there are no additional usermods etc. with my current test code. I may need to reduce that number to fully stabilize it. |
|
btw. what is the reason there are two functions to calculate bus memory? in polybus there is |
Are they? I don't think so. Unfortunately NPB does not provide memory estimation/requirement routines. And you can only get single buffer size out of it once bus is initialised (RMT may use 2 buffers). |
ah, got it. do we need both? decision to allocate a bus is made on the estimation, what is the "actual use" used for? also: you fixed only one of them :) |
ESP8266 does not use double buffering.
- new warnings for LED buffer use, deny adding a bus if it exceeds limits - adds recommendations for users (reboot, disable transitions)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/bus_wrapper.h (1)
1363-1363: Remove duplicate line 1363Line 1363 duplicates the assignment from line 1362.
if (_useParallelI2S) { if (num > 15) return I_NONE; if (num < 8) offset = 1; // 8 I2S followed by 8 RMT - if (num < 8) offset = 1; // 8 I2S followed by 8 RMT } else {wled00/data/settings_leds.htm (1)
208-214: Fix typos in comment- dbl = len * ch * 3; // DMA buffer for parallel I2S (TODO: ony the bus with largst LED count should be used) + dbl = len * ch * 3; // DMA buffer for parallel I2S (TODO: only the bus with the largest LED count should be used)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wled00/FX_fcn.cpp(1 hunks)wled00/bus_manager.cpp(3 hunks)wled00/bus_wrapper.h(4 hunks)wled00/cfg.cpp(1 hunks)wled00/data/settings_leds.htm(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/FX_fcn.cpp
- wled00/cfg.cpp
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware files
Files:
wled00/bus_wrapper.hwled00/bus_manager.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
🧠 Learnings (7)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#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: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: mval-sg
PR: wled/WLED#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: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
📚 Learning: 2025-09-02T01:45:58.047Z
Learnt from: willmmiles
PR: wled/WLED#4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:31-36
Timestamp: 2025-09-02T01:45:58.047Z
Learning: For the NeoEsp32RmtHI driver, RISC-V ESP32-C3 support is currently disabled via bus_wrapper.h rather than compile-time guards, as the maintainer willmmiles is working on resolving underlying nested interrupt issues and prefers to centralize the workaround in one location.
Applied to files:
wled00/bus_wrapper.h
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
PR: wled/WLED#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.
Applied to files:
wled00/bus_wrapper.hwled00/bus_manager.cppwled00/data/settings_leds.htm
📚 Learning: 2025-09-18T03:17:30.081Z
Learnt from: willmmiles
PR: wled/WLED#4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.081Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), digitalCount is intentionally incremented even for placeholder buses to maintain consistent resource mapping across all bus configurations.
Applied to files:
wled00/bus_wrapper.hwled00/bus_manager.cppwled00/data/settings_leds.htm
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#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.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/data/settings_leds.htm
📚 Learning: 2025-09-01T10:26:17.959Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/wled_eeprom.cpp:0-0
Timestamp: 2025-09-01T10:26:17.959Z
Learning: In WLED PR #4876, the DMXStartLED EEPROM backward compatibility issue was partially addressed by keeping it at address 2550 and reading it as a 16-bit value, with DMXChannelsValue array moved to addresses 2552-2566. This maintains compatibility with pre-0.11 EEPROM layouts for DMXStartLED, though legacy "Set to 255" (code 6) configurations may still need migration logic.
Applied to files:
wled00/data/settings_leds.htm
🧬 Code graph analysis (1)
wled00/bus_manager.cpp (2)
wled00/bus_manager.h (1)
BusDigital(234-285)wled00/bus_wrapper.h (3)
getDataSize(1111-1207)memUsage(1209-1276)isParallelI2S1Output(338-338)
⏰ 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). (8)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
🔇 Additional comments (9)
wled00/bus_wrapper.h (2)
472-485: Good work on the ESP32 RMT channel allocation fix!The channel adjustment logic correctly accounts for parallel I2S usage across different ESP32 variants. This addresses the issue where I2S bus at position 0 would conflict with RMT channels.
1116-1275: Ignore the 4× change—the existing multiplier is deliberate
getDataSize()’s 4× factor covers both the I2S front buffer and its DMA pipeline, whereasmemUsage()reports only the per-bus pixel buffer (excluding the shared DMA buffer managed later in BusManager). Removing the 4× would under-allocate the DMA backing required by NeoPixelBus’s I2S methods.Likely an incorrect or invalid review comment.
wled00/bus_manager.cpp (3)
327-328: LGTM - Proper I2S DMA buffer separationThe comment clearly indicates that the common I2S DMA buffer is accounted for separately in BusManager::memUsage(), avoiding double-counting.
773-799: Excellent I2S memory accounting implementation!The refactored memory calculation properly:
- Separates per-bus memory from shared I2S DMA buffers
- Uses the correct cadence factor (3 or 4 step) for DMA buffer sizing
- Only allocates I2S memory for the appropriate buses (first 8 for parallel, first 1 for single)
- Returns the total as
size + maxI2Spreventing double-counting
787-794: Validate I2S common buffer size calculationVerify that i2sCommonSize = stepFactor * bus->getLength() * bus->getNumberOfChannels() * (bus->is16bit()+1) matches NeoPixelBus's actual DMA buffer allocation — confirm the correct stepFactor (3 vs 4) for cadence and that bus->is16bit() should add one byte per channel.
Location: wled00/bus_manager.cpp (lines 787-794).
wled00/data/settings_leds.htm (4)
27-27: Good addition of the toNum helper functionThe
toNum()function correctly converts bus identifiers (0-9, A-Z) to numeric values (0-35), properly handling the expanded bus addressing.
195-217: Memory calculation properly accounts for pixel buffers and I2S DMAThe updated
getMem()function correctly:
- Adds 8-byte pixel buffers per LED (global + segment buffers)
- Handles parallel I2S DMA buffer allocation for the first 8 buses
- Uses the proper 3x cadence multiplier for I2S DMA
- Applies the correct multipliers based on bus type and position
389-389: UI memory warning text updated appropriatelyThe warning message correctly shows when memory usage exceeds 80% and includes an additional alert when over 100% of available memory.
60-105: Well-structured pin conflict validationThe refactored
pinsOK()function properly:
- Uses the new toNum() for bus identification
- Builds a comprehensive used-pin list from reserved and UI pins
- Validates against both reserved and read-only GPIO pins
- Provides clear error messages with the conflicting pin lists
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wled00/bus_wrapper.h (1)
1272-1273: Default case doubles buffer size for unknown bus typesLine 1272 adds a fallback that doubles the memory size for any unrecognized bus type. This could mask configuration errors or new bus types that haven't been properly added to the switch statement.
Consider logging a warning or using a more conservative default (e.g., no multiplication) to avoid silent memory overallocation.
- default : size *= 2; break; // everything else uses 2 buffers + default : /* size unchanged */ break; // unknown bus types use single bufferwled00/FX_fcn.cpp (1)
1189-1196: Consider extracting I2S detection logic for maintainabilityThe platform-specific I2S detection logic is complex and duplicated. Consider extracting it into a helper function for better maintainability.
+ #if !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(ESP8266) + static bool wouldUseI2S(unsigned digitalCount) { + #if defined(CONFIG_IDF_TARGET_ESP32) || defined(CONFIG_IDF_TARGET_ESP32S3) + return (useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == 1); + #elif defined(CONFIG_IDF_TARGET_ESP32S2) + return useParallelI2S && digitalCount <= 8; + #else + return false; + #endif + } + #endif + // estimate maximum I2S memory usage (only relevant for digital non-2pin busses) #if !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(ESP8266) - #if defined(CONFIG_IDF_TARGET_ESP32) || defined(CONFIG_IDF_TARGET_ESP32S3) - const bool usesI2S = ((useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == 1)); - #elif defined(CONFIG_IDF_TARGET_ESP32S2) - const bool usesI2S = (useParallelI2S && digitalCount <= 8); - #else - const bool usesI2S = false; - #endif + const bool usesI2S = wouldUseI2S(digitalCount);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FX_fcn.cpp(1 hunks)wled00/bus_wrapper.h(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for all C++ source and header files
Files:
wled00/FX_fcn.cppwled00/bus_wrapper.h
🧠 Learnings (13)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#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: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: mval-sg
PR: wled/WLED#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.
📚 Learning: 2025-09-16T18:08:42.848Z
Learnt from: DedeHai
PR: wled/WLED#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.
Applied to files:
wled00/FX_fcn.cppwled00/bus_wrapper.h
📚 Learning: 2025-09-18T03:17:30.081Z
Learnt from: willmmiles
PR: wled/WLED#4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.081Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), digitalCount is intentionally incremented even for placeholder buses to maintain consistent resource mapping across all bus configurations.
Applied to files:
wled00/FX_fcn.cppwled00/bus_wrapper.h
📚 Learning: 2025-09-18T03:17:30.081Z
Learnt from: willmmiles
PR: wled/WLED#4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.081Z
Learning: In WLED BusManager (wled00/FX_fcn.cpp), direct access to BusManager::busses is part of the intended public API design, not an encapsulation violation. The wrapper accessor methods are considered unnecessary obfuscation that should be removed in future refactoring.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-09-18T02:59:33.600Z
Learnt from: willmmiles
PR: wled/WLED#4928
File: wled00/FX_fcn.cpp:1181-1181
Timestamp: 2025-09-18T02:59:33.600Z
Learning: In WLED, getFreeHeapSize() is a platform-agnostic function defined in fcn_declare.h that provides accurate free heap reporting across ESP32 and ESP8266 platforms. On ESP32, it uses heap_caps_get_free_size(MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT) instead of ESP.getFreeHeap() because the latter can include other memory types. On ESP8266, it uses ESP.getFreeHeap().
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-08-29T15:38:46.208Z
Learnt from: DedeHai
PR: wled/WLED#4791
File: wled00/FX_fcn.cpp:1187-1191
Timestamp: 2025-08-29T15:38:46.208Z
Learning: In WLED's allocate_buffer() function, BFRALLOC_ENFORCE_PSRAM already includes fallback logic to DRAM if PSRAM is not available, as documented in the comment "use PSRAM if available, otherwise fall back to DRAM". The function also uses validateFreeHeap() for additional safety checks. During setup() when finalizeInit() runs, PSRAM has vast available memory making failures unlikely.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#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.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-09-15T19:13:56.469Z
Learnt from: DedeHai
PR: wled/WLED#4791
File: wled00/util.cpp:737-743
Timestamp: 2025-09-15T19:13:56.469Z
Learning: In WLED's util.cpp, the *_realloc_malloc functions (p_realloc_malloc and d_realloc_malloc) are intentionally designed to free the original buffer on realloc failure and allocate a new buffer, implementing a "replace buffer" semantic rather than traditional realloc behavior. This is documented in the function comments and is the intended design by the author DedeHai.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-09-18T03:17:30.081Z
Learnt from: willmmiles
PR: wled/WLED#4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.081Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-08-29T01:34:34.358Z
Learnt from: willmmiles
PR: wled/WLED#4853
File: wled00/util.cpp:779-781
Timestamp: 2025-08-29T01:34:34.358Z
Learning: On ESP8266 systems, avoid adding no-op stub functions across translation units due to limited code memory constraints, as the compiler cannot inline away the function calls, resulting in wasteful memory usage.
Applied to files:
wled00/FX_fcn.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX_fcn.cpp
🧬 Code graph analysis (1)
wled00/FX_fcn.cpp (4)
wled00/fcn_declare.h (2)
getFreeHeapSize(462-462)getFreeHeapSize(465-465)wled00/bus_manager.h (7)
isDigital(141-142)isDigital(180-180)is2Pin(181-181)hasRGB(163-165)hasWhite(166-172)hasCCT(173-178)is16bit(185-185)wled00/bus_manager.cpp (8)
add(76-81)add(76-76)add(801-822)add(801-801)memUsage(759-770)memUsage(759-759)memUsage(773-799)memUsage(773-773)wled00/bus_wrapper.h (1)
memUsage(1209-1276)
⏰ 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). (2)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
🔇 Additional comments (6)
wled00/bus_wrapper.h (1)
1116-1163: ESP8266 memory calculation refactoring looks goodThe removal of doubling (*2) factors for ESP8266 UART and BitBang bus types aligns with the new I2S-aware memory accounting approach. These bus types no longer artificially inflate their memory requirements, which should provide more accurate memory usage reporting.
wled00/FX_fcn.cpp (5)
1181-1181: Good addition of heap size diagnosticAdding heap size printout before bus creation helps with debugging memory issues, especially relevant for the ESP8266 stability concerns mentioned in the PR comments.
1183-1217: I2S memory accounting implementation addresses the core issueThis implementation correctly accounts for I2S DMA buffer memory that was previously excluded from memory calculations. The architecture-specific handling for ESP32 variants (S2, S3, C3) and the cadence step factor configuration align with the NeoPixelBus I2S implementation details.
The combined check
mem + maxI2S <= MAX_LED_MEMORYensures that both regular buffers and the shared I2S DMA buffer fit within the memory limit, directly addressing the issue where configurations with >659 LEDs were failing to save.
1203-1204: I2S common buffer calculation is correctThe calculation properly accounts for:
- Cadence step factor (3 or 4 bits per pixel bit)
- Number of channels (RGB/White/CCT)
- 16-bit bus types
This matches the actual memory requirements of the NeoPixelBus I2S implementation.
1212-1212: ErrorFlag setting was removedBased on past review comments, the
errorFlag = ERR_NORAM_PX;assignment was removed here during merge conflict resolution but should be restored to properly indicate memory allocation failures to the UI.
1207-1210: Early termination on memory overflow prevents partial bus creationBreaking the loop when memory limit is exceeded ensures that buses are created in order until memory is exhausted, rather than skipping individual buses. This maintains a predictable initialization sequence.
| case I_8266_DM_SM16825_5: size = (size + 2*count)*2*5; break; | ||
| #endif | ||
| #ifdef ARDUINO_ARCH_ESP32 | ||
| // RMT buses (1x front and 1x back buffer) | ||
| case I_32_RN_NEO_4: size = (size + count)*2; break; | ||
| case I_32_RN_TM1_4: size = (size + count)*2; break; | ||
| case I_32_RN_UCS_3: size *= 2*2; break; | ||
| case I_32_RN_UCS_4: size = (size + count)*2*2; break; | ||
| case I_32_RN_FW6_5: size = (size + 2*count)*2; break; | ||
| case I_32_RN_2805_5: size = (size + 2*count)*2; break; | ||
| case I_32_RN_SM16825_5: size = (size + 2*count)*2*2; break; | ||
| // I2S1 bus or paralell buses (individual 1x front and 1 DMA (3x or 4x pixel count) or common back DMA buffers) | ||
| #else | ||
| // RMT buses (1x front and 1x back buffer, does not include small RMT buffer) | ||
| case I_32_RN_NEO_4 : // fallthrough | ||
| case I_32_RN_TM1_4 : size = (size + count)*2; break; // 4 channels | ||
| case I_32_RN_UCS_3 : size *= 2*2; break; // 16bit | ||
| case I_32_RN_UCS_4 : size = (size + count)*2*2; break; // 16bit, 4 channels | ||
| case I_32_RN_FW6_5 : // fallthrough | ||
| case I_32_RN_2805_5 : size = (size + 2*count)*2; break; // 5 channels | ||
| case I_32_RN_SM16825_5: size = (size + 2*count)*2*2; break; // 16bit, 5 channels | ||
| // I2S1 bus or paralell I2S1 buses (1x front, does not include DMA buffer which is front*cadence, a bit(?) more for LCD) | ||
| #ifndef CONFIG_IDF_TARGET_ESP32C3 | ||
| case I_32_I2_NEO_3: size *= 4; break; | ||
| case I_32_I2_NEO_4: size = (size + count)*4; break; | ||
| case I_32_I2_400_3: size *= 4; break; | ||
| case I_32_I2_TM1_4: size = (size + count)*4; break; | ||
| case I_32_I2_TM2_3: size *= 4; break; | ||
| case I_32_I2_UCS_3: size *= 2*4; break; | ||
| case I_32_I2_UCS_4: size = (size + count)*2*4; break; | ||
| case I_32_I2_APA106_3: size *= 4; break; | ||
| case I_32_I2_FW6_5: size = (size + 2*count)*4; break; | ||
| case I_32_I2_2805_5: size = (size + 2*count)*4; break; | ||
| case I_32_I2_TM1914_3: size *= 4; break; | ||
| case I_32_I2_SM16825_5: size = (size + 2*count)*2*4; break; | ||
| case I_32_I2_NEO_3 : // fallthrough | ||
| case I_32_I2_400_3 : // fallthrough | ||
| case I_32_I2_TM2_3 : // fallthrough | ||
| case I_32_I2_APA106_3 : break; // do nothing, I2S uses single buffer + DMA buffer | ||
| case I_32_I2_NEO_4 : // fallthrough | ||
| case I_32_I2_TM1_4 : size = (size + count); break; // 4 channels | ||
| case I_32_I2_UCS_3 : size *= 2; break; // 16 bit | ||
| case I_32_I2_UCS_4 : size = (size + count)*2; break; // 16 bit, 4 channels | ||
| case I_32_I2_FW6_5 : // fallthrough | ||
| case I_32_I2_2805_5 : size = (size + 2*count); break; // 5 channels | ||
| case I_32_I2_SM16825_5: size = (size + 2*count)*2; break; // 16 bit, 5 channels | ||
| #endif | ||
| default : size *= 2; break; // everything else uses 2 buffers | ||
| #endif | ||
| // everything else uses 2 buffers | ||
| default: size *= 2; break; | ||
| } | ||
| return size; |
There was a problem hiding this comment.
🧩 Analysis chain
Missing parallel I2S bus memory accounting in memUsage()
The comment on line 1258 mentions "I2S1 bus or paralell I2S1 buses" but the memory calculation doesn't account for parallel I2S when _useParallelI2S is true. This inconsistency with the getDataSize() function (lines 1181-1192) could lead to incorrect memory estimates.
Consider aligning this with the actual memory requirements when parallel I2S is in use.
🏁 Script executed:
#!/bin/bash
# Verify if parallel I2S flag is used in memUsage calculation elsewhere
rg -n "_useParallelI2S" wled00/bus_wrapper.h -C3Length of output: 22713
memUsage(): account for parallel I2S (_useParallelI2S) for I2S cases
getDataSize() uses useParallelI2S (wled00/bus_wrapper.h:1178–1192) but memUsage() does not (wled00/bus_wrapper.h:1248–1275); update the I_32_I2* branches in memUsage() to check _useParallelI2S and compute size consistent with getDataSize() (i.e., mirror the IP vs I2 class accounting / use the IP bus PixelsSize()*4 where appropriate).
🤖 Prompt for AI Agents
In wled00/bus_wrapper.h around lines 1248–1275, memUsage() currently treats all
I_32_I2_* cases the same and does not consider _useParallelI2S; update the
I_32_I2_* branches to mirror getDataSize() (lines ~1178–1192) by checking
_useParallelI2S and computing size as the IP-class path when parallel I2S is
used (i.e., account using the IP bus PixelsSize()*4 semantics) and otherwise use
the non-parallel I2 accounting (single buffer + DMA buffer semantics), so the
memUsage() result matches getDataSize() for both parallel and non-parallel I2S
configurations.
There was a problem hiding this comment.
the reason _useParallelI2S is used is to ge the correct function pointer, not to change the math.
There was a problem hiding this comment.
@DedeHai Thank you for the clarification! You're absolutely right. Looking at the getDataSize() function, I can see that both the parallel I2S path (static_cast<B_32_IP_*>()->PixelsSize()*4) and non-parallel I2S path (static_cast<B_32_I2_*>()->PixelsSize()*4) return the same calculation - PixelsSize()*4. The _useParallelI2S flag is indeed used to select the correct bus type/function pointer rather than to change the underlying memory math.
The memory requirements are fundamentally the same for both configurations, so the memUsage() function correctly doesn't need to differentiate based on the parallel I2S flag. I appreciate you pointing this out!
✏️ Learnings added
Learnt from: DedeHai
PR: wled/WLED#4928
File: wled00/bus_wrapper.h:1248-1275
Timestamp: 2025-09-21T20:44:09.492Z
Learning: In WLED bus_wrapper.h, the _useParallelI2S flag is used for selecting the correct function pointer (between parallel I2S and non-parallel I2S bus types) rather than for changing memory calculations. Both parallel and non-parallel I2S configurations have the same memory requirements, so memUsage() correctly doesn't differentiate based on this flag.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: willmmiles
PR: wled/WLED#4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.081Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), digitalCount is intentionally incremented even for placeholder buses to maintain consistent resource mapping across all bus configurations.
Learnt from: DedeHai
PR: wled/WLED#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: blazoncek
PR: wled/WLED#4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: mval-sg
PR: wled/WLED#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.
* Attempt at better bus memory calculation and estimation * Remov double buffer count for ESP8266 (thanks @DedeHai) * improve UI calculation * adding mendatory LED buffers to UI memory calculation * adding buffer for transitions to memory calculation, change "error" to "warning" * bugfixes in settings_leds.htm * fix getDataSize() forESP8266, ESP8266 does not use double buffering. * update led settings: fix parsing by @blazoncek * new warnings for LED buffer use, deny adding a bus if it exceeds limits * adds recommendations for users (reboot, disable transitions) Co-authored-by: Blaž Kristan <blaz@kristan-sp.si>
fixes #4614 "Hardware Setup Length can't be saved after 659"
this fix also nees to be back-ported to 0.15
@willmmiles can you please check this is correct? I am not 100% sure I fixed the conflicts correctly, it did work when testing on ESP32 and ESP8266.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor