Skip to content

Analog button fix#5659

Open
benjamw wants to merge 2 commits into
wled:mainfrom
benjamw:analog_button_fix
Open

Analog button fix#5659
benjamw wants to merge 2 commits into
wled:mainfrom
benjamw:analog_button_fix

Conversation

@benjamw
Copy link
Copy Markdown
Contributor

@benjamw benjamw commented Jun 1, 2026

This fixes the issues noted in #5613 (comment)

This also adds clarification for the differences between analog and digital inputs. Showing and hiding dropdowns as needed to make those selections clearer.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

Walkthrough

This PR extends button settings to support analog button types. Helper functions map button type IDs and generate analog-specific options. The addRow() function branches on button type, rendering analog buttons with hidden short/long-press fields and an analog-function selector; non-analog buttons retain separate preset selectors. Backend serialization updated to pass double-press and type parameters.

Changes

Analog Button Type Support

Layer / File(s) Summary
Preset labels and button type helpers
wled00/data/settings_time.htm
Default action preset label updated to include "(0)" suffix. New functions bAO(), isAnalogBtn(), and btnTypeName() map button type IDs to names and generate analog-specific option sets; rBPO() uses these to select appropriate options based on button type.
Button row rendering by type
wled00/data/settings_time.htm
addRow() signature extended with button type parameter; rendering branches on type to output hidden zero values for short/long-press on analog buttons and analog-function selector, while non-analog buttons keep separate preset selectors for short, long, and double press.
Backend serialization for new row signature
wled00/xml.cpp
getSettingsJS() updated to pass button.macroDoublePress and button.type to addRow() calls, extending argument list to match new signature.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • wled/WLED#5636: Modifies button action preset UI labels in settings_time.htm, including switch/press labeling updates.
  • wled/WLED#4757: Directly aligned with analog button type handling and button.macroDoublePress integration in settings UI.
  • wled/WLED#5140: Builds on macro preset integration in settings_time.htm that this PR extends for analog button support.

Suggested labels

enhancement

Suggested reviewers

  • DedeHai
  • softhack007
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Analog button fix' is vague and generic. While it relates to the changeset (which involves analog button handling), it lacks specificity about what was fixed or the nature of the improvement. Consider a more specific title like 'Add analog button support and fix duplicate option handling' to better convey the actual changes made.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@wled00/data/settings_time.htm`:
- Around line 309-311: The label "Double press (n/a)" is misleading because the
select named "MD${b}" is actually hydrated from button.macroDoublePress in
wled00/xml.cpp; either rename the label to "Double press" to reflect it's
functional or make the control non-editable/disabled if the firmware still
ignores the value. Update the HTML in settings_time.htm to change the <label>
text for the select with name "MD${b}" (or add the disabled attribute/class) and
ensure consistency with the button.macroDoublePress usage in wled00/xml.cpp so
the UI accurately represents whether the value is applied.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8bec5088-0633-4560-b48a-0fece4a75f8f

📥 Commits

Reviewing files that changed from the base of the PR and between 0a1a7fc and 47026f5.

📒 Files selected for processing (2)
  • wled00/data/settings_time.htm
  • wled00/xml.cpp

Comment on lines +309 to +311
<div class="ba">
<label>Double press (n/a)</label>
<select name="MD${b}" class="s" required>${presetOpts}</select>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Rename this field or actually disable it.

This select is now hydrated from button.macroDoublePress in wled00/xml.cpp Line 631, so labeling it Double press (n/a) is misleading. Either call it Double press or keep it non-editable if the firmware still ignores the value.

✏️ Proposed text fix
-						<label>Double press (n/a)</label>
+						<label>Double press</label>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@wled00/data/settings_time.htm` around lines 309 - 311, The label "Double
press (n/a)" is misleading because the select named "MD${b}" is actually
hydrated from button.macroDoublePress in wled00/xml.cpp; either rename the label
to "Double press" to reflect it's functional or make the control
non-editable/disabled if the firmware still ignores the value. Update the HTML
in settings_time.htm to change the <label> text for the select with name
"MD${b}" (or add the disabled attribute/class) and ensure consistency with the
button.macroDoublePress usage in wled00/xml.cpp so the UI accurately represents
whether the value is applied.

buttonBlock.innerHTML = `
<div class="bh">Button ${i} - ${typeName}</div>
<div class="bs">
<input type="hidden" name="MP${b}" value="0">
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we require the hidden values? i.e. they are set to default zero in firmware if missing, see set.cpp

if (t===undefined) t = 0;
var b = String.fromCharCode((i<10?48:55)+i);
var presetOpts = '<option value="0">Default Action</option>' + sortedPresetOptions;
var analog = isAnalogBtn(t);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

single use variable adds code for no benefit

@DedeHai
Copy link
Copy Markdown
Collaborator

DedeHai commented Jun 2, 2026

thanks, looks good to me, just two minor things.

edit:
the rabbit has a point about the button vs. switch: since now the page knows the button type and you already went through the effort of labelling the buttons, the function labels could be type specific. Is that something that can be done by adding a few more lines without duplicating the whole block?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants