Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralized GPIO/pin utilities were added to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/data/settings_leds.htm`:
- Around line 844-859: pinDropdowns() currently only re-calls makePinSelect()
for IR, RL and button input fields, so LED selects and already-converted button
<select>s never get updated with pInfo.a/pInfo.n loaded from /json/pins; update
pinDropdowns() so after pins are fetched you rebuild or refresh all pin-related
controls by iterating over the LED fields plus all button controls (both inputs
and existing selects under gId("btns") and d.Sf), re-invoking makePinSelect()
for each relevant name (use the same btnPinFlags(t) logic for buttons), reattach
onchange handlers that call UI() and pinUpd(), and finally re-run
d.Sf.querySelectorAll("select.pin").forEach(pinUpd) so pinUpd() receives the new
pInfo for every select; reference functions/ids: pinDropdowns(),
makePinSelect(), pinUpd(), addBtn(), gId("btns"), d.Sf.
- Around line 872-875: The code collecting LED bus pin values is accidentally
including HUB75 dimension fields (L0..L4); update the iteration in
d.Sf.querySelectorAll(".iST input.s[name^=L]") to explicitly exclude HUB75
dimension inputs (e.g. skip when i.name matches /^L[0-4]$/) before pushing
values into pins so only actual GPIO pin entries are considered (retain the
existing checks for i.value and parseInt(i.value) >= 0 and i.max < 255).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9bb318c0-37e4-4130-8b53-052de0b9326a
📒 Files selected for processing (6)
wled00/data/common.jswled00/data/settings_leds.htmwled00/data/settings_pininfo.htmwled00/data/settings_um.htmwled00/set.cppwled00/xml.cpp
Member
|
can we also include the settings_sync page in this PR? |
Collaborator
Author
|
sure can. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
wled00/data/settings_leds.htm (3)
866-870: Minor: Inconsistent type handling for numeric comparison.Line 868 uses
i.value >= 0which relies on implicit string-to-number coercion, while line 884 explicitly usesparseInt(i.value, 10) >= 0. For consistency and clarity, prefer the explicit approach.🔧 Suggested fix
d.Sf.querySelectorAll(".iST input.s[name^=L]").forEach((i) => { - if (i.value !== "" && i.value >= 0) + if (i.value !== "" && parseInt(i.value, 10) >= 0) i.dataset.val = i.value; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_leds.htm` around lines 866 - 870, The comparison uses implicit string-to-number coercion for inputs selected by d.Sf.querySelectorAll(".iST input.s[name^=L]") when setting i.dataset.val; change the check to parse the value explicitly (e.g., use parseInt(i.value, 10) or Number(i.value) and ensure it is not NaN) before comparing to >= 0 so it matches the explicit style used elsewhere and avoids type-coercion bugs when assigning i.dataset.val.
315-328: Minor indentation inconsistency.Lines 316-318 appear to have a leading space before tabs. As per coding guidelines, web files should use tabs for indentation.
🔧 Suggested fix
// convert pin fields so show/hide applies to the correct element type - for (let p=0; p<5; p++) { - let nm2 = "L"+p+n; - let el = d.Sf[nm2]; + for (let p=0; p<5; p++) { + let nm2 = "L"+p+n; + let el = d.Sf[nm2];(Ensure consistent tab indentation throughout the block)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_leds.htm` around lines 315 - 328, The indentation in the for-loop block that converts pin fields is inconsistent (leading spaces before tabs on the lines handling isVir/isHub75 and the INPUT branch); normalize those lines to use tabs only to match the surrounding code style—specifically adjust the lines referencing isVir(t), isHub75(t), unmakePinSelect(nm2) and the INPUT branch that calls makePinSelect(nm2, 1) and sets d.pinUpdPending so all lines use tabs for indentation.
441-461: Consider removing dead code in a follow-up.The commented-out pin conflict coloring block has been replaced by the deferred
pinUpd()approach. While keeping it for reference during development is fine, consider removing it entirely once the new approach is validated to reduce code noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_leds.htm` around lines 441 - 461, Remove the obsolete commented-out pin conflict coloring block and any related unused helper references left only for that logic; specifically delete the entire /* ... */ block that references LC, gN, nList, d.rsvd, d.um_p, d.Sf, d.ro_gpio and the old conflict-coloring checks, since pin conflict handling is now implemented by pinUpd(); ensure no remaining dead-symbol usages of that logic linger (e.g., unused local vars or commented helper calls) so the file only contains the active pinUpd()-based implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/data/settings_leds.htm`:
- Around line 866-870: The comparison uses implicit string-to-number coercion
for inputs selected by d.Sf.querySelectorAll(".iST input.s[name^=L]") when
setting i.dataset.val; change the check to parse the value explicitly (e.g., use
parseInt(i.value, 10) or Number(i.value) and ensure it is not NaN) before
comparing to >= 0 so it matches the explicit style used elsewhere and avoids
type-coercion bugs when assigning i.dataset.val.
- Around line 315-328: The indentation in the for-loop block that converts pin
fields is inconsistent (leading spaces before tabs on the lines handling
isVir/isHub75 and the INPUT branch); normalize those lines to use tabs only to
match the surrounding code style—specifically adjust the lines referencing
isVir(t), isHub75(t), unmakePinSelect(nm2) and the INPUT branch that calls
makePinSelect(nm2, 1) and sets d.pinUpdPending so all lines use tabs for
indentation.
- Around line 441-461: Remove the obsolete commented-out pin conflict coloring
block and any related unused helper references left only for that logic;
specifically delete the entire /* ... */ block that references LC, gN, nList,
d.rsvd, d.um_p, d.Sf, d.ro_gpio and the old conflict-coloring checks, since pin
conflict handling is now implemented by pinUpd(); ensure no remaining
dead-symbol usages of that logic linger (e.g., unused local vars or commented
helper calls) so the file only contains the active pinUpd()-based
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d649a646-35ac-4d6c-a9bd-c4ce5e909026
📒 Files selected for processing (1)
wled00/data/settings_leds.htm
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/data/settings_sync.htm (1)
51-68: Consider restoring owner text when option becomes disabled again.Line 65 clears the owner name when an option is enabled (
o.text = String(p)), but when the option becomes disabled again (e.g., another DMX dropdown selects it), the owner name is not restored. This is a minor cosmetic issue since the dropdown still functions correctly.Also, the flags logic is correct: RX pin (i=0) uses flags=0 to allow read-only pins, while TX and Enable pins (i=1,2) use flags=1 to exclude read-only pins.
💡 Optional fix to restore owner text
function dmxPinUpd() { const vs = dmxNs.map(n => parseInt(gN(n)?.value??-1)); dmxNs.forEach((n,i) => { for (const o of gN(n)?.options??[]) { const p = parseInt(o.value); if (p<0) continue; + const pInfo = d.pinsData?.find(pi=>pi.p===p); // unlock old selected pin, lock newly selected one - o.disabled = (p!==vs[i] && !dmxOwnPins.includes(p) && !!d.pinsData?.find(pi=>pi.p===p)?.a) || vs.some((v,j)=>j!==i&&v===p); - if (!o.disabled) o.text = String(p); // clear owner name i.e. (DMX input) + const usedByOther = p!==vs[i] && !dmxOwnPins.includes(p) && pInfo?.a; + const usedByDmx = vs.some((v,j)=>j!==i&&v===p); + o.disabled = usedByOther || usedByDmx; + o.text = o.disabled && pInfo?.a ? `${p} (${getOwnerName(pInfo.o, pInfo.t, pInfo.n)})` : String(p); } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_sync.htm` around lines 51 - 68, The dropdown option label isn't restored when an option becomes disabled; update dmxPinUpd() so inside the loop where you compute o.disabled you also set o.text based on whether the option is disabled: look up the owner name from d.pinsData (e.g. const owner = d.pinsData?.find(pi => pi.p===p)?.n) and if o.disabled set o.text = owner ?? String(p) (otherwise keep the existing enabled behavior o.text = String(p)); this uses the existing symbols dmxPinUpd, dmxNs, dmxOwnPins and o to restore the owner name when an option returns to disabled state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/wled.h`:
- Around line 472-474: The guard that decides whether to call DMXInput::init()
incorrectly treats the -1 sentinel (promoted to 255 as uint8_t) as a valid pin;
update the check around the DMX init call to explicitly reject the 255 sentinel
(e.g., require rxPin > 0 && rxPin < 255 && enPin > 0 && enPin < 255 && txPin > 0
&& txPin < 255) or change the DMXInput::init() parameter types to signed
(preserve -1) and adjust the guard to check for >= 0; modify the code paths that
call DMXInput::init() and the DMXInput::init() signature accordingly
(referencing dmxInputReceivePin, dmxInputTransmitPin, dmxInputEnablePin and
DMXInput::init).
---
Nitpick comments:
In `@wled00/data/settings_sync.htm`:
- Around line 51-68: The dropdown option label isn't restored when an option
becomes disabled; update dmxPinUpd() so inside the loop where you compute
o.disabled you also set o.text based on whether the option is disabled: look up
the owner name from d.pinsData (e.g. const owner = d.pinsData?.find(pi =>
pi.p===p)?.n) and if o.disabled set o.text = owner ?? String(p) (otherwise keep
the existing enabled behavior o.text = String(p)); this uses the existing
symbols dmxPinUpd, dmxNs, dmxOwnPins and o to restore the owner name when an
option returns to disabled state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8055b1f3-bd9b-4472-b8fd-d3444a934ee9
📒 Files selected for processing (3)
wled00/data/settings_sync.htmwled00/pin_manager.hwled00/wled.h
✅ Files skipped from review due to trivial changes (1)
- wled00/pin_manager.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/data/settings_sync.htm`:
- Around line 61-65: The code that updates option labels in the loop (using
gN(n)?.options, vs and d.pinsData) overwrites any warning marker added by
makePinSelect(), so dmxPinUpd() removes the "⚠" for invalid pins; fix by
preserving any existing warning prefix when setting o.text (e.g. detect if
o.text currently contains the warning marker added by makePinSelect() and
prepend it to the new String(p) instead of blindly replacing the full label),
keep the same disabled logic and only change the label assignment so invalid-pin
labels retain their marker after update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b3180985-ec3e-4914-b537-fc3b94b4e976
📒 Files selected for processing (1)
wled00/data/settings_sync.htm
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
wled00/data/settings_sync.htm (2)
58-68: Consider updatingdmxOwnPinson change for stricter conflict tracking.
dmxOwnPinsis captured once at initialization and never updated when the user changes selections. This means a pin that was originally assigned to DMX remains "unlocked" even after being deselected and potentially claimed by another component via a page reload.The current behavior is acceptable for most workflows (user will save and reload to see updated allocations), but for stricter parity with
settings_leds.htm'spinUpd()pattern, you could updatedmxOwnPinsat the end ofdmxPinUpd():function dmxPinUpd() { const vs = dmxNs.map(n => parseInt(gN(n)?.value??-1)); dmxNs.forEach((n,i) => { for (const o of gN(n)?.options??[]) { const p = parseInt(o.value); if (p<0) continue; o.disabled = (p!==vs[i] && !dmxOwnPins.includes(p) && !!d.pinsData?.find(pi=>pi.p===p)?.a) || vs.some((v,j)=>j!==i&&v===p); if (!o.disabled) o.text = String(p); } }); + dmxOwnPins = vs.filter(v => v >= 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_sync.htm` around lines 58 - 68, dmxPinUpd currently reads dmxOwnPins once at init and never refreshes it, so update dmxOwnPins at the end of dmxPinUpd to reflect the current selections (like settings_leds.htm's pinUpd pattern); compute the currently selected pin list from dmxNs (using parseInt(gN(n)?.value) or similar) and assign it to dmxOwnPins so subsequent calls use the up-to-date ownership when disabling/enabling options.
53-55: Clarify the flag comment.The comment "RX pin is allowed input only" is ambiguous—it could be read as "RX may only be an input" (correct intent: RX accepts input-only GPIOs) or "RX is restricted to input". Consider rewording:
- dmxNs.forEach((n,i) => { const s = makePinSelect(n, i?1:0); if (s) s.onchange = dmxPinUpd; }); // note on the i?1:0 -> RX pin is allowed input only but TX/EN pins must be GPIO + dmxNs.forEach((n,i) => { const s = makePinSelect(n, i?1:0); if (s) s.onchange = dmxPinUpd; }); // i?1:0: RX (i=0) allows input-only GPIOs; TX/EN require output-capable pins🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_sync.htm` around lines 53 - 55, The inline comment in function pinDropdowns is ambiguous about RX pin behavior; update the comment near the makePinSelect call (involving dmxNs, makePinSelect, and dmxPinUpd) to clearly state that RX may accept GPIOs that are input-only (not that RX is restricted to being an input), e.g. rephrase to “RX may use input-only GPIOs; TX/EN must be full GPIOs” so the intent is unambiguous when reading the dmxNs.map and dmxOwnPins logic that follows (which uses gN).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wled00/data/settings_sync.htm`:
- Around line 58-68: dmxPinUpd currently reads dmxOwnPins once at init and never
refreshes it, so update dmxOwnPins at the end of dmxPinUpd to reflect the
current selections (like settings_leds.htm's pinUpd pattern); compute the
currently selected pin list from dmxNs (using parseInt(gN(n)?.value) or similar)
and assign it to dmxOwnPins so subsequent calls use the up-to-date ownership
when disabling/enabling options.
- Around line 53-55: The inline comment in function pinDropdowns is ambiguous
about RX pin behavior; update the comment near the makePinSelect call (involving
dmxNs, makePinSelect, and dmxPinUpd) to clearly state that RX may accept GPIOs
that are input-only (not that RX is restricted to being an input), e.g. rephrase
to “RX may use input-only GPIOs; TX/EN must be full GPIOs” so the intent is
unambiguous when reading the dmxNs.map and dmxOwnPins logic that follows (which
uses gN).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9d5a9f0f-5b36-499f-970f-a8e7ec5cc033
📒 Files selected for processing (3)
wled00/data/settings_sync.htmwled00/dmx_input.cppwled00/dmx_input.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding functions to easily create (restricted) dropdowns for pins and use those in LED settings and UM settings.
In LED settings pins are restricted by pinmanager, info about pin owner is also pulled from firmware. When populating a dropdown, flags can be passed and the dropdown will omit unusable pins i.e. does not show input only pins in LED outputs or non-analog or touch capable pins for analog/touch buttons.
This restriction is not implemented for usermods (yet) - usermods still show pins without restrictions.
Summary by CodeRabbit
New Features
Bug Fixes
Chores