Usermod Animated Staircase: added sentinel option and fixed off swipe direction #5076
Conversation
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)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (6)📚 Learning: 2025-09-12T17:29:43.826ZApplied to files:
📚 Learning: 2025-06-07T15:58:42.579ZApplied to files:
📚 Learning: 2025-04-24T09:31:06.879ZApplied to files:
📚 Learning: 2025-04-24T09:31:06.879ZApplied to files:
📚 Learning: 2025-10-20T09:41:50.374ZApplied to files:
📚 Learning: 2025-09-02T19:42:57.475ZApplied to files:
🔇 Additional comments (5)
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
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/Animated_Staircase/Animated_Staircase.cpp (1)
242-244: OFF swipe direction is still inverted.The negation in
swipe = !lastSensorinverts the swipe direction relative to the last triggered sensor. Based on the comment at lines 208-211:
lastSensor == truemeans bottom sensor triggered (should swipe UP on OFF)lastSensor == falsemeans top sensor triggered (should swipe DOWN on OFF)But the negation produces the opposite behavior.
Apply this diff to fix the inversion:
- // Swipe OFF in the direction of the last sensor detection - swipe = !lastSensor; + // Swipe OFF in the direction of the last sensor detection + swipe = lastSensor;Or for explicit clarity:
- // Swipe OFF in the direction of the last sensor detection - swipe = !lastSensor; + // Swipe OFF in the direction of the last sensor detection + swipe = lastSensor ? SWIPE_UP : SWIPE_DOWN;
🧹 Nitpick comments (1)
usermods/Animated_Staircase/Animated_Staircase.cpp (1)
28-29: Sentinel configuration fields look good.The new fields are properly initialized with sensible defaults. Consider adding validation in
readFromConfigto clampsentinelDimOpacityto the valid range [0, 255] for clarity, though the current behavior (allowing 0 to disable dimmed sentinels) may be intentional.
📜 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 (6)
📚 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
🔇 Additional comments (5)
usermods/Animated_Staircase/Animated_Staircase.cpp (5)
110-136: Sentinel segment logic is well-implemented.The sentinel feature correctly identifies first and last segments, keeps them active when sentinel mode is enabled, and properly manages opacity transitions:
- Sentinel segments are fully lit (255) when part of the active swipe
- Sentinel segments are dimmed (
sentinelDimOpacity) when outside the swipe range- Non-sentinel segments behave as before
208-212: Sensor tracking update is correct.Recording
lastSensorunconditionally (regardless ofonstate) ensures the OFF swipe can follow the last triggered sensor direction. The comment clearly documents the mapping.
324-336: Sentinel restoration logic on disable is correct.When disabling the usermod, the code properly restores first and last segment opacity to full brightness (255), ensuring a clean state. The segment ID calculation is consistent with
updateSegments(), and defensive checks for active segments prevent errors.
478-479: Configuration persistence is properly implemented.The sentinel settings are correctly persisted to/from config with appropriate change detection. Calling
updateSegments()when sentinel configuration changes (line 555) efficiently applies the new settings without requiring a fullsetup()cycle.Also applies to: 529-531, 554-558
599-600: PROGMEM string declarations are appropriate.The new config keys follow the established pattern of storing strings in PROGMEM to conserve RAM, consistent with the rest of the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/Animated_Staircase/Animated_Staircase.cpp (1)
242-244: OFF swipe direction is inverted.The past review comment correctly identified this issue:
swipe = !lastSensorinverts the swipe direction. When the bottom sensor was the last triggered (lastSensor == true), the code setsswipe = false(SWIPE_DOWN), but it should swipe UP to match the direction of the last sensor.Apply this fix:
- // Swipe OFF in the direction of the last sensor detection - swipe = !lastSensor; + // Swipe OFF in the direction of the last sensor detection + swipe = lastSensor;
📜 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 (6)
📚 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-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-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
🔇 Additional comments (5)
usermods/Animated_Staircase/Animated_Staircase.cpp (5)
28-29: LGTM!The new configuration variables for the sentinel feature are well-defined with sensible defaults.
208-211: LGTM!The mapping of
lastSensorfrombottomSensorReadis clearly documented and logically sound for tracking which sensor triggered last.
324-336: LGTM!The disable path correctly restores sentinel segments to full opacity with proper active-segment guards.
478-479: LGTM!Configuration persistence follows WLED conventions correctly, with proper change detection and segment updates only after initialization.
Also applies to: 529-532, 554-554, 557-557
96-97: LGTM!The new configuration string declarations follow the established PROGMEM pattern for flash memory optimization.
Also applies to: 598-599
|
@realrolfje any comments on this? |
2 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