Improved bus handling: free choice of bus driver in any order and improved memory calculations #5303
+485
−336
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 <6280424+DedeHai@users.noreply.github.com>
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 <6280424+DedeHai@users.noreply.github.com>
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 <6280424+DedeHai@users.noreply.github.com>
…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 <6280424+DedeHai@users.noreply.github.com>
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 <6280424+DedeHai@users.noreply.github.com>
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(), removes the BusDigital index constructor, centralizes channel/I2S tracking and memory budgeting (separating I2S DMA), and surfaces driverType in serialization and the frontend. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Comment on lines
+144
to
+159
| _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; } | ||
| _hasRgb = hasRGB(bc.type); | ||
| _hasWhite = hasWhite(bc.type); | ||
| _hasCCT = hasCCT(bc.type); | ||
| uint16_t lenToCreate = bc.count; | ||
| if (bc.type == TYPE_WS2812_1CH_X3) lenToCreate = NUM_ICS_WS2812_1CH_3X(bc.count); // only needs a third of "RGB" LEDs for NeoPixelBus | ||
| _busPtr = PolyBus::create(_iType, _pins, lenToCreate + _skip, nr); | ||
| _busPtr = PolyBus::create(_iType, _pins, lenToCreate + _skip); | ||
| _valid = (_busPtr != nullptr) && bc.count > 0; | ||
| // fix for wled#4759 | ||
| if (_valid) for (unsigned i = 0; i < _skip; i++) { | ||
| PolyBus::setPixelColor(_busPtr, _iType, i, 0, COL_ORDER_GRB); // set sacrificial pixels to black (CO does not matter here) | ||
| } | ||
| DEBUGBUS_PRINTF_P(PSTR("Bus: %successfully inited #%u (len:%u, type:%u (RGB:%d, W:%d, CCT:%d), pins:%u,%u [itype:%u] mA=%d/%d)\n"), | ||
| _valid?"S":"Uns", | ||
| (int)nr, | ||
| else { | ||
| cleanup(); | ||
| } |
Contributor
There was a problem hiding this comment.
Release allocated pins when iType is invalid.
Line 145 returns early on I_NONE after pin allocation, leaving pins reserved and the bus partially constructed. This is particularly risky for usermods that forget to populate iType.
🛠️ Suggested 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(); // release pins allocated above
+ return;
+ }🤖 Prompt for AI Agents
In `@wled00/bus_manager.cpp` around lines 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.
Comment on lines
+918
to
+927
| let n = sel.name.substring(2,3); | ||
| let t = parseInt(sel.value); | ||
| let driverPref = d.Sf["LD"+n] ? parseInt(d.Sf["LD"+n].value || 0) : 0; | ||
| let ledCount = parseInt(d.Sf["LC"+n].value) || 0; | ||
|
|
||
| if (isDig(t) && !isD2P(t)) { | ||
| if (driverPref === 1) { | ||
| i2sUsed++; | ||
| maxLEDs = maxLEDs > ledCount ? maxLEDs : ledCount; // for parallel I2S the memory buffer is shared, largest bus determines total memory usage | ||
| if (!I2SType) I2SType = t; // first I2S bus determines allowed type for all subsequent I2S buses (parallel I2S limitation) |
Contributor
There was a problem hiding this comment.
Include skipped LEDs in the I2S DMA estimate.
Line 921 only uses LC; skipped LEDs (SL) are still allocated, so DMA usage is undercounted when skip > 0.
🛠️ Suggested fix
- let ledCount = parseInt(d.Sf["LC"+n].value) || 0;
+ let ledCount = (parseInt(d.Sf["LC"+n].value) || 0) + (parseInt(d.Sf["SL"+n].value) || 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let n = sel.name.substring(2,3); | |
| let t = parseInt(sel.value); | |
| let driverPref = d.Sf["LD"+n] ? parseInt(d.Sf["LD"+n].value || 0) : 0; | |
| let ledCount = parseInt(d.Sf["LC"+n].value) || 0; | |
| if (isDig(t) && !isD2P(t)) { | |
| if (driverPref === 1) { | |
| i2sUsed++; | |
| maxLEDs = maxLEDs > ledCount ? maxLEDs : ledCount; // for parallel I2S the memory buffer is shared, largest bus determines total memory usage | |
| if (!I2SType) I2SType = t; // first I2S bus determines allowed type for all subsequent I2S buses (parallel I2S limitation) | |
| let n = sel.name.substring(2,3); | |
| let t = parseInt(sel.value); | |
| let driverPref = d.Sf["LD"+n] ? parseInt(d.Sf["LD"+n].value || 0) : 0; | |
| let ledCount = (parseInt(d.Sf["LC"+n].value) || 0) + (parseInt(d.Sf["SL"+n].value) || 0); | |
| if (isDig(t) && !isD2P(t)) { | |
| if (driverPref === 1) { | |
| i2sUsed++; | |
| maxLEDs = maxLEDs > ledCount ? maxLEDs : ledCount; // for parallel I2S the memory buffer is shared, largest bus determines total memory usage | |
| if (!I2SType) I2SType = t; // first I2S bus determines allowed type for all subsequent I2S buses (parallel I2S limitation) |
🤖 Prompt for AI Agents
In `@wled00/data/settings_leds.htm` around lines 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).
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;
Comment on lines
+143
to
+144
| virtual uint8_t getDriverType() const { return 0; } // Default to RMT (0) for non-digital buses | ||
| virtual size_t getBusSize() const { return sizeof(Bus); } // currently unused |
Contributor
There was a problem hiding this comment.
Preserve driverType for placeholder buses to avoid config drift.
BusPlaceholder doesn’t override getDriverType(), so when a bus falls back to a placeholder (e.g., memory limits), serialization will emit drv=0 and lose the user’s preference. Consider storing the driver type in BusPlaceholder and overriding getDriverType() (and populate it from BusConfig in bus_manager.cpp).
🤖 Prompt for AI Agents
In `@wled00/bus_manager.h` around lines 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.