adding dynamic update of LED type dropdown #5014
Conversation
WalkthroughReplaced per-LT inline type restriction logic with a centralized Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
DedeHai
changed the title
|
Looks like a good solution. 👍 |
fixed bug that allowed to change to 2-pin digital and then back to 1-pin digital.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/data/settings_leds.htm (1)
821-821: Consider improving readability of the analog pin selector.The selector construction is functionally correct but dense. The calculation
maxA - analogB + (isPWM(curType)?numPins(curType):0) + 1and use of'A'.repeat()make the logic hard to follow at a glance.Consider adding a comment or extracting the calculation:
-disable(`option[data-type^="${'A'.repeat(maxA - analogB + (isPWM(curType)?numPins(curType):0) + 1)}"]`); +// Disable analog types that would exceed available pins +const availablePins = maxA - analogB + (isPWM(curType) ? numPins(curType) : 0); +const minPinsToDisable = availablePins + 1; +disable(`option[data-type^="${'A'.repeat(minPinsToDisable)}"]`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/settings_leds.htm(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/settings_leds.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/settings_leds.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_leds.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (3)
wled00/data/settings_leds.htm (3)
436-437: Good placement following reviewer feedback.The call to
updateTypeDropdowns()at the end ofUI()follows the reviewer's recommendation to call it once centrally rather than using event listeners.
798-823: Well-structured centralized approach.The two-pass strategy (counting current usage, then updating each dropdown) is sound and correctly handles the interdependencies between bus configurations.
808-808: Verify that virtual bus limiting is intentionally omitted.The
virtBcount is computed but never used to disable virtual bus types. This appears intentional based on comments elsewhere (line 454 mentions virtual buses are no longer limited), but please confirm that virtual buses truly have no restrictions.
wled00/data/settings_leds.htm
Outdated
| 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); |
There was a problem hiding this comment.
🧩 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
fiLength 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 -10Length 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 -5Length 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 -100Length 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 -20Length 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 -10Length 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 -100Length 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 -30Length 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.htmLength 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 -30Length 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 -10Length 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 815Length 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 -5Length 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 -20Length 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.htmLength 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.htmLength 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 -20Length 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 -30Length 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 -40Length 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 -20Length 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 -15Length 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 -40Length 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 -A3Length 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 -15Length 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.
There was a problem hiding this comment.
@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 |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
S2 : 5/12
S3 : 4/12
32 : 9/16
These are supported configurations. First number is with parallel I2S disabled/unchecked.
There was a problem hiding this comment.
The formula above is correct.
|
Tested this and does not work as expected. |
|
I've come up with the solution: LTs.forEach((sel,i) => {
const last = i == LTs.length-1;
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);
// disallow adding more of a type that has reached its limit
if (digitalB > maxDB && (!isDig(curType) || last)) disable('option[data-type="D"]');
if (twopinB > 2 && (!isD2P(curType) || last)) disable('option[data-type="2P"]');
disable(`option[data-type^="${'A'.repeat(maxA - analogB + (isPWM(curType)?numPins(curType):0) + 1)}"]`);
// we need to handle last added bus separately to properly select available type
if (last && sel.options[0].disabled) sel.selectedIndex = sel.querySelector('option:not(:disabled)').index;
}); |
wled00/data/settings_leds.htm
Outdated
| 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); |
There was a problem hiding this comment.
The formula above is correct.
this is much cleaner than the version from my last commit and achieves the same. |
this actuall does not work properly, my version with setting to a "virtual bus" does work correctly. |
|
I initially went with virtual bus too but found it annoying. |
|
your version worked in my tests when removing: |
|
@netmindz once this is tested a bit more, it can be backported to 0.15 |
|
@DedeHai remove your last commit and move |
|
I've spent a better part of yesterday solving the issue without much succes while trying to keep existing behaviour. What I did manage to get working in the end is something like @DedeHai did with selecting non-demanding On/Off output when creating a new output. This output has no restrictions (apart from available GPIO) so can be selected at any time and allows AI generated code to function unchanged. However the place for The unfortunate consequence is that On/Off bus is selected as default when adding a new bus instead of usual 1st available type. |
2 participants
removing restriction to only allow changing of last bus by dynamically updating the dropdown menus
@blazoncek is this a valid fix or did I misunderstand the issue?
Summary by CodeRabbit