Improved bus handling: free choice of bus driver in any order and improved memory calculations #5303
Conversation
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
- collaboration with copilot, still needs plenty of cleanup
Refactored memory estimation and bus allocation to eliminate code duplication: **Changes:** 1. **Added `iType` field to BusConfig struct** - Stores the determined bus type (I_*) after memory estimation - Initialized to I_NONE in constructor - Allows reuse of bus type determination across estimation and allocation phases 2. **Modified BusConfig::memUsage()** - Now calls `PolyBus::getI()` to determine bus type and allocate channel (single call) - Stores result in `iType` field for later reuse - Uses const_cast to modify iType in const method (safe since it's memoization) 3. **Modified BusDigital constructor** - Reuses `bc.iType` instead of calling `getI()` again - Eliminates double-counting of channels - Comment explains that iType comes from memory estimation phase 4. **Removed determineBusType() helper** - No longer needed since getI() is called only once - Eliminated ~90 lines of duplicated switch-case logic - Simplified bus_wrapper.h by removing helper function 5. **Integrated bus type logic into getI()** - getI() now directly contains the switch-case for bus type determination - Channel allocation and type determination happen atomically - No separate function call needed **Benefits:** - ✅ Eliminates code duplication (~90 lines removed) - ✅ Single source of truth for bus type determination - ✅ getI() called only once per bus (during memUsage) - ✅ No more helper function complexity - ✅ Cleaner separation: estimate→store→reuse - ✅ Channel tracking remains accurate **How it works:** 1. FX_fcn.cpp calls `bus.memUsage()` for memory estimation 2. memUsage() calls `getI()` which allocates channel and returns iType 3. iType stored in BusConfig for later use 4. BusDigital constructor reuses stored iType instead of calling getI() again 5. Result: Each bus uses exactly one channel, no double-counting **Testing:** - ✅ Web UI built successfully - ✅ All 16 npm tests passed - ✅ No compilation errors - ✅ Code significantly simplified Co-authored-by: DedeHai <[email protected]>
Fixed critical bug in I2S memory estimation where parallel I2S wasn't accounting for 8x larger DMA buffer: **Problem:** - Parallel I2S uses 8 channels simultaneously (I2S0-I2S7) - Each channel requires its own DMA buffer - Total DMA buffer is 8x larger than single I2S mode - Both UI and firmware were underestimating memory by 8x **Changes:** 1. **FX_fcn.cpp (firmware estimation):** - Added 8x multiplier when `hasParallelOutput()` is true - Applied after calculating base i2sCommonSize - Only affects parallel I2S mode, single I2S unchanged 2. **settings_leds.htm (UI estimation):** - Added check for parallel I2S checkbox (`d.Sf.PI.checked`) - Multiplies DMA buffer size by 8 when parallel mode enabled - Provides accurate memory estimate to users before saving **Example Impact:** - Single I2S with 300 LEDs: ~2.7KB DMA buffer - Parallel I2S with 300 LEDs: ~21.6KB DMA buffer (8x) - Without fix: UI showed 2.7KB, firmware allocated 21.6KB → confusion - With fix: Both correctly show and allocate 21.6KB **Technical Details:** - Parallel I2S (X8 methods) drive 8 output pins simultaneously - Each pin needs independent DMA buffer for timing - NeoPixelBus library allocates 8 separate buffers internally - Memory estimation must match actual allocation **Testing:** - ✅ Web UI built successfully - ✅ All 16 npm tests passed - ✅ No compilation errors - ✅ Accurate memory estimation for both modes Co-authored-by: DedeHai <[email protected]>
Completely reworked LED settings UI validation to properly handle RMT/I2S driver constraints: **New Validation Rules Implemented:** 1. **RMT Buses:** - Can use any LED type without restriction - Limited only by hardware RMT channel count (ESP32: 8, S2/S3: 4, C3: 2) 2. **I2S Buses:** - Must all be of the same LED type - First I2S bus in list determines the type for all subsequent I2S buses - LED type dropdown automatically restricts options for additional I2S buses 3. **Parallel I2S Validation:** - Requires all I2S buses to have ≤600 LEDs - If any I2S bus has >600 LEDs, parallel mode is disabled - Only single I2S bus allowed when >600 LEDs - Driver selection dropdown turns red when violating this rule 4. **Dynamic Driver Selection:** - When RMT channels full, RMT option disabled in other buses' dropdowns - When I2S channels full, I2S option disabled in other buses' dropdowns - Current bus selection always stays enabled (can change existing assignment) - Updates in real-time as user changes driver selections 5. **Automatic Updates:** - Changing LED type triggers validation update - Changing driver (RMT/I2S) triggers validation update - Changing LED count triggers validation update (checks 600 LED threshold) - All dropdowns re-validate when any change occurs **Technical Changes:** 1. **updateTypeDropdowns() function:** - Three-pass algorithm for accurate validation - Pass 1: Count buses and identify I2S constraints (first type, max LEDs) - Pass 2: Update LED type dropdowns with appropriate restrictions - Pass 3: Update driver selection dropdowns with availability constraints 2. **UI() function improvements:** - Enhanced bus tracking with I2S-specific validation - Tracks >600 LED violations for parallel I2S rules - Colors driver dropdown red when configuration invalid - Accurately counts RMT vs I2S channel usage with fallback logic 3. **Event handlers updated:** - Driver dropdown onChange now calls updateTypeDropdowns() - LED count input onInput now calls updateTypeDropdowns() - LED type dropdown onChange already called updateTypeDropdowns() **Benefits:** - ✅ Guides users to valid configurations only - ✅ Clear visual feedback (red = invalid, white = valid) - ✅ Prevents invalid I2S mixed-type configurations - ✅ Enforces parallel I2S LED count limit (≤600) - ✅ Dynamic channel availability updates - ✅ Maintains flexibility for RMT buses (no restrictions) - ✅ Intuitive UX - options disappear when unavailable **Testing:** - ✅ Web UI built successfully - ✅ All 16 npm tests passed - ✅ No compilation errors Co-authored-by: DedeHai <[email protected]>
…etChannelTracking Fixed code quality issues identified in review: **Issue wled#1: Duplicate resetChannelTracking() method** - Removed duplicate `resetChannelTracking()` static method in PolyBus class - Was defined at both line 1289 and line 1423 in bus_wrapper.h - Kept single definition, eliminated redundancy **Issue wled#2: Code duplication in channel tracking** - Extracted duplicated channel counting logic into `calculateChannelUsage()` helper function - Was duplicated in UI() function (lines ~445-500) and updateTypeDropdowns() (lines ~997-1023) - Helper function returns object with all tracking metrics: - rmtUsed, i2sUsed: channel usage counts - firstI2SType: LED type of first I2S bus - maxLEDsOnI2SBus: highest LED count on any I2S bus - i2sBusesOver600: count of I2S buses exceeding 600 LEDs - parallelI2SAllowed: whether parallel mode is viable - Both UI() and updateTypeDropdowns() now call shared helper - Ensures consistent logic across validation passes - Reduced ~50 lines of duplicated code **Issue wled#3: Event handler ordering** - Changed LED count input event handler from `oninput="updateTypeDropdowns();UI()"` to `oninput="UI();updateTypeDropdowns()"` - Ensures channel counts recalculated by UI() before validation logic runs - Prevents potential stale data in validation **Benefits:** - ✅ Eliminates code duplication (~50 lines reduced) - ✅ Single source of truth for channel tracking - ✅ Easier maintenance - logic changes only need one update - ✅ Prevents logic drift between functions - ✅ Cleaner, more maintainable codebase - ✅ Fixed timing issue with LED count validation **Testing:** - ✅ Web UI built successfully - ✅ All 16 npm tests passed - ✅ No compilation errors - ✅ Validation logic unchanged, just refactored - ✅ File size slightly changed: 54256→54256 bytes (same after minification) Co-authored-by: DedeHai <[email protected]>
Aligned firmware bus validation with UI ruleset for consistent I2S/RMT handling: **Updated finalizeInit() Logic:** 1. **I2S Bus Type Validation:** - Tracks only I2S buses (driverType == 1) instead of all buses - All I2S buses must be same LED type for parallel mode - RMT buses can be any type without restriction 2. **600 LED Restriction:** - ESP32-S3: All I2S buses (LCD driver) must have ≤600 LEDs - ESP32/S2: Only multiple I2S buses require ≤600 LEDs each - Single I2S bus: No LED count restriction 3. **Parallel I2S Requirements:** - All I2S buses same LED type (checked) - LED count validation based on platform and bus count (checked) - Single I2S bus bypasses 600 LED limit **Key Changes:** - Replaced `mixedBusTypes` (all buses) with `mixedI2SBusTypes` (only I2S buses) - Added `i2sBusCount` to distinguish single vs multiple I2S buses - Platform-specific LED count validation (#ifdef CONFIG_IDF_TARGET_ESP32S3) - Checks `bus.driverType == 1` to identify I2S buses - Improved debug output shows I2S-specific metrics **Behavior:** - RMT buses: No restrictions except hardware channel limits - I2S buses: Must match first I2S bus type - Parallel I2S: Enabled only when all requirements met - Single I2S: Falls back when requirements not met (mixed types or >600 LEDs) **Benefits:** - ✅ Firmware matches UI validation logic - ✅ Consistent behavior across UI and firmware - ✅ Single I2S supports unlimited LEDs - ✅ Multiple I2S enforces 600 LED limit - ✅ S3 always enforces 600 LED limit for I2S/LCD - ✅ RMT buses fully flexible **Testing:** - ✅ Web UI built successfully - ✅ All 16 npm tests passed - ✅ No compilation errors - ✅ Logic matches UI calculateChannelUsage() Co-authored-by: DedeHai <[email protected]>
the 600 LEDs per I2S bus in parallel mode is purely artificial, it should be only restricted by memory not as an arbitrary number
- removed unused functions - removed memUsage() from busmanaer as it is not used anymore (and was not before except for debug output)
…266, skip final check on ESP8266
…x LED memory to compensate
Contributor
WalkthroughRefactors bus APIs to add per-bus driver selection and iType resolution via BusManager::getI(); changes BusDigital construction to accept BusConfig only; centralizes channel/I2S tracking and memory budgeting (separating I2S DMA); surfaces driverType in serialization and the frontend. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wled00/data/settings_leds.htm (1)
239-251: Avoid stale I2S preference affecting 2‑pin or non‑I2S buses.
If a bus previously had I2S selected, the hiddenLDvalue can carry over when switching to a 2‑pin type, causing under/over‑estimation. Consider forcingLD=0or ignoring it for non‑eligible buses.🧩 Suggested adjustment
- let driverPref = d.Sf["LD"+n] ? parseInt(d.Sf["LD"+n].value || 0) : 0; // driver preference selection: 0=RMT, 1=I2S - if (driverPref == 1) { // I2S to be used + let driverPref = (!isD2P(t) && d.Sf["LD"+n]) ? parseInt(d.Sf["LD"+n].value || 0) : 0; // driver preference selection: 0=RMT, 1=I2S + if (driverPref == 1) { // I2S to be used mul = 1; // NPB uses single pixel buffer for I2S, DMA buffer serves as second buffer let usage = getDuse(); dma = usage.I2Smem; // DMA buffer for I2S/LCD, getDuse() returns the average per I2S bus so it can be distributed and summed }wled00/bus_manager.cpp (1)
119-169: Initialize_busPtrtonullptrand callcleanup()on earlyI_NONEreturn to prevent undefined behavior.When
_iType == I_NONEis detected after pin allocation, the constructor returns early without cleanup. The destructor will still be called (RAII), and it invokescleanup()which passes the uninitialized_busPtrpointer toPolyBus::cleanup(_busPtr, _iType), causing undefined behavior.Initialize
_busPtr(nullptr)in the member initializer list and callcleanup()before the early return to release allocated pins immediately and ensure safe cleanup.🐛 Proposed fix
BusDigital::BusDigital(const BusConfig &bc, uint8_t nr) : Bus(bc.type, bc.start, bc.autoWhite, bc.count, bc.reversed, (bc.refreshReq || bc.type == TYPE_TM1814)) , _skip(bc.skipAmount) //sacrificial pixels , _colorOrder(bc.colorOrder) +, _busPtr(nullptr) , _milliAmpsPerLed(bc.milliAmpsPerLed) , _milliAmpsMax(bc.milliAmpsMax) { DEBUGBUS_PRINTLN(F("Bus: Creating digital bus.")); if (!isDigital(bc.type) || !bc.count) { DEBUGBUS_PRINTLN(F("Not digial or empty bus!")); return; } if (!PinManager::allocatePin(bc.pins[0], true, PinOwner::BusDigital)) { DEBUGBUS_PRINTLN(F("Pin 0 allocated!")); return; } _frequencykHz = 0U; _colorSum = 0; _pins[0] = bc.pins[0]; if (is2Pin(bc.type)) { if (!PinManager::allocatePin(bc.pins[1], true, PinOwner::BusDigital)) { cleanup(); DEBUGBUS_PRINTLN(F("Pin 1 allocated!")); return; } _pins[1] = bc.pins[1]; _frequencykHz = bc.frequency ? bc.frequency : 2000U; // 2MHz clock if undefined } _iType = PolyBus::getI(bc.type, _pins, nr); - if (_iType == I_NONE) { DEBUGBUS_PRINTLN(F("Incorrect iType!")); return; } + if (_iType == I_NONE) { DEBUGBUS_PRINTLN(F("Incorrect iType!")); cleanup(); return; }wled00/bus_wrapper.h (1)
1291-1400: Bus allocation should fall back to RMT when I2S preference is unavailable.When a bus requests I2S (driverPreference=1) but all I2S channels are exhausted, the function returns
I_NONEand fails bus creation—even when RMT channels remain available. ThedriverPreferenceparameter should be treated as a preference, not a hard requirement. Implement fallback logic to use RMT as an alternative when the preferred driver is unavailable.Proposed fix
- if (driverPreference == 0 && _rmtChannelsAssigned < WLED_MAX_RMT_CHANNELS) { - _rmtChannelsAssigned++; - } else if (_i2sChannelsAssigned < WLED_MAX_I2S_CHANNELS) { - offset = 1; // I2S requested or RMT full - _i2sChannelsAssigned++; - } else { - return I_NONE; // No channels available - } + const bool wantI2S = (driverPreference != 0); + const bool rmtAvail = (_rmtChannelsAssigned < WLED_MAX_RMT_CHANNELS); + const bool i2sAvail = (_i2sChannelsAssigned < WLED_MAX_I2S_CHANNELS); + + if (wantI2S && i2sAvail) { + offset = 1; + _i2sChannelsAssigned++; + } else if (rmtAvail) { + _rmtChannelsAssigned++; + } else if (i2sAvail) { + offset = 1; + _i2sChannelsAssigned++; + } else { + return I_NONE; // No channels available + }
🤖 Fix all issues with AI agents
In `@wled00/bus_manager.cpp`:
- Around line 359-361: Add a virtual isI2S() declaration to the base class Bus
and change BusDigital::isI2S() to be a const override so calls through Bus*
compile; specifically, declare virtual bool isI2S() const = false; (or return
false default) in class Bus, then update the BusDigital method signature to bool
isI2S() const override and keep its implementation, and ensure callers like
esp32RMTInvertIdle() that iterate the busses vector
(std::vector<std::unique_ptr<Bus>>) call the const method correctly.
In `@wled00/bus_wrapper.h`:
- Around line 1166-1193: The 100-byte ESP32 overhead initialization (size = 100)
is currently placed between case labels and therefore never executed; move the
initialization so it runs before the switch that handles ESP32 bus types (i.e.,
set size = 100 prior to evaluating the switch that contains cases like
I_32_RN_NEO_3, I_32_I2_NEO_3, etc.) so that the base overhead is always added,
then keep the existing per-bus additions that reference busPtr and PixelsSize()
(and preserve the _useParallelI2S conditional logic for I2S cases).
In `@wled00/const.h`:
- Around line 56-88: Update the comment inside the CONFIG_IDF_TARGET_ESP32S3
block that currently reads "ESP32‑S2 has 4 RMT output channels" to correctly
reference "ESP32‑S3" so it matches the surrounding defines
(WLED_MAX_RMT_CHANNELS, WLED_MAX_I2S_CHANNELS, WLED_MIN_VIRTUAL_BUSSES) in that
block; ensure only the comment text is changed and the macro names/values
(WLED_MAX_RMT_CHANNELS, WLED_MAX_I2S_CHANNELS, WLED_MIN_VIRTUAL_BUSSES) remain
unchanged.
In `@wled00/data/settings_leds.htm`:
- Around line 934-938: The I2S DMA memory estimation underestimates 16‑bit LED
types because the I2S calc in the I2SType block doesn't account for the 16‑bit
multiplier used elsewhere (e.g., getDuse()); update the I2S memory calculation
to multiply the per‑LED byte/channel count by the 16‑bit factor when the LED
type is 16‑bit. Specifically, in the block calculating ch and I2Smem
(references: I2SType, hasRGB(), hasW(), hasCCT(), I2Smem, maxLEDs, i2sUsed,
isS3()), detect 16‑bit digital types (the same condition or helper used by
getDuse()) and apply the multiplier before computing I2Smem so the estimated
memory matches firmware checks.
In `@wled00/FX_fcn.cpp`:
- Around line 1168-1213: The memory/I2S detection uses the user preference
bus.driverType instead of the resolved driver in bus.iType (set by
BusManager::getI()), so replace checks that use bus.driverType == 1 (e.g., the
i2sBusCount increment and the usesI2S computation before DMA/mem accounting)
with checks against bus.iType (or a helper like "(bus.iType & 0x01) == 0" if
that identifies I2S) and any Bus::isDigital/Bus::is2Pin guards; ensure all
places that decide useParallelI2S, i2sBusCount, and I2SdmaMem/mem estimation
base their logic on bus.iType (or a small helper function) rather than the
user-supplied driverType so the memory calculation matches the actual resolved
driver from BusManager::getI().
🧹 Nitpick comments (1)
wled00/data/settings_leds.htm (1)
1-10: Remember to regenerate embedded UI headers after modifying settings pages.
Please run the UI build step so the firmware embeds the updated HTML/JS. As per coding guidelines, runnpm run buildafter changes underwled00/data/**.
Collaborator
Author
|
@coderabbitai when saving a new bus configuration in the UI it can sometimes trigger a "CORRUPT HEAP: Bad head". I did not observe this prior to this PR. Please check if any of the changes cause this. |
This comment was marked as resolved.
This comment was marked as resolved.
DedeHai
added
hardware
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@wled00/bus_manager.cpp`:
- Around line 144-159: The code returns early when _iType == I_NONE after pins
have been allocated, leaking the reserved pins; update the finalizeInit() flow
so that when _iType == I_NONE you release resources before returning (e.g., call
cleanup() or otherwise free _pins/_busPtr) instead of returning
immediately—modify the block that currently does "if (_iType == I_NONE) {
DEBUGBUS_PRINTLN(...); return; }" to invoke cleanup() (or the appropriate
pin-release routine) then return, ensuring _busPtr, _pins and related state are
cleared; reference symbols: _iType, I_NONE, finalizeInit(), _pins, _busPtr,
cleanup(), PolyBus::create.
In `@wled00/data/settings_leds.htm`:
- Around line 918-927: The DMA estimate only uses LC to compute ledCount,
ignoring skipped LEDs (SL) which still allocate DMA; update the calculation in
the block using sel.name/ n so ledCount = parseInt(d.Sf["LC"+n].value || 0) plus
parseInt(d.Sf["SL"+n].value || 0) (with safe fallbacks), and then use that
adjusted ledCount when updating maxLEDs and any other DMA-related logic
(references: sel.name, t, driverPref, ledCount, I2SType, maxLEDs).
In `@wled00/fcn_declare.h`:
- Around line 448-458: The d_free and p_free helpers currently take a void* by
value so assigning nullptr only affects the local parameter, leaving callers
(e.g., the member in clearName()) with dangling pointers; change their
signatures from void d_free(void* ptr) and inline void p_free(void* ptr) to
accept a reference void*& (e.g., void d_free(void*& ptr) and inline void
p_free(void*& ptr)) and update their implementations to free the memory then set
ptr = nullptr; this will nullify caller lvalues—verify all call sites (30+)
which already pass lvalues still compile and adjust any non-lvalue usages if
present.
In `@wled00/FX_2Dfcn.cpp`:
- Around line 123-125: Replace the non-standard variant check used around the
CDC workaround in FX_2Dfcn.cpp: change the preprocessor condition that currently
tests ARDUINO_ARCH_ESP32S2 to use CONFIG_IDF_TARGET_ESP32S2 so the delay(1)
workaround for the ESP32-S2 runs on the correct builds; update the conditional
surrounding the delay(1) line accordingly to reference CONFIG_IDF_TARGET_ESP32S2
instead of ARDUINO_ARCH_ESP32S2.
In `@wled00/FX.h`:
- Around line 656-657: The clearName() implementation currently calls
p_free(name) but leaves the member pointer dangling, risking a double-free when
setName() later checks and frees name; update clearName() (the
Segment::clearName method) to explicitly set the member variable name to nullptr
after calling p_free(name) so subsequent checks like if (name) in setName(const
String&) are safe and won't attempt to free an already-freed pointer.
♻️ Duplicate comments (1)
wled00/FX_fcn.cpp (1)
1163-1186: Derive parallel-I2S from resolvediType, notdriverType.Lines 1169–1185 count I2S buses from user preference before
getI()resolves the actual driver. IfgetI()downgrades some buses to RMT,useParallelI2Scan be overstated (8× DMA) and valid buses may be dropped.🛠️ Suggested fix
- // validate the bus config: count I2S buses and check if they meet requirements - unsigned i2sBusCount = 0; - for (const auto &bus : busConfigs) { - if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type)) { - digitalCount++; - if (bus.driverType == 1) - i2sBusCount++; - } - } - DEBUG_PRINTF_P(PSTR("Digital buses: %u, I2S buses: %u\n"), digitalCount, i2sBusCount); - - // Determine parallel vs single I2S usage (used for memory calculation only) - bool useParallelI2S = false; - `#if` defined(CONFIG_IDF_TARGET_ESP32S3) - if (i2sBusCount > 0) useParallelI2S = true; - `#else` - if (i2sBusCount > 1) useParallelI2S = true; - `#endif` + unsigned i2sBusCount = 0; + // (after bus.iType is resolved) + for (const auto &bus : busConfigs) { + if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type) && ((bus.iType & 0x01) == 0)) { + i2sBusCount++; + } + } + DEBUG_PRINTF_P(PSTR("Digital buses: %u, I2S buses: %u\n"), digitalCount, i2sBusCount); + + bool useParallelI2S = false; + `#if` defined(CONFIG_IDF_TARGET_ESP32S3) + useParallelI2S = (i2sBusCount > 0); + `#else` + useParallelI2S = (i2sBusCount > 1); + `#endif`Based on learnings, I2S allocation depends on the resolved bus assignment, so
iTypeshould drive memory decisions.
🧹 Nitpick comments (1)
wled00/data/settings_leds.htm (1)
9-10: Regenerate embedded UI headers after this change.Please run
npm run buildso the firmware embeds the updated settings page.As per coding guidelines, please regenerate embedded headers after web UI changes.
Collaborator
Author
|
@willmmiles @softhack007 I am aware that this is a lot to digest and I do not expect you to go over every line of code but if something jumps out, please comment. The rabbit seems pretty happy with the changes |
DedeHai
added
waiting for feedback
This was referenced Jan 19, 2026
Collaborator
Author
|
well that is strange, I was able to merge main into this branch with no conflicts (which there should have been after merging #4939) and this PR is showing that. Something is broken. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/bus_manager.cpp (1)
1117-1131: Fix memory budgeting and driver selection inBusConfig::memUsage().
memis computed but skipped in the early returns, so global/segment buffers aren’t counted for virtual/digital/on‑off buses. Also,PolyBus::getI(..., nr)passes a bus index (or an out‑of‑scope symbol) where a driver preference is expected, which can mis‑classify I2S vs RMT and distort memory limits.Based on learnings, align I2S selection with the actual per‑bus placement (single‑I2S uses the last bus) when estimating memory.🛠️ Proposed fix
size_t BusConfig::memUsage() const { size_t mem = (count + skipAmount) * 8; // 8 bytes per pixel for segment + global buffer if (Bus::isVirtual(type)) { - return sizeof(BusNetwork) + (count * Bus::getNumberOfChannels(type)); + return mem + sizeof(BusNetwork) + (count * Bus::getNumberOfChannels(type)); } else if (Bus::isDigital(type)) { // if any of digital buses uses I2S, there is additional common I2S DMA buffer not accounted for here - return sizeof(BusDigital) + PolyBus::memUsage(count + skipAmount, PolyBus::getI(type, pins, nr)); + return mem + sizeof(BusDigital) + + PolyBus::memUsage(count + skipAmount, PolyBus::getI(type, pins, driverType)); } else if (Bus::isOnOff(type)) { - return sizeof(BusOnOff); + return mem + sizeof(BusOnOff); } else { mem += sizeof(BusPwm); } return mem; }
🤖 Fix all issues with AI agents
In `@wled00/bus_manager.h`:
- Around line 143-144: The BusPlaceholder currently inherits the base
getDriverType() which returns 0 causing serialization to emit drv=0 and
overwrite a user's driver choice; add a stored uint8_t driverType (or similar)
field to the BusPlaceholder class and override virtual uint8_t getDriverType()
const to return that field, then populate that driverType when creating the
placeholder in bus_manager.cpp from the incoming BusConfig (or config->driver)
so the original driver preference is preserved during serialization; optionally
provide a constructor or setter on BusPlaceholder to accept the driver value.
In `@wled00/FX_fcn.cpp`:
- Around line 1193-1204: BusConfig::memUsage() currently calls
PolyBus::getI/type resolution again, double-advancing static channel counters;
change memUsage to use the pre-computed driver type instead of calling getI()
again. Update the BusConfig::memUsage signature (or implementation) to accept or
read the already-set bus.iType (from the earlier BusManager::getI call) and use
that value when computing DMA/RMT channel needs, and then update all callers
(e.g., the loop that does bus.memUsage()) to pass or rely on the stored iType so
PolyBus::getI / BusManager::getI is not invoked twice; ensure declarations
(class header) and any tests/uses are adjusted accordingly.
♻️ Duplicate comments (2)
wled00/FX_fcn.cpp (1)
1165-1187: Resolve I2S bus count aftergetI()to avoid over‑estimating DMA memory.
i2sBusCount(and thususeParallelI2S) is derived from the preference (driverType) beforeBusManager::getI()resolves actual availability. If some buses are forced off I2S (or getI_NONE), the memory estimate can be inflated (×8) and valid configs can be rejected. Count actual I2S buses aftergetI()and ignoreI_NONE.🐛 Suggested adjustment (compute I2S count after resolution)
- // validate the bus config: count I2S buses and check if they meet requirements - unsigned i2sBusCount = 0; - - for (const auto &bus : busConfigs) { - if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type)) { - digitalCount++; - if (bus.driverType == 1) - i2sBusCount++; - } - } - DEBUG_PRINTF_P(PSTR("Digital buses: %u, I2S buses: %u\n"), digitalCount, i2sBusCount); + // validate the bus config: count I2S buses and check if they meet requirements + unsigned i2sBusCount = 0; + for (const auto &bus : busConfigs) { + if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type)) digitalCount++; + } - // Determine parallel vs single I2S usage (used for memory calculation only) - bool useParallelI2S = false; + // Resolve driver types first (getI must be called once per bus) + for (auto &bus : busConfigs) { + bus.iType = BusManager::getI(bus.type, bus.pins, bus.driverType); + `#if` !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(ESP8266) + if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type) && bus.iType != I_NONE && ((bus.iType & 0x01) == 0)) { + i2sBusCount++; + } + `#endif` + } + DEBUG_PRINTF_P(PSTR("Digital buses: %u, I2S buses: %u\n"), digitalCount, i2sBusCount); + // Determine parallel vs single I2S usage (used for memory calculation only) + bool useParallelI2S = false;wled00/bus_manager.cpp (1)
130-145: Release pins when_iTypeis invalid (still leaking on early return).Pins are allocated before the
_iType == I_NONEearly return, so the bus can leak reserved pins; additionally_pins[1]remains uninitialized for 1‑pin buses and is later deallocated incleanup().🛠️ Proposed fix
_pins[0] = bc.pins[0]; + _pins[1] = 255U; // default for 1‑pin buses if (is2Pin(bc.type)) { if (!PinManager::allocatePin(bc.pins[1], true, PinOwner::BusDigital)) { cleanup(); DEBUGBUS_PRINTLN(F("Pin 1 allocated!")); return; } _pins[1] = bc.pins[1]; _frequencykHz = bc.frequency ? bc.frequency : 2000U; // 2MHz clock if undefined } _iType = bc.iType; // reuse the iType that was determined by polyBus in getI() in finalizeInit() - if (_iType == I_NONE) { DEBUGBUS_PRINTLN(F("Incorrect iType!")); return; } + if (_iType == I_NONE) { + DEBUGBUS_PRINTLN(F("Incorrect iType!")); + cleanup(); + return; + }
🧹 Nitpick comments (1)
wled00/cfg.cpp (1)
235-238: Clampdrvto known values (0/1) for robustness.Malformed configs could pass unexpected values and inadvertently force I2S. A simple clamp keeps behavior predictable.
♻️ Suggested tweak
- uint8_t driverType = elm[F("drv")] | 0; // 0=RMT (default), 1=I2S note: polybus may override this if driver is not available + uint8_t driverType = elm[F("drv")] | 0; // 0=RMT (default), 1=I2S note: polybus may override this if driver is not available + driverType = driverType ? 1 : 0;
Contributor
|
Just a note: Memory calculations are still (and fundamentally) wrong (both: |
Collaborator
Author
|
thanks for taking a look. I would not say fundamentally wrong, for standard digital strips its quite accurate and good enough for what it is used for: get a rough estimate if a bus can even be created, there never is any guarantee it will not fail anyway, even if it were accurate down to the last byte. |
Contributor
|
FYI I managed to move all memory calculations into NPB and am now waiting for Makuna to respond if he will consider that as a feature of NPB. The calculations are correct almost to the byte (common DMA buffers cause some issues as they are allocated once buses are started not when they are created or in advance). However, I do not find giving the user a choice of which driver to use beneficial. Most users don't even know what RMT or I2S is let alone what selecting each means. Instead I'd suggest to simplify selection if there is only one type of LEDs and more than 1 output: prefer I2S. In all other cases you'd want RMT. This simplifies everything. If user only has 1 bus you can still use the same selection to switch between RMT and mono I2S. Reasoning? Switch to I2S as the first bus was done by me to combat flickering experienced by many users (including me) when using RMT. Since that was solved by @willmmiles there is no "real" reason to keep I2S as a prefered driver type as it uses more than 4x RAM. It does free up CPU a little, but that is only important with very large set-ups (where RAM is also at premium). |
Collaborator
Author
that would be great!
probably better to hide that by default as a "advanced setting" - it is still useful for poeple who understand it and want to tweak their setups.
the whole point of this PR is to not do that but use RMT by default and still give users the choice of I2S if they want to. |
Contributor
That's what I was saying. But give users the option to use I2S instead (if all outputs use the same type) if they choose so. |
Collaborator
Author
|
then we are on the same page. this PR does exactly that, RMT by default, I2S as an option. |
Contributor
I'm saying it is complicating too much. |
Collaborator
Author
|
only if the option is not hidden by default, which I have planned to add. Otherwise it simplifies things as there is no strict distinction between I2S and RMT and any combination/order of buses is allowed as long as the hardware permits that. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/data/settings_leds.htm`:
- Around line 759-761: When loading older configs, ensure LD is defaulted to 0
and driver reads are normalized: set d.getElementsByName("LD"+i)[0].value to
(v.drv || 0) when populating LT/LD/LS so LD never becomes undefined, and replace
direct parseInt(d.Sf["LD"+n].value || 0) usages with the normalized accessor
getDrv(n) in the functions getMem(), isBCok(), getDuse(), and
updateTypeDropdowns() so driver parsing never yields NaN and validation uses a
consistent driver value.
🧹 Nitpick comments (2)
wled00/data/settings_leds.htm (2)
229-255: Consider caching getDuse() to avoid O(n²) UI work.
getMem()now callsgetDuse()per bus, whileUI()also callsgetDuse()later. Passing a precomputedusageintogetMem()would keep this O(n).♻️ Possible refactor
-function getMem(t, n) { +function getMem(t, n, usage) { ... - let usage = getDuse(); dma = usage.I2Smem; ... } ... - LT.forEach((s,i)=>{ memu += getMem(t,n); }) + const usage = getDuse(); + LT.forEach((s,i)=>{ memu += getMem(t,n, usage); })
944-1017: InitializefirstI2STypefrom usage to avoid order dependence.
If the first I2S bus appears later in the loop, earlier dropdowns won’t see type constraints on this pass. Usingusage.I2STypeavoids that.♻️ Minimal tweak
- let firstI2SType = null; + let firstI2SType = usage.I2SType;
Comment on lines
759
to
761
| d.getElementsByName("LT"+i)[0].value = v.type; | ||
| d.getElementsByName("LD"+i)[0].value = v.drv; // output driver type (RMT or I2S) | ||
| d.getElementsByName("LS"+i)[0].value = v.start; |
Contributor
There was a problem hiding this comment.
Default driver when loading older configs to avoid NaN driver states.
Older configs won’t have v.drv, which sets LD to undefined. That causes parseInt(value || 0) to return NaN (truthy string), leading to I2S/RMT miscounts and invalid-bus states during validation. Default to 0 on load and normalize driver reads.
🛠️ Suggested fix
+function getDrv(n) { return parseInt(d.Sf["LD"+n]?.value, 10) || 0; }
...
- d.getElementsByName("LD"+i)[0].value = v.drv; // output driver type (RMT or I2S)
+ d.getElementsByName("LD"+i)[0].value = (v.drv ?? 0); // default to RMT for legacy configsThen replace parseInt(d.Sf["LD"+n].value || 0) with getDrv(n) in getMem(), isBCok(), getDuse(), and updateTypeDropdowns().
🤖 Prompt for AI Agents
In `@wled00/data/settings_leds.htm` around lines 759 - 761, When loading older
configs, ensure LD is defaulted to 0 and driver reads are normalized: set
d.getElementsByName("LD"+i)[0].value to (v.drv || 0) when populating LT/LD/LS so
LD never becomes undefined, and replace direct parseInt(d.Sf["LD"+n].value || 0)
usages with the normalized accessor getDrv(n) in the functions getMem(),
isBCok(), getDuse(), and updateTypeDropdowns() so driver parsing never yields
NaN and validation uses a consistent driver value.
Collaborator
Author
There was a problem hiding this comment.
so what you are saying is it should be parseInt(value | 0) ?
Contributor
There was a problem hiding this comment.
@DedeHai Not quite—there are two separate contexts here:
-
When loading configs (assignment): Use
(v.drv ?? 0)or(v.drv || 0)to default missing values:d.getElementsByName("LD"+i)[0].value = (v.drv ?? 0);
This ensures old configs without
drvget a sensible default of0(RMT). -
When reading the value (parsing): The pattern
parseInt(d.Sf["LD"+n].value, 10) || 0is clearer thanparseInt(value | 0)because:- It explicitly specifies radix
10 - The
|| 0fallback is more readable - While
value | 0technically coerces to number and handles undefined/empty cases, it's less explicit
- It explicitly specifies radix
If you want a terser solution for reading, you could skip parseInt entirely and just use:
(d.Sf["LD"+n]?.value | 0) // bitwise OR coerces to number and defaults to 0or the slightly more explicit:
parseInt(d.Sf["LD"+n]?.value, 10) || 0Summary: For loading, use (v.drv ?? 0). For reading, either (value | 0) or parseInt(value, 10) || 0 work—pick whichever is clearer to you. The key is ensuring undefined/empty never produces NaN.
🧠 Learnings used
Learnt from: mval-sg
Repo: wled/WLED PR: 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.
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: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T16:42:26.035Z
Learning: In WLED PR `#5267`, the bus allocation strategy for single I2S mode was changed: ESP32 now uses single I2S on bus 8 (the last bus, digitalCount=9) instead of bus 0; ESP32-S2 uses single I2S on bus 4 (digitalCount=5). Memory calculation logic must account for which specific bus uses I2S in single mode, not just check digitalCount <= 8.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.841Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Learnt from: DedeHai
Repo: wled/WLED PR: 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: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
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.
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
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: willmmiles
Repo: wled/WLED PR: 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.
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
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.
Collaborator
Author
There was a problem hiding this comment.
I prefer the solution that uses the least code as code size is important.
Contributor
There was a problem hiding this comment.
@DedeHai For minimal code size, use the bitwise OR operator (| 0):
For loading configs (line ~760):
d.getElementsByName("LD"+i)[0].value = v.drv | 0;For reading driver values (throughout the file), replace:
parseInt(d.Sf["LD"+n].value || 0)→d.Sf["LD"+n].value | 0d.Sf["LD"+n] ? parseInt(d.Sf["LD"+n].value || 0) : 0→d.Sf["LD"+n]?.value | 0
This is the most compact solution. The | 0 pattern:
- Coerces to number
- Returns
0for undefined/null/empty/""/NaN - Is shorter than
parseInt+ fallback
Examples where this applies in the file:
getMem()line ~194:let driverPref = d.Sf["LD"+n]?.value | 0;isBCok()line ~222:let drv = d.Sf["LD"+n]?.value | 0;getDuse()line ~565:let driverPref = d.Sf["LD"+n]?.value | 0;UI()line ~337:if (!d.Sf["LD"+n].value) d.Sf["LD"+n].value = "0";can becomed.Sf["LD"+n].value = d.Sf["LD"+n].value | 0;
🧠 Learnings used
Learnt from: mval-sg
Repo: wled/WLED PR: 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.
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: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
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: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: DedeHai
Repo: wled/WLED PR: 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: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 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.
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
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.
Learnt from: willmmiles
Repo: wled/WLED PR: 4890
File: lib/NeoESP32RmtHI/include/NeoEsp32RmtHIMethod.h:173-180
Timestamp: 2025-09-02T01:56:43.841Z
Learning: willmmiles prefers to maintain consistency with upstream NeoPixelBus patterns (like unchecked malloc in construct() methods) rather than diverging until improvements are made upstream first, to minimize maintenance burden and keep the codebase aligned.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T16:42:26.035Z
Learning: In WLED PR `#5267`, the bus allocation strategy for single I2S mode was changed: ESP32 now uses single I2S on bus 8 (the last bus, digitalCount=9) instead of bus 0; ESP32-S2 uses single I2S on bus 4 (digitalCount=5). Memory calculation logic must account for which specific bus uses I2S in single mode, not just check digitalCount <= 8.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
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.
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
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: willmmiles
Repo: wled/WLED PR: 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Buses are now enumerated automatically, if no driver preference is specified it defaults to RMT
The main improvement is that this allows mixed RMT output types in combination with parallel I2S
The driver behind this PR is to solve the "flickering issues" and give users the option to change the output driver (RMT/I2S) if they encounter flickering after an update as the two do not (and probably never can) use identical pulse-timing.
Fixes #4315
Fixes #5266
Breaking changes
Known issues
Additional changes
Tests performed
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.