Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Improve memory-constrained bus creation with user configuration preservation and UI error handling #4937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c0c875
Initial plan
Copilot Sep 15, 2025
a441ee8
Implement bus creation fallback when memory limit exceeded
Copilot Sep 15, 2025
2dfea5c
Improve bus creation with memory-aware truncation and UI error handling
Copilot Sep 15, 2025
1b19c35
Add comprehensive UI error handling for empty segment lists
Copilot Sep 15, 2025
f75b13a
Simplify bus creation logic as requested - replace complex binary sea…
Copilot Sep 15, 2025
8590538
Use min() to prevent increasing bus count and maintain user configura…
Copilot Sep 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions wled00/FX_fcn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
#include "FXparticleSystem.h" // TODO: better define the required function (mem service) in FX.h?
#include "palettes.h"

#ifndef DEFAULT_LED_COLOR_ORDER
#define DEFAULT_LED_COLOR_ORDER COL_ORDER_GRB //default to GRB
#endif

/*
Custom per-LED mapping has moved!

Expand Down Expand Up @@ -1173,12 +1177,22 @@ void WS2812FX::finalizeInit() {

// create buses/outputs
unsigned mem = 0;
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;
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

} else DEBUG_PRINTF_P(PSTR("Out of LED memory! Bus %d (%d) #%u not created."), (int)bus.type, (int)bus.count, digitalCount);
for (auto bus : busConfigs) {
// Calculate what this bus would use with its current configuration
unsigned busMemUsage = bus.memUsage(Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type) ? digitalCount : 0);

// If memory exceeds limit, set count to minimum of current count and default length
if (mem + busMemUsage > MAX_LED_MEMORY) {
bus.count = min(bus.count, DEFAULT_LED_COUNT);
DEBUG_PRINTF_P(PSTR("Bus %d memory usage exceeds limit, setting count to %d\n"), (int)bus.type, bus.count);
}

if (BusManager::add(bus) != -1) {
mem += bus.memUsage(Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type) ? digitalCount : 0);
if (Bus::isDigital(bus.type) && !Bus::is2Pin(bus.type)) digitalCount++;
} else break;
}

busConfigs.clear();
busConfigs.shrink_to_fit();

Expand Down
45 changes: 38 additions & 7 deletions wled00/data/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,9 +668,20 @@ function parseInfo(i) {
if (loc) name = "(L) " + name;
d.title = name;
simplifiedUI = i.simplifiedui;
ledCount = i.leds.count;
// Add safety checks for LED count data to prevent UI crashes
if (i.leds && typeof i.leds.count !== 'undefined') {
ledCount = i.leds.count;
} else {
console.warn('LED count data missing, using fallback value');
ledCount = 30; // Fallback value matching firmware default
}
//syncTglRecv = i.str;
maxSeg = i.leds.maxseg;
if (i.leds && typeof i.leds.maxseg !== 'undefined') {
maxSeg = i.leds.maxseg;
} else {
console.warn('Max segment data missing, using fallback value');
maxSeg = 16; // Reasonable fallback for max segments
}
pmt = i.fs.pmt;
if (pcMode && !i.wifi.ap) gId('edit').classList.remove("hide"); else gId('edit').classList.add("hide");
gId('buttonNodes').style.display = lastinfo.ndc > 0 ? null:"none";
Expand Down Expand Up @@ -912,12 +923,24 @@ function populateSegments(s)
gId(`segr${i}`).classList.add("hide");
}
if (segCount < 2) {
gId(`segd${lSeg}`).classList.add("hide"); // hide delete if only one segment
if (parseInt(gId("seg0bri").value)==255) gId(`segp0`).classList.add("hide");
// Add safety check for segment elements to prevent UI crashes
const segdElement = gId(`segd${lSeg}`);
if (segdElement) segdElement.classList.add("hide"); // hide delete if only one segment
const seg0briElement = gId("seg0bri");
const segp0Element = gId(`segp0`);
if (seg0briElement && segp0Element && parseInt(seg0briElement.value)==255) segp0Element.classList.add("hide");
// hide segment controls if there is only one segment in simplified UI
if (simplifiedUI) gId("segcont").classList.add("hide");
}
if (!isM && !noNewSegs && (cfg.comp.seglen?parseInt(gId(`seg${lSeg}s`).value):0)+parseInt(gId(`seg${lSeg}e`).value)<ledCount) gId(`segr${lSeg}`).classList.remove("hide");
// Add safety checks for segment control elements
const segSElement = gId(`seg${lSeg}s`);
const segEElement = gId(`seg${lSeg}e`);
const segrElement = gId(`segr${lSeg}`);
if (!isM && !noNewSegs && segSElement && segEElement && segrElement) {
const segLen = cfg.comp.seglen ? parseInt(segSElement.value) : 0;
const segEnd = parseInt(segEElement.value);
if (segLen + segEnd < ledCount) segrElement.classList.remove("hide");
}
gId('segutil2').style.display = (segCount > 1) ? "block":"none"; // rsbtn parent

if (Array.isArray(li.maps) && li.maps.length>1) {
Expand Down Expand Up @@ -2253,7 +2276,9 @@ function rptSeg(s)
var rev = gId(`seg${s}rev`).checked;
var mi = gId(`seg${s}mi`).checked;
var sel = gId(`seg${s}sel`).checked;
var pwr = gId(`seg${s}pwr`).classList.contains('act');
// Add safety check for segment power element to prevent UI crashes
const segPwrElement = gId(`seg${s}pwr`);
var pwr = segPwrElement ? segPwrElement.classList.contains('act') : false;
var obj = {"seg": {"id": s, "n": name, "start": start, "stop": (cfg.comp.seglen?start:0)+stop, "rev": rev, "mi": mi, "on": pwr, "bri": parseInt(gId(`seg${s}bri`).value), "sel": sel}};
if (gId(`seg${s}grp`)) {
var grp = parseInt(gId(`seg${s}grp`).value);
Expand Down Expand Up @@ -2380,7 +2405,13 @@ function setGrp(s, g)

function setSegPwr(s)
{
var pwr = gId(`seg${s}pwr`).classList.contains('act');
// Add safety check for segment power element to prevent UI crashes
const segPwrElement = gId(`seg${s}pwr`);
if (!segPwrElement) {
console.warn('Segment power element not found, skipping power toggle');
return;
}
var pwr = segPwrElement.classList.contains('act');
var obj = {"seg": {"id": s, "on": !pwr}};
requestJson(obj);
}
Expand Down