Add MQTT Animated Staircase usermod v2#5606
Open
watak wants to merge 4 commits into
Open
Conversation
Contributor
WalkthroughAdds a new MQTT-driven "Animated Staircase" usermod (sensor + MQTT triggers, swipe animation, JSON persistence), centralizes v2 usermod registration, extends PinManager/consts, and updates PlatformIO sample overrides for ESP8266 and custom_usermods documentation. ChangesMQTT Animated Staircase + integration
Sequence DiagramsequenceDiagram
participant PIR as PIR Sensor (top/bottom)
participant MQTT as MQTT Broker
participant Usermod as MQTT_Animated_Staircase
participant WLED as WLED Core / Presets
participant Strip as LED Strip
rect rgba(100,150,255,0.5)
PIR->>Usermod: sensor state change
Usermod->>MQTT: publish motion/top or motion/bottom
end
rect rgba(150,200,100,0.5)
MQTT->>Usermod: /swipe message (top/bottom/ON)
Usermod->>WLED: apply preset (black then swipe)
end
rect rgba(200,150,100,0.5)
loop every segment_delay_ms
Usermod->>Usermod: updateSwipe() advance onIndex/offIndex
Usermod->>WLED: updateSegments() set SEG_OPTION_ON per segment
WLED->>Strip: render update
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
added
enhancement
usermod
![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: 6
🤖 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 `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp`:
- Around line 112-125: The helper functions publishMqttToTopic and
applyPresetWithDebug are only defined inside `#ifndef` WLED_DISABLE_MQTT but are
invoked from code that builds even when MQTT is disabled; make these call sites
safe by either moving the real implementations out of the `#ifndef` so they are
always compiled, or by providing no-op stubs when WLED_DISABLE_MQTT is set.
Specifically, ensure publishMqttToTopic(const char*, const char*) and
applyPresetWithDebug(...) have unconditional definitions (or conditional stubs
that mirror the real signatures and do nothing) so calls at the referenced
locations (around lines showing 126-132, 268-269, 375-376, 502-505) compile in
both MQTT-enabled and MQTT-disabled builds.
- Line 548: getId() currently returns the hardcoded value 100; update it to
return the declared constant USERMOD_ID_MQTT_ANIMATED_STAIRCASE so JSON
identification and inter-usermod ownership use the canonical ID (replace the
literal in the getId() function with USERMOD_ID_MQTT_ANIMATED_STAIRCASE).
- Around line 663-670: The pin-change detection is a no-op because
newTopPin/newBottomPin are assigned from
topPIRorTriggerPin/bottomPIRorTriggerPin after the config load, so the
comparison always fails; to fix, capture the current pin values into temp
variables (newTopPin/newBottomPin or better oldTopPin/oldBottomPin) before you
load/update the config, then after loading config compare the saved old values
to the updated members topPIRorTriggerPin and bottomPIRorTriggerPin and run the
reallocation/handling block only when they differ (check/initDone as currently
used) so the reallocation code in the block referencing initDone,
topPIRorTriggerPin, bottomPIRorTriggerPin is actually executed when pins change.
In `@usermods/usermod_mqtt_animated_staircase/README.md`:
- Around line 23-26: Close the opened ini code fence immediately after the
custom_usermods = MQTT_Animated_Staircase line by adding a closing ``` and then
reformat the payload section so it's a normal list (not indented code) using a
bullet for "Payloads:" and inline code formatting for values; specifically
update the README's block containing custom_usermods = MQTT_Animated_Staircase
to end the ``` fence, and change the indented code-style list entries for the
payload examples (the entries referencing top, bottom, ON) into a regular bullet
list with `top`, `bottom`, and `ON` wrapped in backticks.
In `@wled00/usermods_list.cpp`:
- Around line 479-481: The explicit manual registration UsermodManager::add(new
MQTT_Animated_Staircase()) causes double-registration because this usermod
should use the REGISTER_USERMOD(...) macro in its own .cpp; remove the
UsermodManager::add(...) line from usermods_list.cpp and verify that
MQTT_Animated_Staircase.cpp contains REGISTER_USERMOD(MQTT_Animated_Staircase)
(or add it there if missing) so the usermod is registered only via the macro.
- Around line 245-247: The include for the MQTT animated staircase usermod uses
the wrong filename/case: the code conditionally includes
"MQTT_Animated_Staircase.h" under USERMOD_MQTT_ANIMATED_STAIRCASE but the
implementation is added as "MQTT_animated_staircase.cpp" (case/extension
mismatch); fix by making the header include match the real file name and casing
(or rename the header/implementation so both match), i.e. update the include in
usermods_list.cpp (the conditional block with USERMOD_MQTT_ANIMATED_STAIRCASE)
to reference the exact existing header filename/casing used by the
MQTT_animated_staircase implementation.
🪄 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: abffc7a3-9d32-4620-9da1-ef2142bdf7ba
📒 Files selected for processing (7)
platformio_override.sample.iniusermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cppusermods/usermod_mqtt_animated_staircase/README.mdusermods/usermod_mqtt_animated_staircase/library.jsonwled00/const.hwled00/pin_manager.hwled00/usermods_list.cpp
💤 Files with no reviewable changes (1)
- platformio_override.sample.ini
Comment on lines
+245
to
+247
| #ifdef USERMOD_MQTT_ANIMATED_STAIRCASE | ||
| #include "../usermods/usermod_mqtt_animated_staircase/MQTT_Animated_Staircase.h" | ||
| #endif |
Contributor
There was a problem hiding this comment.
MQTT staircase include path appears invalid (name/case/extension mismatch).
This file includes MQTT_Animated_Staircase.h, but the added implementation is MQTT_animated_staircase.cpp. On case-sensitive builds this will fail.
🤖 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/usermods_list.cpp` around lines 245 - 247, The include for the MQTT
animated staircase usermod uses the wrong filename/case: the code conditionally
includes "MQTT_Animated_Staircase.h" under USERMOD_MQTT_ANIMATED_STAIRCASE but
the implementation is added as "MQTT_animated_staircase.cpp" (case/extension
mismatch); fix by making the header include match the real file name and casing
(or rename the header/implementation so both match), i.e. update the include in
usermods_list.cpp (the conditional block with USERMOD_MQTT_ANIMATED_STAIRCASE)
to reference the exact existing header filename/casing used by the
MQTT_animated_staircase implementation.
Comment on lines
+479
to
+481
| #ifdef USERMOD_MQTT_ANIMATED_STAIRCASE | ||
| UsermodManager::add(new MQTT_Animated_Staircase()); | ||
| #endif |
Contributor
There was a problem hiding this comment.
Avoid double registration for the same usermod instance.
UsermodManager::add(new MQTT_Animated_Staircase()) duplicates registration if the usermod already self-registers via REGISTER_USERMOD(...) in its .cpp. Keep only one registration mechanism.
As per coding guidelines: usermods/**/*.cpp: Register usermods with REGISTER_USERMOD() macro in usermod .cpp files.
🤖 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/usermods_list.cpp` around lines 479 - 481, The explicit manual
registration UsermodManager::add(new MQTT_Animated_Staircase()) causes
double-registration because this usermod should use the REGISTER_USERMOD(...)
macro in its own .cpp; remove the UsermodManager::add(...) line from
usermods_list.cpp and verify that MQTT_Animated_Staircase.cpp contains
REGISTER_USERMOD(MQTT_Animated_Staircase) (or add it there if missing) so the
usermod is registered only via the macro.
softhack007
reviewed
May 12, 2026
Member
There was a problem hiding this comment.
please revert this change - deleting the platformio_override.sample.ini file in the WLED repo is a no-go for merging this PR.
softhack007
reviewed
May 12, 2026
softhack007
reviewed
May 12, 2026
| #define WIFI_ENCRYPTION_TYPE_PSK 0 //None/WPA/WPA2 | ||
| #define WIFI_ENCRYPTION_TYPE_ENTERPRISE 1 //WPA/WPA2-Enterprise | ||
| #endif | ||
| #define USERMOD_ID_MQTT_ANIMATED_STAIRCASE 105 // Just pick a number not used in the list |
softhack007
reviewed
May 12, 2026
Member
There was a problem hiding this comment.
this file is deprecated in WLED 16.0.0, please don't re-add it.
softhack007
reviewed
May 12, 2026
|
|
||
| Because this usermod uses the modern WLED v0.16 library structure, you do not need to modify any core WLED files to install it. | ||
|
|
||
| 1. Copy the `MQTT_Animated_Staircase` folder into your WLED `usermods/` directory. |
Member
There was a problem hiding this comment.
obsolete - the folder is already there
softhack007
reviewed
May 12, 2026
| * **Dynamic MQTT Integration:** Seamlessly uses WLED's native MQTT settings—no hardcoded topics! | ||
|
|
||
| ## 🛠️ Hardware Requirements | ||
| * Any ESP32 microcontroller (Classic, S2, S3, or C3). |
Member
There was a problem hiding this comment.
if esp8266 is not supported, you should explicitly state it.
softhack007
added
the
needs_rework
Collaborator
|
what is the difference to the existing staircase UM? Can it be integrated? |
…ate README, and clean const.h
Author
|
Thanks for the great feedback! I have reverted all the core files (usermods_list.cpp, pin_manager.h, etc.), fixed the pin reallocation logic, cleaned up const.h, and added the Home Assistant YAML examples to the README. Ready for another look. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp (3)
500-503: 💤 Low valueConsider removing
delay()calls fromsetup().While
delay()insetup()is less critical than inloop()or callbacks, the two 200ms delays here still block initialization and could be replaced with deferred preset application in the firstloop()iteration.If initialization timing is flexible, schedule the preset applications via flags and apply them in
loop()afterinitDoneis set.🤖 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 `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp` around lines 500 - 503, The two blocking delay(200) calls in setup() around applyPresetWithDebug(3) and applyPresetWithDebug(1) should be removed and the preset applications deferred to the first loop() pass: add a boolean flag (e.g., presetsPending or reuse initDone logic) that setup() sets true after hardware init, then in loop() check the flag and, when true, call applyPresetWithDebug(3), wait the required intervals non-blockingly (use millis() timers) before calling applyPresetWithDebug(1), and clear the flag; this keeps applyPresetWithDebug usage (function name) but eliminates delay() from setup().
108-109: ⚡ Quick winRemove unused static string declarations.
_fadeTime_msand_fadeTargetBrightnessare declared but never defined in the PROGMEM section (lines 727-734) and are not used anywhere in the code. The related config fields are also commented out (lines 598-599, 624-625).🧹 Proposed fix
static const char _togglePower[]; static const char _mqttOnly[]; - static const char _fadeTime_ms[]; - static const char _fadeTargetBrightness[];🤖 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 `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp` around lines 108 - 109, Remove the unused static PROGMEM string declarations _fadeTime_ms and _fadeTargetBrightness from MQTT_animated_staircase (they are declared but never defined or used); also remove or permanently delete related commented config fields that reference those names (the commented lines around the config blocks) so there are no dangling/unused symbols; ensure no other code references _fadeTime_ms or _fadeTargetBrightness before committing.
510-510: 💤 Low valueClarify initialization comment for
offIndex.The comment says "Initialize offIndex to the end" but the code sets
offIndex = maxSegmentId - 1, notmaxSegmentId. This might be intentional for your animation logic, but the comment is slightly misleading.📝 Suggested clarification
- offIndex = maxSegmentId -1; // THIS IS IMPORTANT: Initialize offIndex to the end + offIndex = maxSegmentId - 1; // Initialize offIndex one before the boundary🤖 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 `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp` at line 510, The comment for offIndex is misleading: it states "Initialize offIndex to the end" but the code assigns offIndex = maxSegmentId - 1; update the comment near the assignment in MQTT_animated_staircase.cpp (the offIndex variable initialization) to explicitly state that it is set to the last valid index using zero-based indexing (e.g., "Initialize offIndex to the last segment index (maxSegmentId - 1)"), so the comment matches the code and intent used by the animation logic.
🤖 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 `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp`:
- Around line 24-26: Duplicate definitions of SWIPE_UP and SWIPE_DOWN exist:
remove the macro versions and keep the static constexpr bool declarations
(static constexpr bool SWIPE_UP and static constexpr bool SWIPE_DOWN) so there’s
a single source of truth; update any use-sites (e.g., the variable
swipeDirection) to rely on those constexpr symbols and delete the `#define`
SWIPE_UP / `#define` SWIPE_DOWN macros to avoid shadowing and DRY violations.
- Around line 181-184: The MQTT callback currently uses blocking delay() calls
after applyPresetWithDebug(3) and applyPresetWithDebug(1), which can stall the
event loop; instead, set up the existing non-blocking mechanism by setting
pendingPresetChange = true, pendingPresetId (e.g., to 3 then 1) and
presetChangeTime = millis() + 200 (or chain two scheduled changes) from the MQTT
handler (onMqttMessage) and let loop() perform
applyPresetWithDebug(pendingPresetId) when millis() >= presetChangeTime; remove
delay() calls and avoid direct calls to applyPresetWithDebug with blocking waits
inside the MQTT callback.
- Line 10: Remove the header-guard directive from the implementation file:
delete the lone "#pragma once" line at the top of MQTT_animated_staircase.cpp
(the CPP for the MQTT_animated_staircase us ermod) so the file follows C++
conventions that `#pragma` once belongs only in headers; no other changes are
required.
---
Nitpick comments:
In `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp`:
- Around line 500-503: The two blocking delay(200) calls in setup() around
applyPresetWithDebug(3) and applyPresetWithDebug(1) should be removed and the
preset applications deferred to the first loop() pass: add a boolean flag (e.g.,
presetsPending or reuse initDone logic) that setup() sets true after hardware
init, then in loop() check the flag and, when true, call
applyPresetWithDebug(3), wait the required intervals non-blockingly (use
millis() timers) before calling applyPresetWithDebug(1), and clear the flag;
this keeps applyPresetWithDebug usage (function name) but eliminates delay()
from setup().
- Around line 108-109: Remove the unused static PROGMEM string declarations
_fadeTime_ms and _fadeTargetBrightness from MQTT_animated_staircase (they are
declared but never defined or used); also remove or permanently delete related
commented config fields that reference those names (the commented lines around
the config blocks) so there are no dangling/unused symbols; ensure no other code
references _fadeTime_ms or _fadeTargetBrightness before committing.
- Line 510: The comment for offIndex is misleading: it states "Initialize
offIndex to the end" but the code assigns offIndex = maxSegmentId - 1; update
the comment near the assignment in MQTT_animated_staircase.cpp (the offIndex
variable initialization) to explicitly state that it is set to the last valid
index using zero-based indexing (e.g., "Initialize offIndex to the last segment
index (maxSegmentId - 1)"), so the comment matches the code and intent used by
the animation logic.
🪄 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: 4cc522e1-697b-4df4-9b6d-b1b85d62156f
📒 Files selected for processing (5)
platformio_override.sample.iniusermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cppusermods/usermod_mqtt_animated_staircase/README.mdwled00/const.hwled00/pin_manager.h
✅ Files skipped from review due to trivial changes (1)
- usermods/usermod_mqtt_animated_staircase/README.md
| * | ||
| * See the accompanying README.md file for more info. | ||
| */ | ||
| #pragma once |
Contributor
There was a problem hiding this comment.
Remove #pragma once from .cpp file.
#pragma once is a header guard intended for .h files, not .cpp implementation files. Including it here has no effect and violates convention.
🔧 Proposed fix
-#pragma once
`#include` "wled.h"As per coding guidelines: C++ files should follow standard conventions.
🤖 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 `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp` at line
10, Remove the header-guard directive from the implementation file: delete the
lone "#pragma once" line at the top of MQTT_animated_staircase.cpp (the CPP for
the MQTT_animated_staircase us ermod) so the file follows C++ conventions that
`#pragma` once belongs only in headers; no other changes are required.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp`:
- Around line 235-252: The code currently publishes motion events to hardcoded
topics "motion/top" and "motion/bottom" (in the block handling
bottomSensorRead/bottomSensorState and topSensorRead/topSensorState) but the
integration expects "<device_topic>/motion/0" (top) and
"<device_topic>/motion/1" (bottom); replace the direct calls to
publishMqttToTopic("motion/top"/"motion/bottom", ...) with the existing helper
publishMqtt(bool bottom, ...) so the messages follow the documented topic
contract (call publishMqtt(false, ...) for top and publishMqtt(true, ...) for
bottom and pass the on/off payload as before), ensuring sensorChanged and sensor
state updates remain unchanged.
- Around line 170-194: The MQTT handler onMqttMessage currently performs actions
even when the usermod is disabled; update onMqttMessage to check the usermod's
enabled flag at the top and return false immediately if enabled is false so it
does not copy payload, call applyPresetWithDebug(3), or set
pendingMqttSwipe/timeToApplyMqttSwipe; likewise, ensure the loop() code that
drains pendingMqttSwipe also checks enabled before consuming or executing any
pendingMqttAction (so pendingMqttSwipe is ignored when enabled is false) by
adding the same enabled guard around the pendingMqttSwipe handling logic.
- Around line 586-592: readFromConfig() is checking for a PROGMEM key (e.g., the
constant used for _togglePower, currently named "toggle-on-off") that does not
match the key used when serializing in addStaircaseToJsonObject()
("toggle-power"), causing newly saved configs to appear missing; fix by defining
a single static const char[] PROGMEM key (e.g., ST_KEY_TOGGLE_POWER) and use
that same symbol in both addStaircaseToJsonObject() and readFromConfig() (and
any other save/load sites referenced in the review like the blocks around lines
606–617, 692, 724–729) so the serialized name and the lookup name are identical
and conform to the usermods PROGMEM string guideline.
- Around line 222-260: checkSensors() currently only updates sensor state and
publishes MQTT but never starts the animation; either propagate its
sensorChanged return into loop() or, simpler, trigger the animation directly
inside checkSensors(): when bottomSensorRead != bottomSensorState (or
topSensorRead != topSensorState) detect the active transition and call
setIndex(...) with the appropriate starting index, set swipeDirection (up/down
depending on which sensor fired) and set animationActive = true; ensure you use
the new sensorRead values (bottomSensorRead/topSensorRead) to decide direction
and only start the animation on the active edge (e.g., when sensorRead becomes
true), and keep the existing MQTT publish and topSensorWrite/bottomSensorWrite
resets.
🪄 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: ea49ebcb-2a76-4555-91f2-0979b81ff60f
📒 Files selected for processing (1)
usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp
Comment on lines
+170
to
+194
| bool onMqttMessage(char* topic, char* payload) { | ||
| #ifndef WLED_DISABLE_MQTT | ||
| if (strlen(topic) == 6 && strncmp_P(topic, PSTR("/swipe"), 6) == 0) { | ||
| Serial.print(F("MQTT swipe detected: ")); | ||
| Serial.println(payload); | ||
|
|
||
| // 1. Save the payload text so it isn't deleted from memory | ||
| strncpy(pendingMqttAction, payload, sizeof(pendingMqttAction) - 1); | ||
| pendingMqttAction[sizeof(pendingMqttAction) - 1] = '\0'; | ||
|
|
||
| // 2. Apply the Black preset instantly | ||
| applyPresetWithDebug(3); | ||
|
|
||
| // 3. Start your non-blocking timer | ||
| pendingMqttSwipe = true; | ||
| timeToApplyMqttSwipe = millis(); | ||
|
|
||
| return true; // Message handled! | ||
| } | ||
| #else | ||
| (void)topic; | ||
| (void)payload; | ||
| #endif | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Honor enabled before executing MQTT-triggered actions.
onMqttMessage() applies preset 3 and queues a swipe even when the usermod is disabled, and loop() drains pendingMqttSwipe before the !enabled early return. With "enabled": false, MQTT can still black out the strip and start animations.
Also applies to: 477-528
🤖 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 `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp` around
lines 170 - 194, The MQTT handler onMqttMessage currently performs actions even
when the usermod is disabled; update onMqttMessage to check the usermod's
enabled flag at the top and return false immediately if enabled is false so it
does not copy payload, call applyPresetWithDebug(3), or set
pendingMqttSwipe/timeToApplyMqttSwipe; likewise, ensure the loop() code that
drains pendingMqttSwipe also checks enabled before consuming or executing any
pendingMqttAction (so pendingMqttSwipe is ignored when enabled is false) by
adding the same enabled guard around the pendingMqttSwipe handling logic.
Comment on lines
+222
to
+260
| bool checkSensors() | ||
| { | ||
| bool sensorChanged = false; | ||
|
|
||
| if ((millis() - lastScanTime) > scanDelay) { | ||
| lastScanTime = millis(); | ||
|
|
||
| bottomSensorRead = bottomSensorWrite || | ||
| ((bottomPIRorTriggerPin < 0) ? false : digitalRead(bottomPIRorTriggerPin)); | ||
|
|
||
| topSensorRead = topSensorWrite || | ||
| ((topPIRorTriggerPin < 0) ? false : digitalRead(topPIRorTriggerPin)); | ||
|
|
||
| // Publish MQTT for bottom sensor if state changed | ||
| if (bottomSensorRead != bottomSensorState) | ||
| { | ||
| bottomSensorState = bottomSensorRead; | ||
| sensorChanged = true; | ||
|
|
||
| // Publish to "motion/bottom" with "on" or "off" | ||
| publishMqttToTopic("motion/bottom", bottomSensorState ? "on" : "off"); | ||
| } | ||
|
|
||
| // Publish MQTT for top sensor if state changed | ||
| if (topSensorRead != topSensorState) | ||
| { | ||
| topSensorState = topSensorRead; | ||
| sensorChanged = true; | ||
|
|
||
| publishMqttToTopic("motion/top", topSensorState ? "on" : "off"); | ||
|
|
||
| } | ||
|
|
||
| // Reset API flags | ||
| topSensorWrite = false; | ||
| bottomSensorWrite = false; | ||
| } | ||
|
|
||
| return sensorChanged; |
Contributor
There was a problem hiding this comment.
GPIO sensor transitions never start the staircase animation.
checkSensors() only updates state and publishes MQTT, and loop() ignores the returned sensorChanged flag. As written, configured top/bottom pins never drive setIndex(...), swipeDirection, or animationActive, so the staircase cannot be triggered locally from the attached sensors.
Also applies to: 477-535
🤖 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 `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp` around
lines 222 - 260, checkSensors() currently only updates sensor state and
publishes MQTT but never starts the animation; either propagate its
sensorChanged return into loop() or, simpler, trigger the animation directly
inside checkSensors(): when bottomSensorRead != bottomSensorState (or
topSensorRead != topSensorState) detect the active transition and call
setIndex(...) with the appropriate starting index, set swipeDirection (up/down
depending on which sensor fired) and set animationActive = true; ensure you use
the new sensorRead values (bottomSensorRead/topSensorRead) to decide direction
and only start the animation on the active edge (e.g., when sensorRead becomes
true), and keep the existing MQTT publish and topSensorWrite/bottomSensorWrite
resets.
Comment on lines
+235
to
+252
| // Publish MQTT for bottom sensor if state changed | ||
| if (bottomSensorRead != bottomSensorState) | ||
| { | ||
| bottomSensorState = bottomSensorRead; | ||
| sensorChanged = true; | ||
|
|
||
| // Publish to "motion/bottom" with "on" or "off" | ||
| publishMqttToTopic("motion/bottom", bottomSensorState ? "on" : "off"); | ||
| } | ||
|
|
||
| // Publish MQTT for top sensor if state changed | ||
| if (topSensorRead != topSensorState) | ||
| { | ||
| topSensorState = topSensorRead; | ||
| sensorChanged = true; | ||
|
|
||
| publishMqttToTopic("motion/top", topSensorState ? "on" : "off"); | ||
|
|
Contributor
There was a problem hiding this comment.
Publish the documented motion topics.
This currently emits motion/top and motion/bottom, but the documented integration listens on <device_topic>/motion/0 for top and <device_topic>/motion/1 for bottom. Anyone wiring Home Assistant from the README will miss every sensor update. Reusing publishMqtt(bool bottom, ...) here would keep the code aligned with the published contract. As per coding guidelines: README.md says "read motion state from <device_topic>/motion/0 (top) and <device_topic>/motion/1 (bottom)".
🤖 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 `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp` around
lines 235 - 252, The code currently publishes motion events to hardcoded topics
"motion/top" and "motion/bottom" (in the block handling
bottomSensorRead/bottomSensorState and topSensorRead/topSensorState) but the
integration expects "<device_topic>/motion/0" (top) and
"<device_topic>/motion/1" (bottom); replace the direct calls to
publishMqttToTopic("motion/top"/"motion/bottom", ...) with the existing helper
publishMqtt(bool bottom, ...) so the messages follow the documented topic
contract (call publishMqtt(false, ...) for top and publishMqtt(true, ...) for
bottom and pass the on/off payload as before), ensuring sensorChanged and sensor
state updates remain unchanged.
Comment on lines
+586
to
+592
| staircase[F("enabled")] = enabled; | ||
| staircase[F("mqtt-only-trigger")] = mqttOnlyTrigger; | ||
| staircase[F("segment-delay")] = segment_delay_ms; | ||
| staircase[F("on-time")] = on_time_ms / 1000; // Convert to seconds for storage | ||
| staircase[F("topPIRorTrigger-pin")] = topPIRorTriggerPin; | ||
| staircase[F("bottomPIRorTrigger-pin")] = bottomPIRorTriggerPin; | ||
| staircase[F("toggle-power")] = togglePower; |
Contributor
There was a problem hiding this comment.
readFromConfig() checks a key that this file never writes.
addStaircaseToJsonObject() serializes toggle-power, but _togglePower is defined as toggle-on-off and readFromConfig() returns the presence of that constant. On a freshly saved config this reports "no config" even though the fields were loaded. Please make the PROGMEM keys match the serialized names and reuse them in both save/load paths. As per coding guidelines: usermods/**/*.{cpp,h,hpp}: Store repeated strings as static const char[] PROGMEM in usermods.
Also applies to: 606-617, 692-692, 724-729
🤖 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 `@usermods/usermod_mqtt_animated_staircase/MQTT_animated_staircase.cpp` around
lines 586 - 592, readFromConfig() is checking for a PROGMEM key (e.g., the
constant used for _togglePower, currently named "toggle-on-off") that does not
match the key used when serializing in addStaircaseToJsonObject()
("toggle-power"), causing newly saved configs to appear missing; fix by defining
a single static const char[] PROGMEM key (e.g., ST_KEY_TOGGLE_POWER) and use
that same symbol in both addStaircaseToJsonObject() and readFromConfig() (and
any other save/load sites referenced in the review like the blocks around lines
606–617, 692, 724–729) so the serialized name and the lookup name are identical
and conform to the usermods PROGMEM string guideline.
Collaborator
|
@coderabbitai how much code overlap is there between this PR and the existing animated staircase usermod? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 27831 🏁 Script executed: Length of output: 22012
Identical or Near-Identical Code
Key Differences (What's New)
Integration AssessmentThe new usermod's unique value is the preset-based animation (applying WLED presets 1/2/3 to define the look) and the deferred MQTT-only trigger mode. These could plausibly be merged into the existing The critical blocker for coexistence is that both use |
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
3 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.
Added a usermod for a staircase effect using segments and being triggered by the top of bottom staircase motion sensors (which are added directly onto the WLED board of your choosing)
Summary by CodeRabbit
New Features
Documentation
Chores