Skip to content

button_relay_toggle_v2: bound retained-command guard to a time window, fix active_low re-drive - #28

Open
KrX3D wants to merge 1 commit into
button_relay_toggle_UMfrom
fix/button-relay-toggle-v2-review-bugs
Open

button_relay_toggle_v2: bound retained-command guard to a time window, fix active_low re-drive#28
KrX3D wants to merge 1 commit into
button_relay_toggle_UMfrom
fix/button-relay-toggle-v2-review-bugs

Conversation

@KrX3D

@KrX3D KrX3D commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up review of button_relay_toggle_v2 (after #7/#25), looking for anything left. Found and fixed two real bugs, plus a doc typo:

  • The stale-retained-command guard could swallow genuine live commands. _bootOverridePending[i] gets armed on every settings save and MQTT reconnect via setupRelaySubscriptions() (for every relay with a valid pin, not just non-Default boot-state ones), and stays armed until the next /set message arrives — however much later that is. If a real, live Home Assistant command happened to be the first one received after a reconnect that occurred minutes earlier, it was misclassified as "the stale retained redelivery" and silently overridden back to the relay's current state, dropping the user's command. Replaced the boolean with a short (3s) time-bounded deadline (_overrideDeadline[i]) — retained messages are redelivered by the broker essentially immediately on subscribe, so this window still catches that case without swallowing an unrelated later command.
  • Toggling relay_active_low alone silently flipped the reported state. In readFromConfig()'s "same pin" branch, when only relay_active_low changed (pin/wiring untouched), the code called pinMode(OUTPUT) but never re-drove the pin. Since the reported logical on/off state is derived as activeLow ? !digitalRead(pin) : digitalRead(pin), flipping the setting flipped what HA/MQTT reported even though the relay's actual electrical state never changed. Now captures the old active_low value before the config read and, if it changed, re-drives the pin so the logical state stays the same as before the save.
  • Fixed a corrupted arrow character (? instead of ->) in the readme's wiring diagram.

I looked at a third finding from the review — plumbing the MQTT retain flag through to onMqttMessage() instead of approximating it by timing — but that requires changing the Usermod::onMqttMessage() interface used by every usermod (wled00/mqtt.cpp, wled00/fcn_declare.h), which is out of scope for a usermod-only fix. Noting it here as a possible follow-up for core.

Test plan

  • Static review only in this session (no PlatformIO toolchain available to build/flash here).
  • Recommend testing on real hardware: toggle relay_active_low alone via settings and confirm HA doesn't see a spurious state flip; and confirm a live HA command right after a settings save/reconnect is no longer overridden.

🤖 Generated with Claude Code

https://claude.ai/code/session_01J6bJtispewBKR2dXBcNWpw


Generated by Claude Code

…ndow, fix active_low re-drive

- _bootOverridePending was armed unconditionally on every settings save and
  MQTT reconnect (for every configured relay, not just ones with a non-Default
  boot state) and stayed armed indefinitely until the *next* /set message,
  however much later that arrived. A genuine, live command from Home Assistant
  landing in that window was silently reinterpreted as a stale retained
  redelivery and overridden back to the relay's current state. Replaced the
  boolean with a short (3s) time-bounded deadline: retained messages are
  redelivered by the broker essentially immediately on subscribe, so this
  still catches that case without swallowing a live command that just happens
  to be first after a reconnect minutes earlier.
- When relay_active_low was toggled via settings save while the physical pin
  was unchanged, the code only called pinMode(OUTPUT) and never re-drove the
  pin, so the reported logical on/off state (used for MQTT/HA and the button
  toggle) silently flipped even though the relay's real electrical state
  hadn't changed. Now re-drives the pin to preserve the previous logical
  state when only active_low changes.
- Fixed a corrupted arrow character in the readme's wiring diagram.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J6bJtispewBKR2dXBcNWpw

KrX3D commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

The wled_build / Test cdata.js check failed on the first run, but it's not related to this PR's diff (which only touches usermods/button_relay_toggle_v2/button_relay_toggle_v2.cpp and its readme.md). The failing assertion is in tools/cdata-test.js ("script was executed with -f or --force" expecting wled00/html_ui.h to have been touched by a forced rebuild) — a filesystem-mtime-based test with no connection to this change, and the same commit history's most recent direct push to button_relay_toggle_UM passed this exact check. Re-running the failed job once to confirm it's a flake.


Generated by Claude Code

KrX3D commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Re-run passed — confirms the earlier Test cdata.js failure was the flaky mtime-based subtest, unrelated to this change. CI is green.


Generated by Claude Code

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants