Conversation
BusUsermod - virtual bus to be used by usermods UsermodBus - base class for usermods that implement a bus based on BusUsermod
…with I2C PSPWM firmware
blazoncek
added
enhancement
keep
|
maintainers call for discussion: i think we should first understand if an "adding a bus by usermod" feature still fits to our future view of the WLED architecture. |
|
Since I'm no longer one of the WLED development team members I'll try to explain why I thought this was a good idea at the time. Bus architecture is IMO good and flexible (though it could be better still) so adding a user extensible bus seemed nice addition to allow (advanced) users to extend output to devices of not necessarily LED type. |
|
As the one who started this PR originally, despite not being involved in anything WLED related for the past year, I'm still convinced that it is useful for advanced users (as @blazoncek wrote). There are many exotic use cases out there, that one could cover using WLED with minor additions. The project I was involved in 2024 required a way to control 'polarity change christmas lights', that typically run on 31V "AC". I got that working by writing a phase shift pwm firmware for AT8870 motor shields, and controlling those from WLED via I2C. The same modified motor shields also work perfectly to control 12V-36V RGBW strips. So it's certainly not for the average user, but I was glad to have that possibility back then. |
|
Any modular approach needs to support multiple types, not just one. |
| ;; | ||
| ;; please note that you can NOT update existing ESP32 installs with a "V4" build. Also updating by OTA will not work properly. | ||
| ;; You need to completely erase your device (esptool erase_flash) first, then install the "V4" build from VSCode+platformio. | ||
| default_partitions = tools/WLED_ESP32_4MB_1MB_FS.csv |
There was a problem hiding this comment.
i think this change is obsolete - the V4 environment has evolved, and the default partition should not be set here.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
WalkthroughThis PR introduces a new usermod bus infrastructure that enables usermods to function as custom LED drivers. It adds core bus-usermod bridging classes (BusUsermod and UsermodBus), implements a concrete AT8870 I2C PWM driver usermod, updates the bus manager to recognize the TYPE_USERMOD type, and refactors usermod declarations into a dedicated header file. Changes
Sequence Diagram(s)sequenceDiagram
participant App as WLED App
participant BusMgr as BusManager
participant BusUM as BusUsermod
participant UsermodBus as UsermodBus<br/>(AT8870)
participant Wire as I2C Bus
App->>BusMgr: add(BusConfig with TYPE_USERMOD)
BusMgr->>BusUM: BusUsermod(config)
BusUM->>UsermodBus: lookup(USERMOD_ID_BUS)
UsermodBus->>BusUM: initBus(bus)
UsermodBus->>BusUM: allocateBusData(4 bytes/pixel)
App->>BusUM: setPixelColor(idx, RGBW)
BusUM->>UsermodBus: setBusPixelColor(bus, idx, RGBW)
UsermodBus->>BusUM: store RGBW in 4-byte buffer
App->>BusUM: show()
BusUM->>UsermodBus: showBus(bus)
UsermodBus->>UsermodBus: calc PWM duty cycles
UsermodBus->>UsermodBus: calc inter-channel delays
UsermodBus->>Wire: beginTransmission
UsermodBus->>Wire: write(R_duty, G_duty, B_duty, W_duty, ...)
UsermodBus->>Wire: endTransmission
Wire->>Wire: I2C transmission
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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
added
enhancement
hardware
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
wled00/usermod.h (2)
21-35: Missing include forsize_t.This header uses
size_t(line 23) but doesn't include<stddef.h>or<cstddef>. While this works when the header is included after other headers that transitively definesize_t, it's fragile and could break if include order changes.💡 Proposed fix
`#ifndef` Usermod_h `#define` Usermod_h +#include <stddef.h> // for size_t + //um_manager.cpp typedef enum UM_Data_Types {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/usermod.h` around lines 21 - 35, The header declares UM_Exchange_Data and uses size_t for u_size (typedef um_data_t) but doesn't include the standard header that defines size_t; add an appropriate include (e.g., <cstddef> or <stddef.h>) at the top of the file so UM_Exchange_Data::u_size and the um_data_t typedef are unambiguous and won't break with different include orders.
36-36: Misleading size comment.The comment
// 12 bytesis architecture-dependent (pointer sizes vary between 32-bit and 64-bit platforms). On ESP32 (32-bit), it may be 12 bytes, but on 64-bit systems it would be larger. Consider removing or generalizing this comment.💡 Suggested fix
-const unsigned int um_data_size = sizeof(um_data_t); // 12 bytes +const unsigned int um_data_size = sizeof(um_data_t);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/usermod.h` at line 36, The inline comment on the declaration const unsigned int um_data_size = sizeof(um_data_t); incorrectly asserts a fixed size ("// 12 bytes") which is architecture-dependent; remove the hardcoded byte count or replace it with a note that the size depends on target architecture (e.g., "// size depends on architecture, use sizeof(um_data_t)"), and if you need to enforce a specific layout use static_assert or packed structs around um_data_t to guarantee size instead of a fixed comment.usermods/AT8870_I2C_PSPWM_v2/usermod.h (3)
60-73: Off-by-one inpwm_delaywrap-around logic.The condition
pwm_delay > 254followed bypwm_delay -= 255has an edge case: whenpwm_delay == 255, it becomes 0 (correct), but whenpwm_delay == 254, it doesn't wrap and stays at 254. If the intent is to wrap at 255, the condition should be>= 255.💡 Suggested fix
pwm_delay += pwm_duty; // PWM delay for green, start after red - if ( pwm_delay > 254 ) pwm_delay -= 255; + if ( pwm_delay >= 255 ) pwm_delay -= 255; pwm_duty = (G(c) * bri) / 255; // PWM duty for green, scaled by brightness Wire.write(pwm_duty); Wire.write(pwm_delay); pwm_delay += pwm_duty; // PWM delay for blue, start after green - if ( pwm_delay > 254 ) pwm_delay -= 255; + if ( pwm_delay >= 255 ) pwm_delay -= 255; pwm_duty = (B(c) * bri) / 255; // PWM duty for blue, scaled by brightness Wire.write(pwm_duty); Wire.write(pwm_delay); pwm_delay += pwm_duty; // PWM delay for white, start after blue - if ( pwm_delay > 254 ) pwm_delay -= 255; + if ( pwm_delay >= 255 ) pwm_delay -= 255;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/AT8870_I2C_PSPWM_v2/usermod.h` around lines 60 - 73, The pwm_delay wrap check uses "if (pwm_delay > 254) pwm_delay -= 255;" which misses the case when pwm_delay == 254 should wrap at 255; update each wrap condition (the three occurrences around pwm_delay updates in usermod.h) to use "if (pwm_delay >= 255) pwm_delay -= 255;" so pwm_delay correctly wraps when it reaches 255 after adding pwm_duty; ensure you change the conditions near the pwm_delay updates that precede writing green/blue/white pwm_duty and pwm_delay via Wire.write.
51-78: No I2C error handling.
Wire.endTransmission()returns a status code indicating success or failure (0 = success, 1-5 = various errors). Silently ignoring transmission failures makes debugging difficult if the I2C device is not responding or misconfigured. Consider logging or tracking failed transmissions.💡 Proposed error handling
- Wire.endTransmission(); + uint8_t err = Wire.endTransmission(); + if (err != 0) { + DEBUG_PRINTF("I2C transmission error: %d for address 0x%02X\n", err, pins[0]); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/AT8870_I2C_PSPWM_v2/usermod.h` around lines 51 - 78, The I2C transfer using Wire.endTransmission() in usermod.h currently ignores the return status; capture its return value and handle non-zero errors (1–5) by reporting or recording the failure (e.g., Serial.println/ESP_LOG, incrementing a failure counter, or triggering a retry/backoff), so replace the silent endTransmission() call with logic that checks the status and logs or reacts to failures when writing the PWM bytes via Wire.beginTransmission / Wire.write / Wire.endTransmission.
38-41: Only pixel 0 is read for output.
showBusretrieves color from pixel 0 only (getBusPixelColor(bus, 0)), which means this driver only supports single-pixel (single-channel) output. This is likely intentional for the AT8870 use case, but a brief comment clarifying this limitation would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/AT8870_I2C_PSPWM_v2/usermod.h` around lines 38 - 41, showBus currently reads only pixel 0 via getBusPixelColor(bus, 0), which restricts the driver to single-pixel output; either add a brief comment in showBus stating this intentional single-pixel (single-channel) limitation for the AT8870, or if multi-pixel support is desired, replace the single read with a loop using getBusNumPixels(bus) and getBusPixelColor(bus, idx) to process each pixel—update the showBus implementation (and any related logic) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platformio.ini`:
- Line 239: The change to default_partitions in platformio.ini should not be
committed in this PR; revert the edit that sets default_partitions =
tools/WLED_ESP32_4MB_1MB_FS.csv from platformio.ini, move this partition
override into a local/custom build config or an external platformio override
file for your usermod, and reopen the PR only after obtaining explicit approval
from a maintainer/organization Member; ensure you flag the modification for
review rather than leaving the default_partitions change in-tree.
In `@usermods/AT8870_I2C_PSPWM_v2/usermod.h`:
- Around line 31-36: getBusPixelColor reads four bytes without bounds-checking;
add the same guard used elsewhere to verify pix is in-range before accessing
getBusData. For example, at the start of getBusPixelColor(const BusUsermod* bus,
uint16_t pix) check that pix < bus->numPixels (or use the existing accessor like
getBusLength/getBusPixelCount if your codebase provides one) and return 0 (or a
safe default) when out of range, then proceed to compute i = pix * 4 and build
the RGBW32 value.
- Around line 21-29: setBusPixelColor currently computes i = pix * 4 and writes
4 bytes to the buffer returned by getBusData(bus) without range validation; add
a bounds check at the start of setBusPixelColor to ensure pix is within
bus->getLength() (or another appropriate length accessor) and that
getBusData(bus) is non-null before writing, returning early if the check fails
to prevent buffer overflow when writing R(c)/G(c)/B(c)/W(c).
In `@wled00/bus_manager.h`:
- Line 136: The change added TYPE_USERMOD to isVirtual(uint8_t type) but other
logic still treats usermod buses as non-virtual (BusConfig only copies pins[4]
and getNumVirtualBusses() counts only TYPE_VIRTUAL_MIN..MAX), causing usermod
buses to lose pins beyond 4 and occupy physical slots; update all related
helpers to treat TYPE_USERMOD consistently as virtual: extend BusConfig paths
that handle virtual buses to include TYPE_USERMOD so they copy the full virtual
config structure (not just pins[4]), update getNumVirtualBusses() to count
TYPE_USERMOD instances (or expand its range/logic accordingly), and audit any
other code paths that branch on isVirtual to ensure TYPE_USERMOD receives the
same virtual-bus handling as types in TYPE_VIRTUAL_MIN..TYPE_VIRTUAL_MAX.
- Around line 122-123: The branch in hasWhite() wrongly treats TYPE_USERMOD as
always having a white channel; remove TYPE_USERMOD from the hardcoded check and
instead query a usermod-supplied capability (e.g., add/consult a field or method
on the bus/usermod like usermodHasWhite() or a capability flag) so only buses
whose usermod declares white support use 4-byte pixels; update the hasWhite()
check to include TYPE_NET_DDP_RGBW and TYPE_NET_ARTNET_RGBW but rely on the
usermod capability for TYPE_USERMOD, and ensure buffer layout/offset logic reads
that same capability.
In `@wled00/bus_usermod.cpp`:
- Around line 41-47: Defer binding and replace the unsafe numeric cast: remove
the early call to usermods.lookup(USERMOD_ID_BUS) and the static_cast in this
init path so initBus() is not invoked before the usermod is initialized; instead
implement a typed probe method on the base class (e.g., Usermod::asBus() or a
virtual bool isBus()/UsermodBus* probeBus()) and call that from
UsermodBus::setup() where the usermod is guaranteed initialized, then have
UsermodBus::setup() perform the safe bind and call initBus(this) on the returned
UsermodBus pointer; keep USERMOD_ID_BUS usage for registration but avoid casting
by numeric ID in the early init sequence.
- Around line 56-60: BusUsermod::getPins currently dereferences pinArray even
when callers pass nullptr to query just the count; update the function so after
checking _valid and computing numPins it does not write into pinArray when
pinArray is nullptr—only copy _pins into pinArray if pinArray is non-null (e.g.,
if (pinArray) { for (...) pinArray[i] = _pins[i]; }) and always return the pin
count (or 0 when !_valid). Ensure you still use the same identifiers (_valid,
_pins, numPins, getPins) so the behavior matches other bus implementations.
In `@wled00/bus_usermod.h`:
- Line 24: Replace the non-standard type uint8 with the fixed-width standard
type uint8_t for the _pins member in class/struct declarations to ensure
portability; specifically update the declaration of _pins (uint8 _pins[5]) in
bus_usermod.h to use uint8_t and include <cstdint> or <stdint.h> if the header
does not already import it so the symbol _pins compiles with the correct type
across platforms.
In `@wled00/data/settings_leds.htm`:
- Line 262: The new fifth virtual config byte (L4) is being included in GPIO
checks later in UI(), so update the virtual fast-path there to treat indices
0..4 as non-GPIO (not just 0..3); locate the UI() routine and any
loops/conditions that currently use a hardcoded limit of 4 (e.g., checks like i
< 4 or exemption logic that only skips L0–L3) and change them to include L4 (i <
5 or equivalent), and ensure the pin-count calculation that sets `pins` (the
line using Math.min(gT(t).t.length,1) + 4*isVir(t) - 1*isNet(t)) remains
consistent with the new exemption so L4 uses 0–255 config semantics and is
excluded from max_gpio and pin-conflict highlighting.
---
Nitpick comments:
In `@usermods/AT8870_I2C_PSPWM_v2/usermod.h`:
- Around line 60-73: The pwm_delay wrap check uses "if (pwm_delay > 254)
pwm_delay -= 255;" which misses the case when pwm_delay == 254 should wrap at
255; update each wrap condition (the three occurrences around pwm_delay updates
in usermod.h) to use "if (pwm_delay >= 255) pwm_delay -= 255;" so pwm_delay
correctly wraps when it reaches 255 after adding pwm_duty; ensure you change the
conditions near the pwm_delay updates that precede writing green/blue/white
pwm_duty and pwm_delay via Wire.write.
- Around line 51-78: The I2C transfer using Wire.endTransmission() in usermod.h
currently ignores the return status; capture its return value and handle
non-zero errors (1–5) by reporting or recording the failure (e.g.,
Serial.println/ESP_LOG, incrementing a failure counter, or triggering a
retry/backoff), so replace the silent endTransmission() call with logic that
checks the status and logs or reacts to failures when writing the PWM bytes via
Wire.beginTransmission / Wire.write / Wire.endTransmission.
- Around line 38-41: showBus currently reads only pixel 0 via
getBusPixelColor(bus, 0), which restricts the driver to single-pixel output;
either add a brief comment in showBus stating this intentional single-pixel
(single-channel) limitation for the AT8870, or if multi-pixel support is
desired, replace the single read with a loop using getBusNumPixels(bus) and
getBusPixelColor(bus, idx) to process each pixel—update the showBus
implementation (and any related logic) accordingly.
In `@wled00/usermod.h`:
- Around line 21-35: The header declares UM_Exchange_Data and uses size_t for
u_size (typedef um_data_t) but doesn't include the standard header that defines
size_t; add an appropriate include (e.g., <cstddef> or <stddef.h>) at the top of
the file so UM_Exchange_Data::u_size and the um_data_t typedef are unambiguous
and won't break with different include orders.
- Line 36: The inline comment on the declaration const unsigned int um_data_size
= sizeof(um_data_t); incorrectly asserts a fixed size ("// 12 bytes") which is
architecture-dependent; remove the hardcoded byte count or replace it with a
note that the size depends on target architecture (e.g., "// size depends on
architecture, use sizeof(um_data_t)"), and if you need to enforce a specific
layout use static_assert or packed structs around um_data_t to guarantee size
instead of a fixed comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a6d40fa-c074-4aaa-9468-8b717beedcee
📒 Files selected for processing (11)
platformio.iniusermods/AT8870_I2C_PSPWM_v2/usermod.hwled00/bus_manager.cppwled00/bus_manager.hwled00/bus_usermod.cppwled00/bus_usermod.hwled00/const.hwled00/data/settings_leds.htmwled00/fcn_declare.hwled00/usermod.hwled00/usermods_list.cpp
| ;; | ||
| ;; please note that you can NOT update existing ESP32 installs with a "V4" build. Also updating by OTA will not work properly. | ||
| ;; You need to completely erase your device (esptool erase_flash) first, then install the "V4" build from VSCode+platformio. | ||
| default_partitions = tools/WLED_ESP32_4MB_1MB_FS.csv |
There was a problem hiding this comment.
Please keep this usermod PR out of the main platformio.ini.
Even a one-line change here can perturb shared CI/build environments, and this PR does not look like the dual-framework exception that needs core build-system edits. This should stay in a custom override/build config unless a maintainer explicitly wants it in-tree.
As per coding guidelines, modifications to platformio.ini MUST be approved explicitly by a maintainer or WLED organisation Member. Flag all modifications as they may break GitHub Action builds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platformio.ini` at line 239, The change to default_partitions in
platformio.ini should not be committed in this PR; revert the edit that sets
default_partitions = tools/WLED_ESP32_4MB_1MB_FS.csv from platformio.ini, move
this partition override into a local/custom build config or an external
platformio override file for your usermod, and reopen the PR only after
obtaining explicit approval from a maintainer/organization Member; ensure you
flag the modification for review rather than leaving the default_partitions
change in-tree.
| void setBusPixelColor(BusUsermod* bus, uint16_t pix, uint32_t c) override | ||
| { | ||
| uint8_t* data = getBusData(bus); | ||
| uint16_t i = pix * 4; | ||
| data[i++] = R(c); | ||
| data[i++] = G(c); | ||
| data[i++] = B(c); | ||
| data[i++] = W(c); | ||
| } |
There was a problem hiding this comment.
Missing bounds check before buffer access.
setBusPixelColor computes i = pix * 4 and writes 4 bytes without validating that pix < bus->getLength(). If an out-of-range pixel index is passed, this will write beyond the allocated buffer.
🛡️ Proposed bounds check
void setBusPixelColor(BusUsermod* bus, uint16_t pix, uint32_t c) override
{
+ if (pix >= bus->getLength()) return;
uint8_t* data = getBusData(bus);
uint16_t i = pix * 4;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/AT8870_I2C_PSPWM_v2/usermod.h` around lines 21 - 29,
setBusPixelColor currently computes i = pix * 4 and writes 4 bytes to the buffer
returned by getBusData(bus) without range validation; add a bounds check at the
start of setBusPixelColor to ensure pix is within bus->getLength() (or another
appropriate length accessor) and that getBusData(bus) is non-null before
writing, returning early if the check fails to prevent buffer overflow when
writing R(c)/G(c)/B(c)/W(c).
| uint32_t getBusPixelColor(const BusUsermod* bus, uint16_t pix) const override | ||
| { | ||
| uint8_t* data = getBusData(bus); | ||
| uint16_t i = pix * 4; | ||
| return RGBW32(data[i], data[i+1], data[i+2], data[i+3]); | ||
| } |
There was a problem hiding this comment.
Same bounds check needed for getBusPixelColor.
Reading from an out-of-range pixel index will access uninitialized or out-of-bounds memory.
🛡️ Proposed bounds check
uint32_t getBusPixelColor(const BusUsermod* bus, uint16_t pix) const override
{
+ if (pix >= bus->getLength()) return 0;
uint8_t* data = getBusData(bus);📝 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.
| uint32_t getBusPixelColor(const BusUsermod* bus, uint16_t pix) const override | |
| { | |
| uint8_t* data = getBusData(bus); | |
| uint16_t i = pix * 4; | |
| return RGBW32(data[i], data[i+1], data[i+2], data[i+3]); | |
| } | |
| uint32_t getBusPixelColor(const BusUsermod* bus, uint16_t pix) const override | |
| { | |
| if (pix >= bus->getLength()) return 0; | |
| uint8_t* data = getBusData(bus); | |
| uint16_t i = pix * 4; | |
| return RGBW32(data[i], data[i+1], data[i+2], data[i+3]); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/AT8870_I2C_PSPWM_v2/usermod.h` around lines 31 - 36,
getBusPixelColor reads four bytes without bounds-checking; add the same guard
used elsewhere to verify pix is in-range before accessing getBusData. For
example, at the start of getBusPixelColor(const BusUsermod* bus, uint16_t pix)
check that pix < bus->numPixels (or use the existing accessor like
getBusLength/getBusPixelCount if your codebase provides one) and return 0 (or a
safe default) when out of range, then proceed to compute i = pix * 4 and build
the RGBW32 value.
| type == TYPE_NET_DDP_RGBW || type == TYPE_NET_ARTNET_RGBW || | ||
| type == TYPE_USERMOD; // network types with white channel |
There was a problem hiding this comment.
Don’t hardcode every usermod bus as RGBW.
TYPE_USERMOD is the generic extension point, but hasWhite() feeds both UI parsing and buffer layout. With this branch every usermod bus gets white-channel handling and 4-byte pixel storage, so an RGB-only/custom implementation will receive the wrong channel offsets. Please make these capabilities usermod-supplied instead of baking them into the shared type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/bus_manager.h` around lines 122 - 123, The branch in hasWhite()
wrongly treats TYPE_USERMOD as always having a white channel; remove
TYPE_USERMOD from the hardcoded check and instead query a usermod-supplied
capability (e.g., add/consult a field or method on the bus/usermod like
usermodHasWhite() or a capability flag) so only buses whose usermod declares
white support use 4-byte pixels; update the hasWhite() check to include
TYPE_NET_DDP_RGBW and TYPE_NET_ARTNET_RGBW but rely on the usermod capability
for TYPE_USERMOD, and ensure buffer layout/offset logic reads that same
capability.
| static constexpr bool isOnOff(uint8_t type) { return (type == TYPE_ONOFF); } | ||
| static constexpr bool isPWM(uint8_t type) { return (type >= TYPE_ANALOG_MIN && type <= TYPE_ANALOG_MAX); } | ||
| static constexpr bool isVirtual(uint8_t type) { return (type >= TYPE_VIRTUAL_MIN && type <= TYPE_VIRTUAL_MAX); } | ||
| static constexpr bool isVirtual(uint8_t type) { return ((type >= TYPE_VIRTUAL_MIN && type <= TYPE_VIRTUAL_MAX) || type == TYPE_USERMOD); } |
There was a problem hiding this comment.
TYPE_USERMOD doesn’t fit the existing virtual-bus helper semantics yet.
In this header, isVirtual() is reused for more than classification: BusConfig still copies only 4 config bytes for virtual buses, and getNumVirtualBusses() still only counts the old 80-95 network range. So this change drops pins[4] for usermod buses and still makes them consume a physical-bus slot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/bus_manager.h` at line 136, The change added TYPE_USERMOD to
isVirtual(uint8_t type) but other logic still treats usermod buses as
non-virtual (BusConfig only copies pins[4] and getNumVirtualBusses() counts only
TYPE_VIRTUAL_MIN..MAX), causing usermod buses to lose pins beyond 4 and occupy
physical slots; update all related helpers to treat TYPE_USERMOD consistently as
virtual: extend BusConfig paths that handle virtual buses to include
TYPE_USERMOD so they copy the full virtual config structure (not just pins[4]),
update getNumVirtualBusses() to count TYPE_USERMOD instances (or expand its
range/logic accordingly), and audit any other code paths that branch on
isVirtual to ensure TYPE_USERMOD receives the same virtual-bus handling as types
in TYPE_VIRTUAL_MIN..TYPE_VIRTUAL_MAX.
| // error: 'dynamic_cast' not permitted with '-fno-rtti' | ||
| // _usermod = dynamic_cast<UsermodBus*> (usermods.lookup(USERMOD_ID_BUS)); | ||
| _usermod = static_cast<UsermodBus*> (usermods.lookup(USERMOD_ID_BUS)); | ||
|
|
||
| if ( _usermod ) { | ||
| _usermod->initBus(this); | ||
| } |
There was a problem hiding this comment.
Binding the backing usermod here is too early, and the cast is not type-safe.
At this point the usermod has been registered but not yet run through usermods.setup(), so initBus() can fire before the usermod initializes itself. Separately, lookup(USERMOD_ID_BUS) is only an ID match; static_cast<UsermodBus*> becomes UB if any unrelated usermod also returns 127. Please defer binding to UsermodBus::setup() and expose a typed probe on Usermod instead of casting by numeric ID.
Based on learnings, hardcoded usermod IDs are intentionally used as part of a strategy to avoid modifying core code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/bus_usermod.cpp` around lines 41 - 47, Defer binding and replace the
unsafe numeric cast: remove the early call to usermods.lookup(USERMOD_ID_BUS)
and the static_cast in this init path so initBus() is not invoked before the
usermod is initialized; instead implement a typed probe method on the base class
(e.g., Usermod::asBus() or a virtual bool isBus()/UsermodBus* probeBus()) and
call that from UsermodBus::setup() where the usermod is guaranteed initialized,
then have UsermodBus::setup() perform the safe bind and call initBus(this) on
the returned UsermodBus pointer; keep USERMOD_ID_BUS usage for registration but
avoid casting by numeric ID in the early init sequence.
| uint8_t BusUsermod::getPins(uint8_t* pinArray) const { | ||
| if (!_valid) return 0; | ||
| unsigned numPins = 5; | ||
| for (unsigned i = 0; i < numPins; i++) pinArray[i] = _pins[i]; | ||
| return numPins; |
There was a problem hiding this comment.
Honor the getPins(nullptr) contract in this override.
Other bus implementations let callers query just the pin count without supplying a buffer. This version unconditionally dereferences pinArray and will crash on that path.
Suggested fix
uint8_t BusUsermod::getPins(uint8_t* pinArray) const {
if (!_valid) return 0;
unsigned numPins = 5;
- for (unsigned i = 0; i < numPins; i++) pinArray[i] = _pins[i];
+ if (pinArray) for (unsigned i = 0; i < numPins; i++) pinArray[i] = _pins[i];
return numPins;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/bus_usermod.cpp` around lines 56 - 60, BusUsermod::getPins currently
dereferences pinArray even when callers pass nullptr to query just the count;
update the function so after checking _valid and computing numPins it does not
write into pinArray when pinArray is nullptr—only copy _pins into pinArray if
pinArray is non-null (e.g., if (pinArray) { for (...) pinArray[i] = _pins[i]; })
and always return the pin count (or 0 when !_valid). Ensure you still use the
same identifiers (_valid, _pins, numPins, getPins) so the behavior matches other
bus implementations.
|
|
||
| protected: | ||
| UsermodBus* _usermod; | ||
| uint8 _pins[5]; // used as configuration hints for the Usermod |
There was a problem hiding this comment.
Use uint8_t instead of uint8.
uint8 is not a standard C/C++ type. Use uint8_t for portability and consistency with the rest of the codebase.
🔧 Proposed fix
- uint8 _pins[5]; // used as configuration hints for the Usermod
+ uint8_t _pins[5]; // used as configuration hints for the Usermod📝 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.
| uint8 _pins[5]; // used as configuration hints for the Usermod | |
| uint8_t _pins[5]; // used as configuration hints for the Usermod |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/bus_usermod.h` at line 24, Replace the non-standard type uint8 with
the fixed-width standard type uint8_t for the _pins member in class/struct
declarations to ensure portability; specifically update the declaration of _pins
(uint8 _pins[5]) in bus_usermod.h to use uint8_t and include <cstdint> or
<stdint.h> if the header does not already import it so the symbol _pins compiles
with the correct type across platforms.
| gId("off"+n).innerText = off; | ||
| // secondary pins show/hide (type string length is equivalent to number of pins used; except for network and on/off) | ||
| let pins = Math.min(gT(t).t.length,1) + 3*isNet(t); // fixes network pins to 4 | ||
| let pins = Math.min(gT(t).t.length,1) + 4*isVir(t) - 1*isNet(t); // fixes network pins to 4, other virtual to 5 |
There was a problem hiding this comment.
The new fifth virtual config field is still treated like a GPIO later in UI().
This branch exposes L4 for non-network virtual buses, but the later virtual fast-path still only exempts L0-L3. As a result, L4 keeps max_gpio limits and pin-conflict highlighting instead of 0-255 config semantics, so the fifth usermod config byte is unusable for many values.
Suggested follow-up
- if (nm=="L0" || nm=="L1" || nm=="L2" || nm=="L3") {
+ if (nm=="L0" || nm=="L1" || nm=="L2" || nm=="L3" || nm=="L4") {
if (isVir(t)) {
LC.max = 255;
LC.min = 0;
LC.style.color="#fff";
return; // do not check conflicts
} else {📝 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.
| let pins = Math.min(gT(t).t.length,1) + 4*isVir(t) - 1*isNet(t); // fixes network pins to 4, other virtual to 5 | |
| if (nm=="L0" || nm=="L1" || nm=="L2" || nm=="L3" || nm=="L4") { | |
| if (isVir(t)) { | |
| LC.max = 255; | |
| LC.min = 0; | |
| LC.style.color="#fff"; | |
| return; // do not check conflicts | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/data/settings_leds.htm` at line 262, The new fifth virtual config byte
(L4) is being included in GPIO checks later in UI(), so update the virtual
fast-path there to treat indices 0..4 as non-GPIO (not just 0..3); locate the
UI() routine and any loops/conditions that currently use a hardcoded limit of 4
(e.g., checks like i < 4 or exemption logic that only skips L0–L3) and change
them to include L4 (i < 5 or equivalent), and ensure the pin-count calculation
that sets `pins` (the line using Math.min(gT(t).t.length,1) + 4*isVir(t) -
1*isNet(t)) remains consistent with the new exemption so L4 uses 0–255 config
semantics and is excluded from max_gpio and pin-conflict highlighting.
softhack007
added
the
needs_rework
4 participants
PR regarding bus implementations as usermods, as discussed with @blazoncek
Summary by CodeRabbit
New Features
Updates