Usermod Animated Staircase: added sentinel option and fixed off swipe direction#5076
Usermod Animated Staircase: added sentinel option and fixed off swipe direction#5076SentiQ wants to merge 9 commits into
Conversation
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds per-stair sentinel support to Animated_Staircase: two new config fields ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/Animated_Staircase/Animated_Staircase.cpp(11 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
usermods/Animated_Staircase/Animated_Staircase.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
usermods/Animated_Staircase/Animated_Staircase.cpp
📚 Learning: 2025-04-24T09:31:06.879Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4658
File: wled00/led.cpp:90-91
Timestamp: 2025-04-24T09:31:06.879Z
Learning: In the WLED codebase, the `getSegment()` function guards against out-of-bounds segment IDs by falling back to the main segment ID, and `getFirstSelectedSegId()` falls back to `getMainSegmentId()` if no segments are selected, ensuring no crashes when used through the `setValuesFromFirstSelectedSeg()` macro.
Applied to files:
usermods/Animated_Staircase/Animated_Staircase.cpp
📚 Learning: 2025-10-20T09:41:50.374Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Applied to files:
usermods/Animated_Staircase/Animated_Staircase.cpp
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
usermods/Animated_Staircase/Animated_Staircase.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
usermods/Animated_Staircase/Animated_Staircase.cpp
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@realrolfje any comments on this? |
blazoncek
left a comment
blazoncek
left a comment
There was a problem hiding this comment.
I do not use Animated Staircase usermod so cannot comment on usability enhancements.
|
@coderabbitai are the changes fully backwards compatible or are there any breaking changes for existing setups? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 24082
✅ Fully backwards compatible
|
|
FYI: Animated Staircase usermod actually not working anymore with current main branch. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
usermods/Animated_Staircase/Animated_Staircase.cpp (2)
334-342: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t reset opacity when sentinel mode is disabled.
enabledSentineldefaults tofalse, but disabling the usermod now still forces first/last segment opacity to255, overriding custom opacity for existing setups. Gate this restore behind sentinel mode at minimum.Proposed fix
- if (strip.getSegmentsNum() > 0) { + if (enabledSentinel && strip.getSegmentsNum() > 0) { byte firstSegId = strip.getMainSegmentId();🤖 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/Animated_Staircase/Animated_Staircase.cpp` around lines 334 - 342, The opacity reset in Animated_Staircase::loop (the block using getMainSegmentId, getLastActiveSegmentId, and setOpacity(255)) is now running even when sentinel mode is disabled. Gate this first/last segment restore behind enabledSentinel so the existing custom opacity is preserved unless sentinel mode is actually active.
125-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor
sentinelDimOpacity = 0as a valid configured opacity.Line 128 turns sentinels off when the configured opacity is
0, even though the value is clamped and persisted as part of the valid[0,255]range. Keep the segment active and apply opacity0.Proposed fix
- } else if (enabledSentinel && isSentinel && sentinelDimOpacity > 0) { + } else if (enabledSentinel && isSentinel) { seg.setOption(SEG_OPTION_ON, true); seg.setOpacity(sentinelDimOpacity);🤖 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/Animated_Staircase/Animated_Staircase.cpp` around lines 125 - 130, The sentinel rendering branch in Animated_Staircase.cpp is treating sentinelDimOpacity as “disabled” when it equals 0, but 0 is a valid persisted opacity. Update the conditional around the sentinel path in the segment update logic so enabledSentinel and isSentinel still keep the segment on even when sentinelDimOpacity is 0, and always apply seg.setOpacity(sentinelDimOpacity) for that branch.
🧹 Nitpick comments (3)
usermods/Animated_Staircase/Animated_Staircase.cpp (3)
218-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the
lastSensorcomment with the actual stored value.The new comment says
truemeans bottom andfalsemeans top, which conflicts with the nearbyLOWER/UPPERconstants. Reword this as “mirrorsbottomSensorRead” to avoid future swipe-direction mistakes. As per path instructions, verify comments match code behavior.🤖 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/Animated_Staircase/Animated_Staircase.cpp` around lines 218 - 221, The comment around lastSensor assignment conflicts with the LOWER/UPPER sensor naming and can mislead future changes. Update the comment near the lastSensor = bottomSensorRead assignment in Animated_Staircase so it simply says lastSensor mirrors bottomSensorRead, matching the actual stored value and avoiding any implied swipe-direction semantics.Source: Path instructions
505-506: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused invert snapshots.
oldTopAPinInvertandoldBottomAPinInvertare assigned but never read inreadFromConfig(). Delete them unless they are needed for a change check. As per coding guidelines, remove dead/unused code or justify it with comments.🤖 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/Animated_Staircase/Animated_Staircase.cpp` around lines 505 - 506, Remove the unused invert snapshot variables in readFromConfig(), since oldTopAPinInvert and oldBottomAPinInvert are assigned but never read. Update the Animated_Staircase::readFromConfig() logic to delete these dead declarations unless you actually need them for a change-detection comparison, and keep the function focused on the values it uses.Source: Coding guidelines
174-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the extra indentation in
readPIRPin().Line 176 is indented one level too deep. As per coding guidelines, C++ files should use 2-space indentation with no tabs.
Proposed fix
- bool v = digitalRead(pin); + bool v = digitalRead(pin);🤖 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/Animated_Staircase/Animated_Staircase.cpp` around lines 174 - 177, Fix the indentation in readPIRPin() so it matches the project’s 2-space C++ style: the digitalRead(pin) assignment is indented one level too deep relative to the surrounding if and return statements. Update the readPIRPin function in Animated_Staircase.cpp to use consistent two-space indentation with no tabs, keeping the logic unchanged.Sources: Coding guidelines, Path instructions
🤖 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.
Outside diff comments:
In `@usermods/Animated_Staircase/Animated_Staircase.cpp`:
- Around line 334-342: The opacity reset in Animated_Staircase::loop (the block
using getMainSegmentId, getLastActiveSegmentId, and setOpacity(255)) is now
running even when sentinel mode is disabled. Gate this first/last segment
restore behind enabledSentinel so the existing custom opacity is preserved
unless sentinel mode is actually active.
- Around line 125-130: The sentinel rendering branch in Animated_Staircase.cpp
is treating sentinelDimOpacity as “disabled” when it equals 0, but 0 is a valid
persisted opacity. Update the conditional around the sentinel path in the
segment update logic so enabledSentinel and isSentinel still keep the segment on
even when sentinelDimOpacity is 0, and always apply
seg.setOpacity(sentinelDimOpacity) for that branch.
---
Nitpick comments:
In `@usermods/Animated_Staircase/Animated_Staircase.cpp`:
- Around line 218-221: The comment around lastSensor assignment conflicts with
the LOWER/UPPER sensor naming and can mislead future changes. Update the comment
near the lastSensor = bottomSensorRead assignment in Animated_Staircase so it
simply says lastSensor mirrors bottomSensorRead, matching the actual stored
value and avoiding any implied swipe-direction semantics.
- Around line 505-506: Remove the unused invert snapshot variables in
readFromConfig(), since oldTopAPinInvert and oldBottomAPinInvert are assigned
but never read. Update the Animated_Staircase::readFromConfig() logic to delete
these dead declarations unless you actually need them for a change-detection
comparison, and keep the function focused on the values it uses.
- Around line 174-177: Fix the indentation in readPIRPin() so it matches the
project’s 2-space C++ style: the digitalRead(pin) assignment is indented one
level too deep relative to the surrounding if and return statements. Update the
readPIRPin function in Animated_Staircase.cpp to use consistent two-space
indentation with no tabs, keeping the logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81f30a77-61af-4362-88d8-63b077a30608
📒 Files selected for processing (1)
usermods/Animated_Staircase/Animated_Staircase.cpp
| // We may need to copy mode and colors from segment 0 to make sure | ||
| // changes are propagated even when the config is changed during a wipe | ||
| // seg.setMode(mainsegment.mode); | ||
| // seg.setColor(0, mainsegment.colors[0]); |
There was a problem hiding this comment.
what's the reason for deleting this comment? Is it wrong or maybe outdated?
There was a problem hiding this comment.
It's outdated. Obviously used in an old version of the usermod.
4 participants
Added a new option to the UI to activate the first and last segment as a sentinel segment - if activated both segments will stay active with the configured opacity
Fixed the swipe off direction so that the swipe always goes off to the direction of the last triggered sensor.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes