"Unlimited" buses #4529
"Unlimited" buses #4529
Conversation
- use unique_ptr/make_unique for busses
- added config upload options - number of buses it limited to 36 (0-9+A-Z identifiers) - WRNING web server may not support that many variables
|
Apparently will require #4484 to be merged 1st. |
| // Really simple C++11 shim for non-array case; implementation from cppreference.com | ||
| template<class T, class... Args> | ||
| std::unique_ptr<T> | ||
| make_unique(Args&&... args) |
There was a problem hiding this comment.
should be moved to a different header file
There was a problem hiding this comment.
True, but I want to limit number of modified files for transparency sake.
Once toolchain is updated we may not need this.
There was a problem hiding this comment.
for transparency sake
The git diff won't look much different
Once toolchain is updated
Will that actually happen? ^^
Anyway it should be easier and cleaner to delete a polyfill.h than to delete a function here and check if other files included bus_manager.h just for the sake of make_unique
wled00/bus_manager.h
Outdated
| @@ -78,7 +93,7 @@ class Bus { | |||
| _autoWhiteMode = Bus::hasWhite(type) ? aw : RGBW_MODE_MANUAL_ONLY; | |||
| }; | |||
|
|
|||
| virtual ~Bus() {} //throw the bus under the bus | |||
| virtual ~Bus() {} //throw the bus under the bus (derived class needs to freeData()) | |||
There was a problem hiding this comment.
One way to make this safer would be to call freeData() here, and then override freeData() in the busses that don't need to do anything with an empty function
There was a problem hiding this comment.
Can you elaborate a bit more?
I was thinking if Bus does not allocate any data for itself (but provides non-virtual methods) a derived class should take care of deallocation if it allocates anything.
My first thoughts were to implement automatic deallocation upon destruction, but some derived classes assign _data pointer to class member variables to reduce heap fragmentation. In such case no deallocation should happen.
There was a problem hiding this comment.
I was thinking about something like this:
//Bus
virtual Bus::~Bus() { freeData(); }
virtual void Bus::freeData() { if (_data != nullptr) free(_data); _data = nullptr; }
//Bus1 uses _data and doesn't override freeData()
//Bus2 doesn't use _data
virtual void Bus::freeData() {}However:
freeData checks if _data is nullptr, so really I don't think you'd have to do anything like that:
//Bus
virtual Bus::~Bus() { freeData(); }
void Bus::freeData() { if (_data != nullptr) free(_data); _data = nullptr; }
//Bus1 uses _data and doesn't override freeData()
//Bus2 doesn't use _data and doesn't override freeData()Should work just fine, shouldn't it?
Another option would be to move _data into the sub classes, so each class would be responsible for their own memory.
There was a problem hiding this comment.
Another option would be to move _data into the sub classes, so each class would be responsible for their own memory.
This would be my preferred option. It also lets each class use type-safe storage and descriptive names for their state data as well.
There was a problem hiding this comment.
I find that counterintuitive.
All derived classes may find use of _data pointer but some may allocate space on heap, some on stack (or on heap within its instance). For those that use heap, allocateData() and freeData() are provided.
If I understand virtual methods correctly if you override virtual base method in a derived class then you have to call base method explicitly if you want to use its implementation. I.e.
void Derived::freeData() {
... some stuff ...
Base::freeData();
... more stuff ...
}or implement functionality of Base::freeData() in derived (a nonsense). Having virtual Base::freeData() that does nothing (and is not pure virtual) and having pure virtual (that needs to be implemented in derived class) seems counterproductive in this particular instance as allocateData() does something.
As allocateData() and freeData() are not virtual, two less pointers need to be allocated for derived class' vtables (which is good in our case). Of course that means that when derived class uses heap allocated data, it needs to call allocateData() at some point and then (at least in destructor) call freeData(). All is well and also well defined.
One could add heap/stack detection into virtual Base::freeData() (if that approach is taken) so that correct de-allocation may happen, but I think that is a hack best avoided as it is architecture dependant.
BTW: All of the derived classes use _data pointer in one way or another, but only BusDigital and BusNetwork use heap allocated buffers. When I first implemented buffers, _data was defined in those two derived classes (as suggested), but that meant to also implement 2 allocateData() and 2 freeData() methods. I found that redundant and prone to error more than forgetting to call base class freeData().
There was a problem hiding this comment.
do we need allocateData or freeData?
allocateData is called exactly once, during construction (in 2 classes). Which means there is no need for gating or freeing _data before allocation. The zero length check is probably also unnecessary, leaving only one effective call: calloc. Does it make sense to abstract that? Not really. Does it improve anything to have allocateData that can be used in two classes instead of calloc (which can also be used in both classes)? Well, no.
What about freeData? Once again, it is called exactly once, during destruction (in 2 classes). Oh but it performs a nullptr check, that has to be useful, right? well, no; free does nothing when called with nullptr. Ok but it also sets _data to nullptr when it's done. Which sounds like a good safety move, but remember, this happens during destruction, so even reading the value _data would be unsafe. So really this function can be replaced with a single free call as well.
About the error-proneness of this all: using calloc and free is error prone. If you are worried about that, consider unique_ptr or other type safe per-class solutions instead :)
There was a problem hiding this comment.
What about Bus::_data then? It should also be eliminated and moved into those 2 classes that use it. Right?
There was a problem hiding this comment.
Well, in an ideal world, yes. I'm not sure how other functions that currently use _data would work then though.
| extern std::vector<std::unique_ptr<Bus>> busses; | ||
| //extern std::vector<Bus*> busses; | ||
| extern uint16_t _gMilliAmpsUsed; | ||
| extern uint16_t _gMilliAmpsMax; |
There was a problem hiding this comment.
I still think adding public and extern gloabals is a really bad idea
There was a problem hiding this comment.
Even if enclosed in namespace?
I could do without but then functions could no longer be inline (not that this is good or bad).
There was a problem hiding this comment.
- They are public now. Anyone can access and modify them. Functional safety isn't about not doing the wrong thing because you know what you are doing, its about preventing the wrong thing even if not everyone knows what they are doing.
- You now have to separate definition and initialization.
externcan prevent optimizations
Avoiding this would work by keeping the class or moving the variables to the cpp. I don't know how the latter affects embedded code though.
There was a problem hiding this comment.
Avoiding this would work by keeping the class or moving the variables to the cpp. I don't know how the latter affects embedded code though.
Moving the declarations in to the cpp would give up any inlined access -- everything would have to be a function call. We don't have link-time optimization to help us here. :(
The previous code (a non-instantiated class, where everything is static) is the only way I know of to get both access control and still support inlining.
There was a problem hiding this comment.
WLED is full of globals, accessible/accessed throughout the code.
If you'll look many may be shadowed in certain functions which is prone to error (forget or accidentally remove definition and you are using global).
Using namespaced variable reduces the possibility to inadvertently use such global. But yes, I agree that having globals is poor programming practice.
As we discuss these issues it may indeed be better to forego namespace and keep using instanceless class.
- as suggested by @TripleWhy - minimum length guard Conflicts: wled00/bus_manager.cpp wled00/bus_manager.h
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes refactor several modules to improve pointer safety, memory management, and control flow. Bus pointers are now declared as const, and their validity checks are simplified. The bus management system has transitioned from fixed-size raw pointer arrays to using Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration Loader
participant BM as Bus Manager
participant Bus as Bus Object
Config->>BM: deserializeConfig()
BM->>BM: Iterate bus configurations (fixed limit)
BM->>Bus: Validate bus (using const pointer check)
Bus-->>BM: Return bus status
BM->>BM: Add bus config via vector emplace_back
BM-->>Config: Return updated configuration
sequenceDiagram
participant JS as JS UI
participant LCM as LED Config Manager
participant LED as LED Bus
JS->>LCM: loadCfg()
LCM->>LED: Remove existing LED outputs
LCM->>LCM: Loop through LED configurations (fixed limit of 36)
LCM->>LED: Generate LED ID using chrID and update properties
LED-->>LCM: LED settings applied
LCM-->>JS: Return updated LED configuration
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
wled00/cfg.cpp (1)
190-190: Too many arguments to BusManager::memUsage().
The current function signature of memUsage() takes no arguments, causing a build error. Implement an overload or remove extra parameters.- mem = BusManager::memUsage(maxChannels, maxLedsOnBus, 8); + // Either implement an overloaded memUsage(...) or remove arguments: + // mem = BusManager::memUsage();🧰 Tools
🪛 GitHub Actions: WLED CI
[error] 190-190: Too many arguments to function 'size_t BusManager::memUsage()'.
🧹 Nitpick comments (3)
wled00/data/settings_leds.htm (1)
353-364: Consider using constants for hardware-specific values.While the logic correctly handles different ESP variants, using magic numbers makes the code harder to maintain. Consider defining constants for these values.
+// Hardware-specific constants +const S2_MAX_BUSES = 14; +const S2_MAX_VIRTUAL = 4; +const S3_MAX_VIRTUAL = 6; +const RMT_CHANNELS_S2_S3 = 4; +const RMT_CHANNELS_ESP32 = 8; +const PARALLEL_I2S_CHANNELS = 8; + - const S2 = (oMaxB == 14) && (maxV == 4); - const S3 = (oMaxB == 14) && (maxV == 6); + const S2 = (oMaxB == S2_MAX_BUSES) && (maxV == S2_MAX_VIRTUAL); + const S3 = (oMaxB == S2_MAX_BUSES) && (maxV == S3_MAX_VIRTUAL);wled00/bus_manager.h (1)
14-24: Consider moving this C++11 polyfill to a dedicated header or shared utility.
Currently, the inline implementation of make_unique might be duplicated elsewhere or may cause clutter. Creating a separate "cpp11_compat.h" or similar makes the code more modular.wled00/FX_fcn.cpp (1)
1395-1395: Fix integer signedness comparison warning.The comparison between
i(int) andBusManager::getNumBusses()(size_t) triggers a warning. Consider changing the loop variable type tosize_tto match the return type ofgetNumBusses().- for (int i=0; i<BusManager::getNumBusses(); i++) { + for (size_t i=0; i<BusManager::getNumBusses(); i++) {🧰 Tools
🪛 GitHub Actions: WLED CI
[warning] 1395-1395: Comparison of integer expressions of different signedness: 'int' and 'size_t'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
wled00/FX_fcn.cpp(7 hunks)wled00/bus_manager.cpp(29 hunks)wled00/bus_manager.h(16 hunks)wled00/cfg.cpp(3 hunks)wled00/const.h(3 hunks)wled00/data/settings_leds.htm(5 hunks)wled00/set.cpp(3 hunks)wled00/wled.h(1 hunks)wled00/xml.cpp(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: WLED CI
wled00/FX_fcn.cpp
[warning] 1395-1395: Comparison of integer expressions of different signedness: 'int' and 'size_t'.
wled00/cfg.cpp
[error] 190-190: Too many arguments to function 'size_t BusManager::memUsage()'.
wled00/bus_manager.cpp
[error] 148-148: 'd_calloc' was not declared in this scope. Suggested alternative: 'ps_calloc'.
[error] 387-387: 'getDataSize' is not a member of 'PolyBus'.
[error] 747-747: 'd_calloc' was not declared in this scope. Suggested alternative: 'ps_calloc'.
[error] 810-810: 'memUsage' is not a member of 'PolyBus'.
[error] 810-810: Invalid conversion from 'const uint8_t*' to 'uint8_t*'.
🔇 Additional comments (21)
wled00/data/settings_leds.htm (2)
44-53: Great documentation of hardware-specific limits!The detailed comments for each parameter clearly explain the maximum values for different ESP models, which improves code maintainability and helps prevent configuration errors.
424-425: 🏁 Script executed:#!/bin/bash # Search for memory-related constants and calculations rg -A 2 'maxM|bquot|memu'Length of output: 2834
wled00/bus_manager.h (2)
86-86: Good safeguard for minimum length.
Enforcing at least length 1 avoids edge cases with zero-length buses.
98-98: Verify the default return of canShow().
Returning true by default may mask bus-specific readiness checks. If a derived bus must hold off on updates, ensure it properly overrides this method.wled00/cfg.cpp (1)
225-226: Use of emplace_back is a good improvement.
Emplace construction simplifies memory management for BusConfig.wled00/const.h (2)
52-52: LGTM! Updated virtual bus purpose for ESP8266.The comment clarifies that virtual buses are now used for UI distinction rather than bus creation, which aligns with the architectural changes.
66-70: Verify ESP32S3 bus configuration changes.The significant increase in maximum buses (from 6 to 14) and digital channels (to 12) for ESP32S3 needs validation:
- Max buses increased from 6 to 14
- Digital channels increased to support x4 RMT + x8 I2S-LCD
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify ESP32S3 bus configuration changes # Check for any existing bus configuration that might be affected # Search for bus configurations in the codebase rg -A 5 "WLED_MAX_BUSSES|WLED_MAX_DIGITAL_CHANNELS" --type cppLength of output: 7707
ESP32S3 bus configuration verified
Based on the grep output, the changes for CONFIG_IDF_TARGET_ESP32S3 are properly applied in wled00/const.h with:
• WLED_MAX_BUSSES set to 14 (supporting 12 digital and 2 analog bus channels)
• WLED_MAX_DIGITAL_CHANNELS set to 12 (documented as x4 RMT + x8 I2S‐LCD)Furthermore, the updated macros are referenced consistently in files like wled.cpp, bus_manager.cpp, and others. There’s no indication of further conflicts or legacy checks affecting the new configuration for ESP32S3.
wled00/xml.cpp (6)
295-296: LGTM! Improved bus pointer safety.The changes enhance safety by:
- Using const pointer for bus object
- Adding comprehensive validation with
!bus || !bus->isOk()
315-315: LGTM! Fixed potential integer overflow.Changed from offset-based to direct ASCII calculation, preventing potential integer overflow issues.
360-360: LGTM! Enhanced entry validation.Added length check
!entry || !entry->lento prevent processing invalid entries.
275-275: LGTM! Helpful comment added.The added comment clarifies that quotes must be included when defining
WLED_RELEASE_NAME, which helps prevent initialization errors.
295-296: Improved bus pointer safety and validation.The changes enhance robustness through:
- Using
constto prevent unintended modifications to the bus.- Adding
isOk()check to ensure bus validity.- Using
breakto stop processing when an invalid bus is encountered.
315-316: Enhanced pin handling for both physical and virtual buses.The changes improve pin configuration by:
- Using direct ASCII conversion for pin indexing.
- Adding support for virtual bus configurations through the
isVirtual()check.wled00/wled.h (3)
894-898: LGTM! Improved bus configuration management.The changes enhance memory management and control:
- Using
std::vector<BusConfig>for dynamic sizing- Added control variables for bus initialization and LED mapping
Note: These changes align with the PR objective to support up to 36 buses by removing fixed-size limitations.
894-895: Improved memory management using std::vector.The changes enhance safety and flexibility by:
- Using
std::vector<BusConfig>for dynamic bus configuration storage.- Eliminating manual memory management of raw pointer arrays.
896-898: Added support for bus initialization and LED mapping.The changes add necessary state tracking for:
- Bus initialization through the
doInitBussesflag.- LED map loading and current map tracking.
wled00/set.cpp (1)
144-144: LGTM! The loop limit change aligns with the PR objective.The change from using constants to a fixed value of 36 aligns with the PR objective of supporting up to 36 buses. The debug output has been improved to show the number of buses processed.
Also applies to: 160-160
wled00/FX_fcn.cpp (4)
1120-1122: Improved const correctness and simplified bus validation.The bus pointer is now correctly marked as const since the function doesn't modify the bus. The bus validity check has been simplified to a more concise form.
1397-1397: Simplified bus validity check.The bus validity check has been simplified while maintaining the same functionality.
1691-1692: Improved const correctness in hasRGBWBus().The bus pointer is now correctly marked as const since the function only reads from the bus. The bus validity check has been simplified.
1701-1702: Improved const correctness in hasCCTBus().The bus pointer is now correctly marked as const since the function only reads from the bus. The bus validity check has been simplified.
| let c = JSON.parse(lines); | ||
| if (c.hw) { | ||
| if (c.hw.led) { | ||
| for (var i=0; i<10; i++) addLEDs(-1); | ||
| var l = c.hw.led; | ||
| // remove all existing outputs | ||
| for (const i=0; i<36; i++) addLEDs(-1); // was i<maxb+maxV when number of virtual buses was limited (now :"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ") | ||
| let l = c.hw.led; | ||
| l.ins.forEach((v,i,a)=>{ | ||
| addLEDs(1); | ||
| for (var j=0; j<v.pin.length; j++) d.getElementsByName(`L${j}${i}`)[0].value = v.pin[j]; | ||
| d.getElementsByName("LT"+i)[0].value = v.type; | ||
| d.getElementsByName("LS"+i)[0].value = v.start; | ||
| d.getElementsByName("LC"+i)[0].value = v.len; | ||
| d.getElementsByName("CO"+i)[0].value = v.order; | ||
| d.getElementsByName("SL"+i)[0].value = v.skip; | ||
| d.getElementsByName("LT"+i)[0].value = v.type; | ||
| d.getElementsByName("LS"+i)[0].value = v.start; | ||
| d.getElementsByName("LC"+i)[0].value = v.len; | ||
| d.getElementsByName("CO"+i)[0].value = v.order & 0x0F; | ||
| d.getElementsByName("SL"+i)[0].value = v.skip; | ||
| d.getElementsByName("RF"+i)[0].checked = v.ref; | ||
| d.getElementsByName("CV"+i)[0].checked = v.rev; | ||
| d.getElementsByName("AW"+i)[0].value = v.rgbwm; | ||
| d.getElementsByName("WO"+i)[0].value = (v.order>>4) & 0x0F; | ||
| d.getElementsByName("SP"+i)[0].value = v.freq; | ||
| d.getElementsByName("LA"+i)[0].value = v.ledma; | ||
| d.getElementsByName("MA"+i)[0].value = v.maxpwr; | ||
| }); | ||
| d.getElementsByName("PR")[0].checked = l.prl | 0; | ||
| d.getElementsByName("LD")[0].checked = l.ld; | ||
| d.getElementsByName("MA")[0].value = l.maxpwr; | ||
| d.getElementsByName("ABL")[0].checked = l.maxpwr > 0; | ||
| } | ||
| if(c.hw.com) { | ||
| resetCOM(); | ||
| c.hw.com.forEach(e => { | ||
| addCOM(e.start, e.len, e.order); | ||
| }); | ||
| } | ||
| if (c.hw.btn) { | ||
| var b = c.hw.btn; | ||
| let b = c.hw.btn; | ||
| if (b) { | ||
| if (Array.isArray(b.ins)) gId("btns").innerHTML = ""; | ||
| b.ins.forEach((v,i,a)=>{ | ||
| addBtn(i,v.pin[0],v.type); | ||
| }); | ||
| d.getElementsByName("TT")[0].value = b.tt; | ||
| } | ||
| if (c.hw.ir) { | ||
| d.getElementsByName("IR")[0].value = c.hw.ir.pin; | ||
| d.getElementsByName("IT")[0].value = c.hw.ir.type; | ||
| let ir = c.hw.ir; | ||
| if (ir) { | ||
| d.getElementsByName("IR")[0].value = ir.pin; | ||
| d.getElementsByName("IT")[0].value = ir.type; | ||
| } | ||
| let rl = c.hw.relay; | ||
| if (rl) { | ||
| d.getElementsByName("RL")[0].value = rl.pin; | ||
| d.getElementsByName("RM")[0].checked = rl.rev; | ||
| d.getElementsByName("RO")[0].checked = rl.odrain; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for loaded configurations.
While the configuration loading handles the new bus limit, it lacks validation for:
- Maximum number of buses per ESP variant
- Memory constraints
- Valid pin configurations
Add validation before applying the configuration:
let c = JSON.parse(lines);
+// Validate configuration
+function validateConfig(config) {
+ if (!config.hw?.led?.ins) return false;
+ const totalBuses = config.hw.led.ins.length;
+ if (totalBuses > 36) return false;
+ // Add more validation as needed
+ return true;
+}
+
if (c.hw) {
+ if (!validateConfig(c)) {
+ alert("Invalid configuration!");
+ return;
+ }
if (c.hw.led) {📝 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 c = JSON.parse(lines); | |
| if (c.hw) { | |
| if (c.hw.led) { | |
| for (var i=0; i<10; i++) addLEDs(-1); | |
| var l = c.hw.led; | |
| // remove all existing outputs | |
| for (const i=0; i<36; i++) addLEDs(-1); // was i<maxb+maxV when number of virtual buses was limited (now :"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ") | |
| let l = c.hw.led; | |
| l.ins.forEach((v,i,a)=>{ | |
| addLEDs(1); | |
| for (var j=0; j<v.pin.length; j++) d.getElementsByName(`L${j}${i}`)[0].value = v.pin[j]; | |
| d.getElementsByName("LT"+i)[0].value = v.type; | |
| d.getElementsByName("LS"+i)[0].value = v.start; | |
| d.getElementsByName("LC"+i)[0].value = v.len; | |
| d.getElementsByName("CO"+i)[0].value = v.order; | |
| d.getElementsByName("SL"+i)[0].value = v.skip; | |
| d.getElementsByName("LT"+i)[0].value = v.type; | |
| d.getElementsByName("LS"+i)[0].value = v.start; | |
| d.getElementsByName("LC"+i)[0].value = v.len; | |
| d.getElementsByName("CO"+i)[0].value = v.order & 0x0F; | |
| d.getElementsByName("SL"+i)[0].value = v.skip; | |
| d.getElementsByName("RF"+i)[0].checked = v.ref; | |
| d.getElementsByName("CV"+i)[0].checked = v.rev; | |
| d.getElementsByName("AW"+i)[0].value = v.rgbwm; | |
| d.getElementsByName("WO"+i)[0].value = (v.order>>4) & 0x0F; | |
| d.getElementsByName("SP"+i)[0].value = v.freq; | |
| d.getElementsByName("LA"+i)[0].value = v.ledma; | |
| d.getElementsByName("MA"+i)[0].value = v.maxpwr; | |
| }); | |
| d.getElementsByName("PR")[0].checked = l.prl | 0; | |
| d.getElementsByName("LD")[0].checked = l.ld; | |
| d.getElementsByName("MA")[0].value = l.maxpwr; | |
| d.getElementsByName("ABL")[0].checked = l.maxpwr > 0; | |
| } | |
| if(c.hw.com) { | |
| resetCOM(); | |
| c.hw.com.forEach(e => { | |
| addCOM(e.start, e.len, e.order); | |
| }); | |
| } | |
| if (c.hw.btn) { | |
| var b = c.hw.btn; | |
| let b = c.hw.btn; | |
| if (b) { | |
| if (Array.isArray(b.ins)) gId("btns").innerHTML = ""; | |
| b.ins.forEach((v,i,a)=>{ | |
| addBtn(i,v.pin[0],v.type); | |
| }); | |
| d.getElementsByName("TT")[0].value = b.tt; | |
| } | |
| if (c.hw.ir) { | |
| d.getElementsByName("IR")[0].value = c.hw.ir.pin; | |
| d.getElementsByName("IT")[0].value = c.hw.ir.type; | |
| let ir = c.hw.ir; | |
| if (ir) { | |
| d.getElementsByName("IR")[0].value = ir.pin; | |
| d.getElementsByName("IT")[0].value = ir.type; | |
| } | |
| let rl = c.hw.relay; | |
| if (rl) { | |
| d.getElementsByName("RL")[0].value = rl.pin; | |
| d.getElementsByName("RM")[0].checked = rl.rev; | |
| d.getElementsByName("RO")[0].checked = rl.odrain; | |
| } | |
| let c = JSON.parse(lines); | |
| // Validate configuration | |
| function validateConfig(config) { | |
| if (!config.hw?.led?.ins) return false; | |
| const totalBuses = config.hw.led.ins.length; | |
| if (totalBuses > 36) return false; | |
| // Add more validation as needed | |
| return true; | |
| } | |
| if (c.hw) { | |
| if (!validateConfig(c)) { | |
| alert("Invalid configuration!"); | |
| return; | |
| } | |
| if (c.hw.led) { | |
| // remove all existing outputs | |
| for (const i=0; i<36; i++) addLEDs(-1); // was i<maxb+maxV when number of virtual buses was limited (now :"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ") | |
| let l = c.hw.led; | |
| l.ins.forEach((v,i,a)=>{ | |
| addLEDs(1); | |
| for (var j=0; j<v.pin.length; j++) d.getElementsByName(`L${j}${i}`)[0].value = v.pin[j]; | |
| d.getElementsByName("LT"+i)[0].value = v.type; | |
| d.getElementsByName("LS"+i)[0].value = v.start; | |
| d.getElementsByName("LC"+i)[0].value = v.len; | |
| d.getElementsByName("CO"+i)[0].value = v.order & 0x0F; | |
| d.getElementsByName("SL"+i)[0].value = v.skip; | |
| d.getElementsByName("RF"+i)[0].checked = v.ref; | |
| d.getElementsByName("CV"+i)[0].checked = v.rev; | |
| d.getElementsByName("AW"+i)[0].value = v.rgbwm; | |
| d.getElementsByName("WO"+i)[0].value = (v.order>>4) & 0x0F; | |
| d.getElementsByName("SP"+i)[0].value = v.freq; | |
| d.getElementsByName("LA"+i)[0].value = v.ledma; | |
| d.getElementsByName("MA"+i)[0].value = v.maxpwr; | |
| }); | |
| d.getElementsByName("PR")[0].checked = l.prl | 0; | |
| d.getElementsByName("LD")[0].checked = l.ld; | |
| d.getElementsByName("MA")[0].value = l.maxpwr; | |
| d.getElementsByName("ABL")[0].checked = l.maxpwr > 0; | |
| } | |
| if(c.hw.com) { | |
| resetCOM(); | |
| c.hw.com.forEach(e => { | |
| addCOM(e.start, e.len, e.order); | |
| }); | |
| } | |
| let b = c.hw.btn; | |
| if (b) { | |
| if (Array.isArray(b.ins)) gId("btns").innerHTML = ""; | |
| b.ins.forEach((v,i,a)=>{ | |
| addBtn(i,v.pin[0],v.type); | |
| }); | |
| d.getElementsByName("TT")[0].value = b.tt; | |
| } | |
| let ir = c.hw.ir; | |
| if (ir) { | |
| d.getElementsByName("IR")[0].value = ir.pin; | |
| d.getElementsByName("IT")[0].value = ir.type; | |
| } | |
| let rl = c.hw.relay; | |
| if (rl) { | |
| d.getElementsByName("RL")[0].value = rl.pin; | |
| d.getElementsByName("RM")[0].checked = rl.rev; | |
| d.getElementsByName("RO")[0].checked = rl.odrain; | |
| } |
wled00/bus_manager.cpp
Outdated
| unsigned numPins = is2Pin(_type) + 1; | ||
| if (pinArray) for (unsigned i = 0; i < numPins; i++) pinArray[i] = _pins[i]; | ||
| return numPins; | ||
| } | ||
|
|
||
| size_t BusDigital::getBusSize() const { | ||
| return sizeof(BusDigital) + (isOk() ? PolyBus::getDataSize(_busPtr, _iType) + (_data ? _len * getNumberOfChannels() : 0) : 0); |
There was a problem hiding this comment.
PolyBus::getDataSize() not found.
The build fails because 'getDataSize' is not a member of 'PolyBus'. Confirm if there's a replacement method or remove this call.
Possible fix:
-return sizeof(BusDigital) + (isOk() ? PolyBus::getDataSize(_busPtr, _iType) + (_data ? _len * getNumberOfChannels() : 0) : 0);
+return sizeof(BusDigital) + (isOk() ? (_data ? _len * getNumberOfChannels() : 0) : 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.
| return sizeof(BusDigital) + (isOk() ? PolyBus::getDataSize(_busPtr, _iType) + (_data ? _len * getNumberOfChannels() : 0) : 0); | |
| return sizeof(BusDigital) + (isOk() ? (_data ? _len * getNumberOfChannels() : 0) : 0); |
🧰 Tools
🪛 GitHub Actions: WLED CI
[error] 387-387: 'getDataSize' is not a member of 'PolyBus'.
wled00/bus_manager.cpp
Outdated
| if (Bus::isVirtual(type)) { | ||
| return sizeof(BusNetwork) + (count * Bus::getNumberOfChannels(type)); | ||
| } else if (Bus::isDigital(type)) { | ||
| return sizeof(BusDigital) + PolyBus::memUsage(count + skipAmount, PolyBus::getI(type, pins, nr)) + doubleBuffer * (count + skipAmount) * Bus::getNumberOfChannels(type); |
There was a problem hiding this comment.
'memUsage' is not a member of 'PolyBus', invalid conversion.
The function call to PolyBus::memUsage(...) doesn't exist, and "pins" might be read-only. Confirm the correct approach or if a different method is needed.
- return sizeof(BusDigital) + PolyBus::memUsage(count + skipAmount, PolyBus::getI(type, pins, nr)) ...
+ // Possibly remove or replace with a valid memory usage function
+ return sizeof(BusDigital) + (doubleBuffer ? (count + skipAmount) * Bus::getNumberOfChannels(type) : 0);Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: WLED CI
[error] 810-810: 'memUsage' is not a member of 'PolyBus'.
[error] 810-810: Invalid conversion from 'const uint8_t*' to 'uint8_t*'.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/data/settings_leds.htm (1)
618-623: 🛠️ Refactor suggestionUpdated configuration loading to support more buses.
The configuration loading has been modified to support up to 36 buses by changing the loop limit from
maxB + maxVto a fixed value of 36. This aligns with the other changes in the file.However, there's no validation to ensure the loaded configuration is compatible with the current device's capabilities.
Add validation for loaded configurations:
let c = JSON.parse(lines); +// Validate configuration +function validateConfig(config) { + if (!config.hw?.led?.ins) return false; + const totalBuses = config.hw.led.ins.length; + if (totalBuses > 36) return false; + // Add more validation as needed + return true; +} + if (c.hw) { + if (!validateConfig(c)) { + alert("Invalid configuration!"); + return; + } if (c.hw.led) {
🧹 Nitpick comments (1)
wled00/bus_manager.h (1)
428-433: Converted BusManager from class to namespace with externalized state.This significant architectural change converts BusManager from a class with static methods to a namespace with instance variables and methods. The use of
std::vector<std::unique_ptr<Bus>>instead of raw pointers is a major improvement for memory management and safety.Consider documenting the rationale for this architectural change with a comment block at the beginning of the namespace:
namespace BusManager { +/* + * BusManager namespace manages bus instances and their lifecycle. + * This was converted from a class to a namespace to support up to 36 buses, + * using std::vector<std::unique_ptr<Bus>> for improved memory management. + */ extern std::vector<std::unique_ptr<Bus>> busses; //extern std::vector<Bus*> busses; extern uint16_t _gMilliAmpsUsed; extern uint16_t _gMilliAmpsMax;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
wled00/FX_fcn.cpp(7 hunks)wled00/bus_manager.cpp(22 hunks)wled00/bus_manager.h(9 hunks)wled00/cfg.cpp(4 hunks)wled00/const.h(3 hunks)wled00/data/settings_leds.htm(5 hunks)wled00/set.cpp(3 hunks)wled00/wled.h(1 hunks)wled00/xml.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- wled00/xml.cpp
- wled00/set.cpp
- wled00/cfg.cpp
- wled00/const.h
- wled00/bus_manager.cpp
🔇 Additional comments (26)
wled00/wled.h (1)
897-902: Improved resource management by using std::vector for BusConfigThe change from a static array to
std::vector<BusConfig>forbusConfigsprovides better flexibility and memory management. This will allow for dynamic sizing of bus configurations, supporting the PR's goal of handling up to 36 buses.wled00/FX_fcn.cpp (5)
1141-1143: Improved pointer safety with const Bus pointer and simplified validity checkThe change from
Bus *bustoconst Bus *busenforces const correctness, preventing accidental modification of the bus object. The validity check is also simplified into a single condition, making the code cleaner and more efficient.
1747-1748: Enhanced const correctness in hasRGBWBus methodUsing
const Bus *businstead ofBus *busimproves code safety by preventing unintended modification of the bus object. The simplified validity check also makes the code more readable.
1757-1758: Consistent const correctness applied to hasCCTBus methodSame pattern as the previous methods - using const pointer and simplified validity check. This consistent approach across the codebase improves maintainability and safety.
1811-1816: Improved safety in makeAutoSegments with const Bus pointerThe conversion to
const Bus *busand simplified validity checks maintains consistency with the other methods. This modification ensures that bus pointers cannot be modified inadvertently during the segment creation process.
1905-1906: Completed const correctness pattern in checkSegmentAlignmentFinal application of the same pattern, ensuring that all methods that interact with Bus pointers use const references and consistent validity checks.
wled00/data/settings_leds.htm (8)
45-46: Updated bus limits to support more buses on ESP32 variants.The limits for maximum buses and digital channels have been appropriately updated to accommodate more hardware configurations:
- Maximum buses for ESP32 increased from 19 to 20
- Maximum digital channels increased from 16 to 17
48-48: Increased minimum virtual buses for better device support.Virtual bus minimums have been increased from 3/4 to 4/6 for different ESP variants, improving flexibility for complex LED setups.
425-426: Expanded bus limit check to support up to 36 buses.The condition for adding LEDs has been changed from checking against a calculated limit (
maxB + maxV) to a fixed limit of 36, aligning with the PR's main objective of supporting up to 36 buses. This change properly accommodates the new bus limit implementation.
499-499: Updated display logic comment to match new bus limit.The comment has been updated to reflect that the "+" button display logic is now based on a fixed limit of 36 buses rather than a calculated value (
maxB + maxV).
624-639: Enhanced configuration parsing with additional properties.The code now sets a more comprehensive set of LED configuration properties, including color order, maximum power, and LED current. This provides better compatibility with the expanded bus capabilities.
640-644: Added support for parallel outputs and ABL settings.The parser now correctly handles the parallel output setting (
prl), LED data buffer setting (ld), and maximum power settings (maxpwr), ensuring that all relevant LED settings are properly loaded from the configuration.
651-663: Added support for button and IR sensor configuration.The configuration loading now properly handles button and IR sensor settings from the loaded JSON, improving the completeness of the configuration loading process.
664-673: Added support for relay and light segment settings.The configuration parser now also handles relay and light segment settings, ensuring all hardware-related settings are properly loaded.
wled00/bus_manager.h (12)
14-24: Added C++11 compatible make_unique implementation.This is a good addition for compatibility with environments lacking C++14 support. The implementation correctly uses std::forward to preserve perfect forwarding semantics and is properly guarded by the C++ version check.
Consider adding a comment indicating that this implementation can be removed once C++14 becomes the minimum supported standard:
#if __cplusplus >= 201402L using std::make_unique; #else // Really simple C++11 shim for non-array case; implementation from cppreference.com +// TODO: Remove this once C++14 becomes the minimum supported standard template<class T, class... Args> std::unique_ptr<T> make_unique(Args&&... args) { return std::unique_ptr<T>(new T(std::forward<Args>(args)...)); } #endif
109-109: Enhanced constructor to prevent zero-length bus.Using
std::maxto ensure _len is never less than 1 is a good defensive programming practice that prevents issues with zero-length arrays or invalid indexing.
160-161: Improved type safety with constexpr size_t return types.Changing the return type of
getNumberOfPinsandgetNumberOfChannelsfromuint32_ttosize_tand making them constexpr improves type safety and enables compile-time optimization.
235-235: Simplified BusDigital constructor by removing ColorOrderMap dependency.Removing the ColorOrderMap parameter from the BusDigital constructor indicates that ColorOrderMap is no longer directly tied to BusDigital, which aligns with the PR description about updating ColorOrderMap to use a vector.
297-297: Simplified BusPwm cleanup method.The cleanup method has been simplified to just call deallocatePins(), removing unnecessary null pointer assignments. This is appropriate as long as the memory management pattern ensures that the pointer is never accessed after cleanup.
324-324: Simplified BusOnOff cleanup method.Similar to the BusPwm changes, the cleanup method has been simplified to just deallocate the pin without additional null pointer assignments. This is appropriate as long as the deallocatePin method handles null checks internally.
375-375: Enhanced BusConfig constructor to prevent zero-length bus.Similar to the Bus constructor change, using
std::maxto ensure count is never less than 1 is a good defensive programming practice.
438-442: Enhanced getNumVirtualBusses with range-based for loop.The implementation of getNumVirtualBusses using a range-based for loop and leveraging the unique_ptr container is more concise and safer than traditional indexing.
444-448: Improved current management with inline functions.The inline accessor functions for memory usage and current reporting are appropriate for these simple operations. The commented-out ablMilliampsMax implementation shows a transition from calculating the sum dynamically to using a cached value, which is a good optimization.
460-471: Optimized pixel operations with gnu::hot attribute.The use of the
[[gnu::hot]]attribute on frequently called pixel operations is a good optimization hint for the compiler. The implementation using range-based for loops on the unique_ptr vector is also cleaner and safer.
474-478: Added new utility function getTotalLength.This utility function provides a clean way to get the total length of all buses, with an option to exclude virtual buses. This is a valuable addition for code that needs to differentiate between physical and virtual LEDs.
479-480: Added utilities for LED types and color order mapping.These two new functions provide important utilities for accessing LED type information and color order mapping, which aligns with the PR's objective of improving color order map management.
Add ability to have up to 36 (theoretical limit) buses, physical or virtual.
Stuff also done:
WARNING web server may not support that many variables when creating buses so actual limit may be less.
Many thanks to @willmmiles @TripleWhy and @PaoloTK for helpful comments and suggestions.
Summary by CodeRabbit
• New Features
- Expanded support for LED configurations on select hardware, enabling increased digital channel capacity and more flexible setups.
- Enhanced debugging output for bus configurations, improving troubleshooting capabilities.
• Bug Fixes
- Enhanced error checking and validation during hardware bus initialization, reducing the risk of improper configurations and runtime issues.
• Refactor
- Streamlined configuration processing and resource management, leading to improved system stability, performance, and maintainability.
- Improved memory management through the use of modern C++ features, enhancing code safety and clarity.