Skip to content

adding dynamic update of LED type dropdown #5014

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

adding dynamic update of LED type dropdown #5014

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
025be3b
adding dynamic update of LED type dropdown, remove restriction
DedeHai Oct 21, 2025
0e0e1ac
call updateTypeDropdowns() at end of UI(), bugfix
DedeHai Oct 22, 2025
7ce356a
add clarifying comments
DedeHai Oct 22, 2025
d89b7f9
bugfix: dont allow additional digital buses
DedeHai Oct 23, 2025
d0fdaf2
move updateTypeDropdowns()
DedeHai Oct 26, 2025
31c3356
add comment as suggested by the rabbit
DedeHai Oct 26, 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
47 changes: 28 additions & 19 deletions wled00/data/settings_leds.htm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,6 @@
let dC = 0; // count of digital buses (for parallel I2S)
let LTs = d.Sf.querySelectorAll("#mLC select[name^=LT]");
LTs.forEach((s,i)=>{
if (i < LTs.length-1) s.disabled = true; // prevent changing type (as we can't update options)
// is the field a LED type?
var n = s.name.substring(2,3); // bus number (0-Z)
var t = parseInt(s.value);
Expand Down Expand Up @@ -434,6 +433,8 @@
gId('fpsNone').style.display = (d.Sf.FR.value == 0) ? 'block':'none';
gId('fpsWarn').style.display = (d.Sf.FR.value == 0) || (d.Sf.FR.value >= 80) ? 'block':'none';
gId('fpsHigh').style.display = (d.Sf.FR.value >= 80) ? 'block':'none';
// restrict bus types in dropdowns to max allowed digital/analog buses
updateTypeDropdowns();
}
function lastEnd(i) {
if (i-- < 1) return 0;
Expand All @@ -447,17 +448,8 @@
{
var o = gEBCN("iST");
var i = o.length;
let disable = (sel,opt) => { sel.querySelectorAll(opt).forEach((o)=>{o.disabled=true;}); }

var f = gId("mLC");
let digitalB = 0, analogB = 0, twopinB = 0, virtB = 0;
f.querySelectorAll("select[name^=LT]").forEach((s)=>{
let t = s.value;
if (isDig(t) && !isD2P(t)) digitalB++;
if (isD2P(t)) twopinB++;
if (isPWM(t)) analogB += numPins(t); // each GPIO is assigned to a channel
if (isVir(t)) virtB++;
});

if ((n==1 && i>=36) || (n==-1 && i==0)) return; // used to be i>=maxB+maxV when virtual buses were limited (now :"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ")
var s = chrID(i);
Expand Down Expand Up @@ -522,18 +514,9 @@
}
});
enLA(d.Sf["LAsel"+s],s); // update LED mA
// disable inappropriate LED types
let sel = d.getElementsByName("LT"+s)[0];
// 32 & S2 supports mono I2S as well as parallel so we need to take that into account; S3 only supports parallel
let maxDB = maxD - (is32() || isS2() || isS3() ? (!d.Sf["PR"].checked)*8 - (!isS3()) : 0); // adjust max digital buses if parallel I2S is not used
if (digitalB >= maxDB) disable(sel,'option[data-type="D"]'); // NOTE: see isDig()
if (twopinB >= 2) disable(sel,'option[data-type="2P"]'); // NOTE: see isD2P() (we will only allow 2 2pin buses)
disable(sel,`option[data-type^="${'A'.repeat(maxA-analogB+1)}"]`); // NOTE: see isPWM()
sel.selectedIndex = sel.querySelector('option:not(:disabled)').index;
}
if (n==-1) {
o[--i].remove();--i;
o[i].querySelector("[name^=LT]").disabled = false;
}

gId("+").style.display = (i<35) ? "inline":"none"; // was maxB+maxV-1 when virtual buses were limited (now :"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ")
Expand Down Expand Up @@ -812,6 +795,32 @@
}
return opt;
}
// dynamically enforce bus type availability based on current usage
function updateTypeDropdowns() {
let LTs = d.Sf.querySelectorAll("#mLC select[name^=LT]");
let digitalB = 0, analogB = 0, twopinB = 0, virtB = 0;
// count currently used buses
LTs.forEach(sel => {
let t = parseInt(sel.value);
if (isDig(t) && !isD2P(t)) digitalB++;
if (isPWM(t)) analogB += numPins(t);
if (isD2P(t)) twopinB++;
if (isVir(t)) virtB++;
});
// enable/disable type options according to limits in dropdowns
LTs.forEach(sel => {
const curType = parseInt(sel.value);
const disable = (q) => sel.querySelectorAll(q).forEach(o => o.disabled = true);
const enable = (q) => sel.querySelectorAll(q).forEach(o => o.disabled = false);
enable('option'); // reset all first
// max digital count
let maxDB = maxD - ((is32() || isS2() || isS3()) ? (!d.Sf["PR"].checked) * 8 - (!isS3()) : 0);
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify the maxDB calculation logic.

The arithmetic (!d.Sf["PR"].checked) * 8 - (!isS3()) produces unexpected results. When parallel I2S is enabled (PR.checked = true) and not on S3, this evaluates to 0 * 8 - 1 = -1, which increases maxDB to maxD + 1. This seems incorrect unless there's a specific hardware reason for allowing more digital buses in that configuration.

Additionally, using boolean-to-number coercion ((!condition) * 8) reduces readability compared to an explicit conditional.

Consider refactoring for clarity and correctness:

-let maxDB = maxD - ((is32() || isS2() || isS3()) ? (!d.Sf["PR"].checked) * 8 - (!isS3()) : 0);
+let reduction = 0;
+if (is32() || isS2() || isS3()) {
+	if (!d.Sf["PR"].checked) reduction += 8;  // parallel I2S disabled
+	if (!isS3()) reduction += 1;  // additional reduction for non-S3
+}
+let maxDB = maxD - reduction;

Please verify this matches the intended hardware constraints.

I need to investigate the actual code and its context to verify the review comment. Let me examine the repository.

🏁 Script executed:

#!/bin/bash
# Locate and examine the settings file around line 817
if [ -f "wled00/data/settings_leds.htm" ]; then
  echo "=== Lines 810-825 of settings_leds.htm ==="
  sed -n '810,825p' wled00/data/settings_leds.htm
  echo ""
  echo "=== Search for maxDB and maxD definitions/usage ==="
  grep -n "maxDB\|maxD" wled00/data/settings_leds.htm | head -20
else
  echo "File not found: wled00/data/settings_leds.htm"
  find . -name "settings_leds.htm" -type f 2>/dev/null
fi

Length of output: 1888

🏁 Script executed:

#!/bin/bash
# Search for documentation or comments about digital bus limits
echo "=== Search for LED bus, digital, or maxDB context ==="
grep -r "maxD\|maxDB\|digital.*bus\|parallel.*I2S" wled00/data/*.htm wled00/data/*.js 2>/dev/null | grep -i "comment\|//\|/\*" | head -10

Length of output: 1353

🏁 Script executed:

#!/bin/bash
# Look for any firmware comments or constants related to bus/LED limits
echo "=== Checking for related documentation ==="
find wled00 -name "*.md" -o -name "*.txt" -o -name "*README*" | xargs grep -l "bus\|LED\|maxD" 2>/dev/null | head -5

Length of output: 82

🏁 Script executed:

#!/bin/bash
# Check git log for when this line was added and any commit messages
echo "=== Recent changes to settings_leds.htm around line 817 ==="
git log -p --follow -S "maxDB" -- "wled00/data/settings_leds.htm" 2>/dev/null | head -100

Length of output: 59

🏁 Script executed:

#!/bin/bash
# Search more broadly for the maxD/maxDB logic to understand intent
echo "=== Context search for PR.checked and isS3 patterns ==="
grep -n "PR.checked\|isS3\|is32\|isS2" wled00/data/settings_leds.htm | head -20

Length of output: 796

🏁 Script executed:

#!/bin/bash
# Look for function definitions of is32, isS2, isS3
echo "=== Function definitions for board detection ==="
grep -n "function\s\+is32\|function\s\+isS2\|function\s\+isS3\|is32\s*=\|isS2\s*=\|isS3\s*=" wled00/data/settings_leds.htm | head -10

Length of output: 317

🏁 Script executed:

#!/bin/bash
# Examine broader context around line 817 (200 lines before and after)
echo "=== Broader context: lines 750-850 ==="
sed -n '750,850p' wled00/data/settings_leds.htm | cat -n | tail -150 | head -100

Length of output: 5155

🏁 Script executed:

#!/bin/bash
# Check if there are test files or comments explaining the maxDB formula
grep -r "maxD\|maxDB" wled00 --include="*.js" --include="*.htm" -B3 -A3 2>/dev/null | grep -E "(maxD|maxDB|//|comment)" | head -30

Length of output: 1837

🏁 Script executed:

#!/bin/bash
# Look at the file size and overall structure
echo "=== File structure check ==="
wc -l wled00/data/settings_leds.htm
echo ""
echo "=== Checking for any inline comments near maxDB calculation ==="
sed -n '800,830p' wled00/data/settings_leds.htm

Length of output: 1566

🏁 Script executed:

#!/bin/bash
# Search for any explanation of what PR means in context
grep -n "Sf\[.PR.\]\|\"PR\"" wled00/data/settings_leds.htm -B2 -A2 | head -30

Length of output: 1273

🏁 Script executed:

#!/bin/bash
# Look for pattern of boolean arithmetic to understand coding style
echo "=== Other boolean arithmetic in file ==="
grep -n "!\s*\w\+.*\*\s*[0-9]" wled00/data/settings_leds.htm | head -10

Length of output: 143

🏁 Script executed:

#!/bin/bash
# Final check: extract the exact line 817 with full context
echo "=== Exact line 817 and surrounding lines ==="
sed -n '815,820p' wled00/data/settings_leds.htm | nl -v 815

Length of output: 85

🏁 Script executed:

#!/bin/bash
# Check for any related issues or pull requests mentioning this calculation
echo "=== Search for hardware/LED bus documentation ==="
find wled00 -type f \( -name "*.cpp" -o -name "*.h" \) -exec grep -l "maxD\|digital.*bus" {} \; 2>/dev/null | head -5

Length of output: 165

🏁 Script executed:

#!/bin/bash
# One more check - look for the actual values of maxD to understand the range
grep -n "maxD\s*=\|let maxD\|var maxD\|const maxD" wled00/data/settings_leds.htm -A2 | head -20

Length of output: 1216

🏁 Script executed:

#!/bin/bash
# Verify the exact calculation in context - get 30 lines before line 817
sed -n '787,820p' wled00/data/settings_leds.htm

Length of output: 1474

🏁 Script executed:

#!/bin/bash
# Search for any validation or constraints on maxDB after calculation
sed -n '815,835p' wled00/data/settings_leds.htm

Length of output: 1039

🏁 Script executed:

#!/bin/bash
# Check if there's documentation in HTML comments
grep -n "<!--\|-->" wled00/data/settings_leds.htm | grep -A5 -B5 "bus\|maxD\|LED" | head -20

Length of output: 0

I need to check the C++ firmware source to understand the hardware constraints behind this formula.

🏁 Script executed:

#!/bin/bash
# Look for parallel I2S constraints in C++ headers
echo "=== Checking const.h for parallel I2S and digital channel info ==="
grep -n "PARALLEL\|I2S\|LED_COUNT\|MAX.*DIG\|DMA" wled00/const.h | head -30

Length of output: 1215

🏁 Script executed:

#!/bin/bash
# Search bus_manager.cpp for digital bus constraints
echo "=== bus_manager.cpp parallel I2S references ==="
grep -n "parallel\|PR\|maxD\|digital" wled00/bus_manager.cpp -i | head -40

Length of output: 3022

🏁 Script executed:

#!/bin/bash
# Check for comments or logic around S2/S3 digital channel limits
echo "=== Searching for S2/S3 specific digital bus constraints ==="
grep -rn "S2\|S3\|ESP32-S" wled00/const.h wled00/bus_manager.cpp 2>/dev/null | grep -i "dig\|chan\|bus" | head -20

Length of output: 2545

🏁 Script executed:

#!/bin/bash
# Look for any test or documentation about PR (parallel I2S) behavior
echo "=== Search for PR or parallel I2S documentation ==="
grep -rn "parallel.*I2S\|PR.*checked" wled00 --include="*.cpp" --include="*.h" -i | head -15

Length of output: 1600

🏁 Script executed:

#!/bin/bash
# Check if there's any recent commit or comment explaining the maxDB logic
echo "=== Checking for explanation in code comments around digital channels ==="
grep -rn "maxDB\|max.*digital\|digital.*count" wled00 --include="*.cpp" --include="*.h" -B2 -A2 | head -40

Length of output: 1501

🏁 Script executed:

#!/bin/bash
# Look at how maxDB value affects actual bus allocation
echo "=== Searching where maxDB variable/concept is used in logic ==="
grep -n "maxDB" wled00/data/settings_leds.htm -B3 -A3

Length of output: 707

🏁 Script executed:

#!/bin/bash
# Check if there are any GitHub issues or PR comments mentioning this arithmetic
echo "=== Verify ESP32-S2 vs S3 digital I2S capability differences ==="
grep -rn "channel\|digital.*output" wled00 --include="*.h" | grep -E "S2|S3|16|12" | head -15

Length of output: 1864

Fix the maxDB calculation to prevent allowing more digital buses than hardware supports.

The formula (!d.Sf["PR"].checked) * 8 - (!isS3()) produces incorrect results when parallel I2S is enabled. On ESP32 and S2 boards with PR checked, this evaluates to -1, making maxDB = maxD + 1. This allows the UI to show capacity for more digital buses than the hardware maximum.

The issue is inconsistent: S3 correctly returns maxD when PR is checked, but ESP32 and S2 incorrectly exceed their hardware limits.

Likely intended logic: reduce capacity by 8 when NOT using parallel I2S (when PR is unchecked). The subtraction of (!isS3()) appears to be attempting a per-board adjustment but uses subtraction instead of addition, inverting the logic.

Replace with:

-let maxDB = maxD - ((is32() || isS2() || isS3()) ? (!d.Sf["PR"].checked) * 8 - (!isS3()) : 0);
+let maxDB = maxD - ((is32() || isS2() || isS3()) ? ((!d.Sf["PR"].checked) * 8) + (!isS3()) : 0);

Or refactor for clarity:

-let maxDB = maxD - ((is32() || isS2() || isS3()) ? (!d.Sf["PR"].checked) * 8 - (!isS3()) : 0);
+let reduction = 0;
+if (is32() || isS2() || isS3()) {
+	if (!d.Sf["PR"].checked) reduction += 8;
+	if (!isS3()) reduction += 1;
+}
+let maxDB = maxD - reduction;

Verify that (!isS3()) reduction is intentional for S2/ESP32 when parallel I2S is enabled.

🤖 Prompt for AI Agents 
wled00/data/settings_leds.htm around line 817: the current expression makes
maxDB larger when PR is checked on ESP32/S2 because (!d.Sf["PR"].checked) * 8 -
(!isS3()) can evaluate to -1; replace that sub-expression with a clear
conditional that subtracts 8 only when PR is unchecked (e.g. use
((!d.Sf["PR"].checked) ? 0 : 8)), and if any additional per-board adjustment for
non-S3 targets is required, apply it as an additive/subtractive term explicitly
(not by subtracting a boolean) and verify the intended (!isS3()) reduction for
ESP32/S2.

Copy link
Collaborator Author

@DedeHai DedeHai Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blazoncek this line is unchanged from previous code. is this the indended behaviour or is the rabbit right?

Chip PR.checked Explanation Effective subtraction Result
ESP32/S2 false (mono I²S) Mono mode uses 8 busses 8 - 1 = 7 maxDB = maxD - 7
ESP32/S2 true (parallel) Parallel mode → don’t reserve I²S busses 0 - 1 = -1 maxDB = maxD + 1
ESP32-S3 false (mono I²S) Mono mode uses 8 busses 8 - 0 = 8 maxDB = maxD - 8
ESP32-S3 true (parallel) Only parallel mode available 0 - 0 = 0 maxDB = maxD

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Collaborator

@blazoncek blazoncek Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S2 : 5/12
S3 : 4/12
32 : 9/16
These are supported configurations. First number is with parallel I2S disabled/unchecked.

Copy link
Collaborator

@blazoncek blazoncek Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formula above is correct.

// disallow adding more of a type that has reached its limit
if (digitalB >= maxDB && !(isDig(curType) && !isD2P(curType))) disable('option[data-type="D"]');
if (twopinB >= 2 && !isD2P(curType)) disable('option[data-type="2P"]');
disable(`option[data-type^="${'A'.repeat(maxA - analogB + (isPWM(curType)?numPins(curType):0) + 1)}"]`);
});
}
</script>
<style>@import url("style.css");</style>
</head>
Expand Down