"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.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 std::vector with unique pointers. In addition, loop limits in configuration and LED settings have been standardized, method return types have been updated from uint8_t to size_t where appropriate, and several functions have been refactored for clearer, more concise control flow. 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
🪧 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; | |
| } |
| 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'.
| 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*'.
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.
• 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.