-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Improve memory-constrained bus creation with user configuration preservation and UI error handling #4937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
@copilot also: is it possible to catch null / missing element error in the index.js (there are probably other cases) and handle them correctly with some default values so the UI loads correctly and does not throw errors but would throw a warning? |
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
I've implemented all your suggestions in commit 2dfea5c. The improvements include: 1. Bus Truncation with User Config Preservation:
2. Better Fallback Strategy:
3. UI Error Handling:
Key Benefits:
The system now honors user intent as much as possible while staying within ESP8266 memory limits. |
@copilot please also address other errors in the UI. for example: if the segment list is empty, the UI will also fail to load with the error "TypeError: Cannot read properties of null (reading 'classList')" |
Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
I've addressed the additional UI errors for empty segment lists in commit 1b19c35. The improvements include: Comprehensive Error Handling:
Key Changes:
Testing: The UI now remains functional even when segment lists are completely empty, allowing users to reconfigure their setup without JavaScript crashes. |
for (const auto &bus : busConfigs) { | ||
mem += bus.memUsage(Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type) ? digitalCount++ : 0); // includes global buffer | ||
if (mem <= MAX_LED_MEMORY) { | ||
if (BusManager::add(bus) == -1) break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot instead of adding a lot of code:
if mem > MAX_LED_MEMORY just set the value for count in busconfig to default length and add it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in f75b13a. Replaced the complex binary search logic with the simple approach you suggested - when memory exceeds MAX_LED_MEMORY, the bus count is set to DEFAULT_LED_COUNT (30) and the bus is added with all user configuration preserved (pins, type, etc.). This maintains support for multiple buses while greatly simplifying the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot it should not increase size, so use min(bus.count, DEFAULT_LED_COUNT)
also why did you change
for (const auto &bus : busConfigs) {
into
for (auto bus : busConfigs) {
please explain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8590538. Changed to use min(bus.count, DEFAULT_LED_COUNT)
to prevent increasing the bus size beyond user configuration.
Regarding the loop change from const auto &bus
to auto bus
: This was necessary because we need to modify bus.count
when memory limits are exceeded. With const auto &bus
, the bus configuration is a const reference and cannot be modified. Using auto bus
creates a copy that we can safely modify without affecting the original configuration in busConfigs
.
…rch with simple default count fallback Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
…tion Co-authored-by: DedeHai <6280424+DedeHai@users.noreply.github.com>
I'm not a fan at all of this solution, it does not match what you detailed as the behaviour in the issue. I think we chalk this up as nice try copilot, but just write write the proposed change ourselves |
Root Cause
In
WS2812FX::finalizeInit()
, when bus configurations exceedMAX_LED_MEMORY
on ESP8266, WLED 0.16 fails to create any buses, resulting in two critical issues:Solution
Simplified Memory-Aware Bus Creation
Replaced complex bus handling with simple approach that preserves user configuration:
UI Error Handling
Added defensive programming to prevent JavaScript crashes:
Testing Results
Key Improvements
This provides graceful degradation that honors user intent as much as possible while staying within hardware constraints, instead of complete system failure when upgrading between versions with different memory calculations.
Fixes #4936.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.