Improved bus handling: free choice of bus driver in any order and improved memory calculations #5303
Conversation
- 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors bus APIs to add per-bus driver selection and iType resolution via BusManager::getI; BusDigital construction now takes only BusConfig; centralizes channel/I2S tracking and memory budgeting (separating I2S DMA); exposes driverType in serialization and adds per-bus driver UI/validation. 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/**.
|
@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
waiting for feedback
|
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. |
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;
|
Just a note: Memory calculations are still (and fundamentally) wrong (both: |
|
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. |
|
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). |
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. |
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. |
|
then we are on the same page. this PR does exactly that, RMT by default, I2S as an option. |
I'm saying it is complicating too much. |
|
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. |
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;
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 259-267: isBCok currently fails every bus when any global resource
is exceeded; narrow the global checks to the bus's own driver: inside isBCok use
the bus driver variable drv (parsed from LD+n) and only apply the RMT limit
check when drv==0 (or drv===0) and usage.rmtUsed>maxRMT, and only apply the I2S
limit check when drv==1 and usage.i2sUsed>maxI2S; keep the existing I2S type
mismatch check (drv==1 && usage.I2SType!==null && t!==usage.I2SType) and leave
trySubmit behavior unchanged—this will stop UI() per-bus highlighting from
turning unrelated buses red.
🧹 Nitpick comments (4)
wled00/data/settings_leds.htm (4)
56-67: Parameterdshadows globald(document alias), andmaxVcomment is stale.
The parameter
dinbLimits(v,p,m,l,o,d,r,i,a,n)shadows the globald = documentalias defined incommon.js. Currently safe because no DOM access occurs inside, but any future maintenance adding DOM calls here would silently break.The comment on line 57 still reads "min virtual buses: 6 - ESP32/S3, 4 - S2/C3, 3 - ESP8266" while the actual values are now 0–4 (platform enum). This is misleading for anyone reading the code.
Suggested fix
-function bLimits(v,p,m,l,o,d,r,i,a,n) { - maxV = v; // maxV - min virtual buses: 6 - ESP32/S3, 4 - S2/C3, 3 - ESP8266 (only used to distinguish S2/S3) +function bLimits(v,p,m,l,o,dc,r,i,a,n) { + maxV = v; // maxV - platform ID: 0 - ESP8266, 1 - C3, 2 - S2, 3 - S3, 4 - ESP32 maxPB = p; // maxPB - max LEDs per bus maxM = m; // maxM - max LED memory maxL = l; // maxL - max LEDs (will serve to determine ESP >1664 == ESP32) maxCO = o; // maxCO - max Color Order mappings - maxD = d; // maxD - max digital channels (can be changed if using ESP32 parallel I2S): 16 - ESP32, 12 - S3/S2, 2 - C3, 3 - 8266 + maxD = dc; // maxD - max digital channels (can be changed if using ESP32 parallel I2S): 16 - ESP32, 12 - S3/S2, 2 - C3, 3 - 8266
244-251:getMem()callsgetDuse()for every I2S bus — compute once and pass it in.
getDuse()iterates all buses. SincegetMem()is called inside theUI()loop for each bus, every I2S bus triggers a redundant full iteration.UI()already computesusage = getDuse()at line 444. Pass the usage object intogetMem()instead.Suggested refactor sketch
-function getMem(t, n) { +function getMem(t, n, usage) { if (isAna(t)) return 5; ... if (driverPref == 1) { // I2S to be used mul = 1; - let usage = getDuse(); dma = usage.I2Smem; }Then in
UI(), computeusagebefore the accumulation loop and pass it:+let usage = getDuse(); LTs.forEach((s,i)=>{ var n = s.name.substring(2,3); var t = parseInt(s.value); - memu += getMem(t, n); + memu += getMem(t, n, usage);
127-132: Consider a more descriptive validation error.The alert at line 132 says "Invalid Bus-config" without indicating which bus or why (RMT overflow? I2S type mismatch? I2S overflow?). Users with many buses may find this difficult to act on — especially since the red highlighting on driver dropdowns requires the Advanced Settings checkbox to be visible.
958-1017: Remove unnecessarycurDriverre-declaration at line 1000.At line 964,
curDriveris already declared asconstin the forEach callback scope. At line 1000, it's unnecessarily redeclared asletwithin an inner if block, shadowing the outer binding. This re-reads the same value and should be removed—just reuse the existingconst curDriverfrom line 964.Suggested fix
if (drvsel && isDig(curType) && !isD2P(curType)) { let rmtOpt = drvsel.querySelector('option[value="0"]'); let i2sOpt = drvsel.querySelector('option[value="1"]'); rmtOpt.disabled = false; i2sOpt.disabled = false; - let curDriver = parseInt(d.Sf["LD"+n].value || 0); - if (curDriver === 0) { + if (curDriver === 0) { // reuse const curDriver from line 964
|
this also fixes a bug introduced in #4939 |
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 56-72: The comment for maxV in function bLimits is outdated and
misleading; update or replace it so it documents the actual enum-like mapping
used by the platform-detection helpers (is8266, isC3, isS2, isS3, is32) where
maxV values 0..4 correspond to ESP8266, C3, S2, S3, ESP32 respectively (or
remove the old numeric examples and describe the mapping in words), ensuring the
comment next to maxV and/or above the isXXXX() functions reflects the current
semantics.
🧹 Nitpick comments (2)
wled00/data/settings_leds.htm (2)
244-256:getDuse()is recomputed insidegetMem()for every bus — O(n²).
getMem()is called per-bus in theUI()loop (line 324). Each call invokesgetDuse()(line 249), which itself iterates all buses. SincegetDuse()is already computed inUI()at line 444 and the result doesn't change within a single pass, consider passing theusageobject as a parameter to avoid the quadratic overhead.Suggested refactor sketch
- function getMem(t, n) { + function getMem(t, n, usage) { ... - let usage = getDuse(); - dma = usage.I2Smem; + dma = usage ? usage.I2Smem : 0;And at the call site (line 324):
- memu += getMem(t, n); + memu += getMem(t, n, usage);(Compute
usageonce before the loop.)
958-1018: Auto-switch of driver values during iteration uses staleusage— converges only on second pass.Lines 1006 and 1014 mutate
d.Sf["LD"+n].valueduring iteration, but theusageobject (line 946) isn't refreshed. The disable-option checks at lines 1002 and 1010 use the staleusage.i2sUsed/usage.rmtUsed, so they may fail to disable a driver option for one UI pass.This converges because
updateTypeDropdowns()is called twice inUI()(lines 318 and 468) withgetDuse()recomputed between them (line 444). The design works but is fragile — if someone removes the second call or reorders the logic, driver limits could be silently bypassed.Consider adding a brief comment near line 468 explaining why the second call is necessary for convergence.
| function bLimits(v,p,m,l,o,d,r,i,a,n) { | ||
| maxV = v; // maxV - min virtual buses: 6 - ESP32/S3, 4 - S2/C3, 3 - ESP8266 (only used to distinguish S2/S3) | ||
| maxPB = p; // maxPB - max LEDs per bus | ||
| maxM = m; // maxM - max LED memory | ||
| maxL = l; // maxL - max LEDs (will serve to determine ESP >1664 == ESP32) | ||
| maxCO = o; // maxCO - max Color Order mappings | ||
| maxD = d; // maxD - max digital channels (can be changed if using ESP32 parallel I2S): 16 - ESP32, 12 - S3/S2, 2 - C3, 3 - 8266 | ||
| maxRMT = r; // maxRMT - max RMT channels: 8 - ESP32, 4 - S2/S3, 2 - C3, 0 - 8266 | ||
| maxI2S = i; // maxI2S - max I2S/LCD channels: 8 - ESP32/S2/S3, 0 - C3/8266 | ||
| maxA = a; // maxA - max analog channels: 16 - ESP32, 8 - S3/S2, 6 - C3, 5 - 8266 | ||
| maxBT = n; // maxBT - max buttons | ||
| } | ||
| function is8266() { return maxA == 5 && maxD == 3; } // NOTE: see const.h | ||
| function is32() { return maxA == 16 && maxD == 16; } // NOTE: see const.h | ||
| function isC3() { return maxA == 6 && maxD == 2; } // NOTE: see const.h | ||
| function isS2() { return maxA == 8 && maxD == 12 && maxV == 4; } // NOTE: see const.h | ||
| function isS3() { return maxA == 8 && maxD == 12 && maxV == 6; } // NOTE: see const.h | ||
| function is8266() { return maxV == 0; } // NOTE: see const.h WLED_MIN_VIRTUAL_BUSSES | ||
| function isC3() { return maxV == 1; } | ||
| function isS2() { return maxV == 2; } | ||
| function isS3() { return maxV == 3; } | ||
| function is32() { return maxV == 4; } |
There was a problem hiding this comment.
Misleading comment on maxV values.
Line 57 states maxV - min virtual buses: 6 - ESP32/S3, 4 - S2/C3, 3 - ESP8266 but the actual platform-detection functions (lines 68–72) check for values 0–4. The comment describes old semantics and will confuse anyone reading this code.
Suggested fix
- maxV = v; // maxV - min virtual buses: 6 - ESP32/S3, 4 - S2/C3, 3 - ESP8266 (only used to distinguish S2/S3)
+ maxV = v; // maxV - platform ID: 0 - ESP8266, 1 - C3, 2 - S2, 3 - S3, 4 - ESP32📝 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.
| function bLimits(v,p,m,l,o,d,r,i,a,n) { | |
| maxV = v; // maxV - min virtual buses: 6 - ESP32/S3, 4 - S2/C3, 3 - ESP8266 (only used to distinguish S2/S3) | |
| maxPB = p; // maxPB - max LEDs per bus | |
| maxM = m; // maxM - max LED memory | |
| maxL = l; // maxL - max LEDs (will serve to determine ESP >1664 == ESP32) | |
| maxCO = o; // maxCO - max Color Order mappings | |
| maxD = d; // maxD - max digital channels (can be changed if using ESP32 parallel I2S): 16 - ESP32, 12 - S3/S2, 2 - C3, 3 - 8266 | |
| maxRMT = r; // maxRMT - max RMT channels: 8 - ESP32, 4 - S2/S3, 2 - C3, 0 - 8266 | |
| maxI2S = i; // maxI2S - max I2S/LCD channels: 8 - ESP32/S2/S3, 0 - C3/8266 | |
| maxA = a; // maxA - max analog channels: 16 - ESP32, 8 - S3/S2, 6 - C3, 5 - 8266 | |
| maxBT = n; // maxBT - max buttons | |
| } | |
| function is8266() { return maxA == 5 && maxD == 3; } // NOTE: see const.h | |
| function is32() { return maxA == 16 && maxD == 16; } // NOTE: see const.h | |
| function isC3() { return maxA == 6 && maxD == 2; } // NOTE: see const.h | |
| function isS2() { return maxA == 8 && maxD == 12 && maxV == 4; } // NOTE: see const.h | |
| function isS3() { return maxA == 8 && maxD == 12 && maxV == 6; } // NOTE: see const.h | |
| function is8266() { return maxV == 0; } // NOTE: see const.h WLED_MIN_VIRTUAL_BUSSES | |
| function isC3() { return maxV == 1; } | |
| function isS2() { return maxV == 2; } | |
| function isS3() { return maxV == 3; } | |
| function is32() { return maxV == 4; } | |
| function bLimits(v,p,m,l,o,d,r,i,a,n) { | |
| maxV = v; // maxV - platform ID: 0 - ESP8266, 1 - C3, 2 - S2, 3 - S3, 4 - ESP32 | |
| maxPB = p; // maxPB - max LEDs per bus | |
| maxM = m; // maxM - max LED memory | |
| maxL = l; // maxL - max LEDs (will serve to determine ESP >1664 == ESP32) | |
| maxCO = o; // maxCO - max Color Order mappings | |
| maxD = d; // maxD - max digital channels (can be changed if using ESP32 parallel I2S): 16 - ESP32, 12 - S3/S2, 2 - C3, 3 - 8266 | |
| maxRMT = r; // maxRMT - max RMT channels: 8 - ESP32, 4 - S2/S3, 2 - C3, 0 - 8266 | |
| maxI2S = i; // maxI2S - max I2S/LCD channels: 8 - ESP32/S2/S3, 0 - C3/8266 | |
| maxA = a; // maxA - max analog channels: 16 - ESP32, 8 - S3/S2, 6 - C3, 5 - 8266 | |
| maxBT = n; // maxBT - max buttons | |
| } | |
| function is8266() { return maxV == 0; } // NOTE: see const.h WLED_MIN_VIRTUAL_BUSSES | |
| function isC3() { return maxV == 1; } | |
| function isS2() { return maxV == 2; } | |
| function isS3() { return maxV == 3; } | |
| function is32() { return maxV == 4; } |
🤖 Prompt for AI Agents
In `@wled00/data/settings_leds.htm` around lines 56 - 72, The comment for maxV in
function bLimits is outdated and misleading; update or replace it so it
documents the actual enum-like mapping used by the platform-detection helpers
(is8266, isC3, isS2, isS3, is32) where maxV values 0..4 correspond to ESP8266,
C3, S2, S3, ESP32 respectively (or remove the old numeric examples and describe
the mapping in words), ensuring the comment next to maxV and/or above the
isXXXX() functions reflects the current semantics.
There was a problem hiding this comment.
it's still a hack. since it is now only used for platform ID, it would be better to just rename it throughout the code. can you give me the changes needed to do that?
There was a problem hiding this comment.
🧩 Analysis chain
@DedeHai, great idea! Let me search for all occurrences of maxV in the codebase to provide you with a complete list of changes needed for the rename.
🏁 Script executed:
#!/bin/bash
# Find all occurrences of maxV (case-sensitive, whole word matches)
echo "=== Files containing maxV ==="
rg -n '\bmaxV\b' --type=html --type=js --type=cpp -C2
echo ""
echo "=== Summary of files ==="
rg -l '\bmaxV\b' --type=html --type=js --type=cpp
Length of output: 2868
Let me also check how bLimits is called from the backend to ensure we understand the full picture:
🏁 Script executed:
#!/bin/bash
# Find where bLimits is called (likely in xml.cpp or similar)
echo "=== Files calling bLimits ==="
rg -n 'bLimits' --type=cpp -C3
Length of output: 488
@DedeHai, perfect! I found all occurrences. Here are the complete changes needed to rename maxV to platformID:
Changes Required
1. wled00/data/settings_leds.htm - Line 9 (global declaration)
- var maxD=1,maxI2S=0,maxRMT=0,maxA=1,maxV=0,maxM=4000,maxPB=2048,maxL=1664,maxCO=5,maxBT=4;
+ var maxD=1,maxI2S=0,maxRMT=0,maxA=1,platformID=0,maxM=4000,maxPB=2048,maxL=1664,maxCO=5,maxBT=4;2. wled00/data/settings_leds.htm - Lines 56-67 (bLimits function)
function bLimits(v,p,m,l,o,d,r,i,a,n) {
- maxV = v; // maxV - min virtual buses: 6 - ESP32/S3, 4 - S2/C3, 3 - ESP8266 (only used to distinguish S2/S3)
+ platformID = v; // platformID - ESP platform: 0 - ESP8266, 1 - C3, 2 - S2, 3 - S3, 4 - ESP32
maxPB = p; // maxPB - max LEDs per bus
maxM = m; // maxM - max LED memory
maxL = l; // maxL - max LEDs (will serve to determine ESP >1664 == ESP32)
maxCO = o; // maxCO - max Color Order mappings
maxD = d; // maxD - max digital channels (can be changed if using ESP32 parallel I2S): 16 - ESP32, 12 - S3/S2, 2 - C3, 3 - 8266
maxRMT = r; // maxRMT - max RMT channels: 8 - ESP32, 4 - S2/S3, 2 - C3, 0 - 8266
maxI2S = i; // maxI2S - max I2S/LCD channels: 8 - ESP32/S2/S3, 0 - C3/8266
maxA = a; // maxA - max analog channels: 16 - ESP32, 8 - S3/S2, 6 - C3, 5 - 8266
maxBT = n; // maxBT - max buttons
}
- function is8266() { return maxV == 0; } // NOTE: see const.h WLED_MIN_VIRTUAL_BUSSES
- function isC3() { return maxV == 1; }
- function isS2() { return maxV == 2; }
- function isS3() { return maxV == 3; }
- function is32() { return maxV == 4; }
+ function is8266() { return platformID == 0; } // NOTE: see const.h WLED_MIN_VIRTUAL_BUSSES
+ function isC3() { return platformID == 1; }
+ function isS2() { return platformID == 2; }
+ function isS3() { return platformID == 3; }
+ function is32() { return platformID == 4; }3. wled00/xml.cpp - Lines 294-295 (update comment for clarity)
// set limits
settingsScript.printf_P(PSTR("bLimits(%d,%d,%d,%d,%d,%d,%d,%d,%d,%d);"),
- WLED_MIN_VIRTUAL_BUSSES, // irrelevant, but kept to distinguish ESP types in UI
+ WLED_MIN_VIRTUAL_BUSSES, // used as platformID to distinguish ESP chip types in UI
MAX_LEDS_PER_BUS,
MAX_LED_MEMORY,That's it! The variable is only used in settings_leds.htm for the platform detection functions, and the backend just passes WLED_MIN_VIRTUAL_BUSSES (which conveniently has different values per platform) as the first parameter to bLimits(). The rename makes the code much clearer about its actual purpose.
🧠 Learnings used
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: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5223-5226
Timestamp: 2025-10-20T09:38:51.997Z
Learning: WLED matrices: each dimension (SEG_W, SEG_H) is limited to ≤255; 256 or larger per side is not supported/feasible on ESP32, so effects should assume per-dimension max 255.
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: 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: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: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Learnt from: 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: blazoncek
Repo: wled/WLED PR: 4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.200Z
Learning: In WLED, maximum segment name length varies by platform:
- ESP8266: 32 characters (WLED_MAX_SEGNAME_LEN = 32)
- ESP32: 64 characters (WLED_MAX_SEGNAME_LEN = 64)
This platform difference can cause truncation when syncing longer names from ESP32 to ESP8266. Additionally, the WLED UI has limitations regarding modified maximum segment name lengths.
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T16:42:26.035Z
Learning: In WLED, both parallel I2S and single I2S modes require additional DMA buffers (4× the pixel buffer size). The memory calculation must account for which specific buses use I2S hardware, not just check digitalCount <= 8, because single I2S mode places the I2S bus at the END (ESP32 bus 8 with digitalCount=9, S2 bus 4 with digitalCount=5).
Learnt from: DedeHai
Repo: wled/WLED PR: 4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.355Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
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: blazoncek
Repo: wled/WLED PR: 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.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: wled00/wled.cpp:385-387
Timestamp: 2026-02-08T19:32:56.086Z
Learning: In WLED ESP32 code, when checking whether USB CDC serial initialization delay is needed, rely on the ARDUINO_USB_CDC_ON_BOOT flag rather than explicit CONFIG_IDF_TARGET checks, because not all development boards for chips with native USB capability (ESP32-C3/C5/C6/S2/S3/H2/P4) actually use the native USB—many have external UART bridge chips instead. The ARDUINO_USB_CDC_ON_BOOT flag correctly indicates the board's actual USB configuration.
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: 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: 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 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: 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.
| function isC3() { return maxA == 6 && maxD == 2; } // NOTE: see const.h | ||
| function isS2() { return maxA == 8 && maxD == 12 && maxV == 4; } // NOTE: see const.h | ||
| function isS3() { return maxA == 8 && maxD == 12 && maxV == 6; } // NOTE: see const.h | ||
| function is8266() { return maxV == 0; } // NOTE: see const.h WLED_MIN_VIRTUAL_BUSSES |
There was a problem hiding this comment.
Maybe the right question is "why do we need to know the chip type"?
The list of isXX could grow soon, because our V5 branch allows 4 new MCU's to be added: esp32-C5, esp32-C6, esp32-C61, esp32-P4. Except for the P4, they are all having two RMT outputs like C3, and none of them support parallel I2S. But for some of them, a parlio parallel driver might come in the future.
I did not read the code in detail yet - indirect conclusions from the chip type like "supports parallel I2S" or "only has 2 RMT busses", "supports hub75" could be directly communicated to the UI, instead if implementing the decision logic in the UI.
There was a problem hiding this comment.
that is absolutely true, it is a major refactor though and should not be part of this PR. From the top of my head this is what the chip type is used for in led settings:
- determine capabilities of driver outputs
- calculate memory based on bus type (different for S3 / 8266 and wheter single I2S is used or parallel)
- determining to show some features that are or are not available on current type
moving to a feature based structure rather than chip based affects the whole codebase and will introduce new bugs, I would do that in 0.17. One step at a time :)
There was a problem hiding this comment.
Yes, I agree with the refactoring aspect - that's one of the reasons why the V5 branch is for 17.x, but not for 16.x.
Side-topic: its already possible to move small parts towards "feature based" instead of "chip model based" - it does not need to be a "big bang refactoring".
Like replacing long #if defined() ... chains with shorter flags: WLED_HAVE_PARALLEL_I2S, __riscv, __XTENSA, SOC_CPU_CORES_NUM <2, SOC_I2S_NUM > 1, etc.
There was a problem hiding this comment.
areed, the IDF provides some feature based defines, I just used #if SOC_DEDICATED_GPIO_SUPPORTED the other day instead of using the chip definitions for an upcoming PR that does "soft on" solid state relay switching without using hardware PWM
edit: it may bite us though to use those, in case they got scrapped in V5...
There was a problem hiding this comment.
Side-topic: its already possible to move small parts towards "feature based" instead of "chip model based" - it does not need to be a "big bang refactoring".
I've started with de-cluttering in V5-C6: https://github.com/wled/WLED/blob/V5-C6/wled00/wled_boards.h . Comments welcome :-)
There was a problem hiding this comment.
once you have a starting point and maybe a structure defined as suggested by @willmmiles:
copied from discord: "In fact, I think we should go one step further and set up a proper HAL tree with device-specific folders which can include cpp files later.
For ESP32 I do recommend following the same structure as the IDF, eg. hal/esp32/c5/platform.h, so that file can also #include "hal/esp32/platform.h" for common fragments
and then ultimately hal/platform.h has the big #elif defined(ARDUINO_ARCH_XXX) block for selecting the target"
it could be worth a shot to task an AI agent to try and find all current chip defines and compile a list of suggestions for that HAL. If you did not try it already: install google antigravity, its a vscode fork that allows you to task an agent in "planning mode" and it can do quite complex stuff, although for complicated tasks you will run out of free tokens and have to wait a few days. It does unfortunately not support platformio so can't compile in that IDE. It is very similar to using copilot on git.
There was a problem hiding this comment.
A short summary (not exhaustive) is here #5048 (comment)
I'll continue with my idea in the V5 branch, just to see how well it works and where it gets us. If the "hal" structure does not get too cluttered, agreed it would be the future-safest solution.
softhack007
left a comment
softhack007
left a comment
There was a problem hiding this comment.
this also fixes a bug introduced in #4939
if there are no objections I would like to merge this and get broader feedback. I did test and so far, did not find any bad thigs happening.
ok for me to release this into the wild 👍
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