fix buffer size calculation for ESP8266 (0.15 edition) #4951
Conversation
also adds proper global buffer calculation and RMT/I2S channel calculation thx to @blazoncek
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors ESP32-specific channel mapping and memory sizing in bus creation, adding per-target branches for ESP32, ESP32-S2, and ESP32-S3, and revises data size calculations across ESP8266/ESP32 code paths. Updates UI memory estimation for digital LEDs to use a fixed 4 bytes per LED for double buffering. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (2)
wled00/bus_wrapper.h (2)
472-485: RMT channel remap looks right; add guardrails for max channel count per target.Adjustment logic for classic ESP32, S2, and S3 matches the bus ordering decisions. To harden it, clamp the final channel to the valid RMT range for the target (e.g., 0–7 on ESP32, 0–3 on S2/S3), and assert in debug builds to catch misconfigurations. Also consider a short inline helper to document the mapping.
Can you run a quick sanity check across targets with and without parallel I2S (first RMT bus should resolve to channel 0 in all cases)?
1311-1369: memUsage() model looks consistent with backend buffers; keep defaults explicit.The separation between ESP8266 DMA (5x) and ESP32 RMT/I2S (2x front/back; I2S front+DMA) is sound. Minor: consider replacing the default “size *= 2” with explicit cases for the remaining digital types to avoid accidental future catch‑alls.
Please confirm UI getMem() totals now match these backend numbers within a small delta for representative configs (UART 3‑ch, DMA 3‑ch, 16‑bit UCS, and RGBW).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/bus_wrapper.h(3 hunks)wled00/data/settings_leds.htm(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 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: 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: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
📚 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-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/data/settings_leds.htmwled00/bus_wrapper.h
📚 Learning: 2025-09-20T09:00:08.933Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.933Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated headers wled00/html_*.h
Applied to files:
wled00/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-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/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 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/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/bus_wrapper.h
🔇 Additional comments (1)
wled00/bus_wrapper.h (1)
1212-1260: ESP8266 getDataSize() corrections LGTM.Using PixelsSize() for UART/bitbang paths and keeping 5x for DMA aligns with actual NPB allocations and 16‑bit features. This should fix the overestimation regressions on 0.15.
this is a back-port from @blazoncek fork with some additions
Summary by CodeRabbit
Bug Fixes
Refactor