Migrated AnimatedStaircase usermod to VL53L0X TOF sensors#5656
Open
Hoverman1977 wants to merge 12 commits into
Open
Migrated AnimatedStaircase usermod to VL53L0X TOF sensors#5656Hoverman1977 wants to merge 12 commits into
Hoverman1977 wants to merge 12 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new WLED usermod AnimatedStaircase_VL53L0X that uses two VL53L0X distance sensors for swipe detection and segmented staircase animations, with configurable XSHUT sequencing, background polling, JSON/MQTT state I/O, lifecycle controls, documentation, and PlatformIO/library manifests. ChangesVL53L0X Staircase Animation Usermod
Documentation and Build Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp (2)
64-69: ⚡ Quick winVerify the fixed MQTT buffers are large enough.
Both paths build on
char subuf[64]with unboundedsprintf_P/strcpy+strcat_P. IfmqttDeviceTopicever grows past the expected limit, this becomes a stack overwrite. Please confirm the upstream max length leaves room for"/motion/%d"and"/swipe", or switch to bounded APIs so the safety invariant lives in this file.Also applies to: 103-109
🤖 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/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp` around lines 64 - 69, The publishMqtt routine uses a fixed stack buffer char subuf[64] and unbounded sprintf_P/strcpy/strcat_P which can overflow if mqttDeviceTopic grows; change publishMqtt (and the similar code around where subuf is used again) to build the topic with bounded APIs or a length check: compute required length (strlen(mqttDeviceTopic) + room for "/motion/%d" or "/swipe" + null), and either allocate dynamically or use snprintf/strncpy/strncat (or ensure the buffer constant is increased safely) so you never write past subuf; update both occurrences that reference subuf and mqttDeviceTopic to follow this bounded pattern.
28-34: ⚡ Quick winReplace these
#definebooleans with scoped constants.
SWIPE_UP,SWIPE_DOWN,LOWER, andUPPERare still file-global after preprocessing, so they leak outside the class and lose type checking.static constexpr boolor a small enum keeps them scoped and matches the repo rule here.As per coding guidelines, "Prefer
constexprover#definefor compile-time constants in C++."🤖 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/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp` around lines 28 - 34, Replace the four preprocessor booleans (SWIPE_UP, SWIPE_DOWN, LOWER, UPPER) with scoped compile-time constants to avoid global leakage and preserve type safety: remove the `#define` lines and declare either static constexpr bool SWIPE_UP/ SWIPE_DOWN and static constexpr bool LOWER/ UPPER in the relevant class/namespace or define small scoped enums (e.g., enum class Swipe { Up, Down } and enum class SensorPos { Lower, Upper }) and update the variables swipe and lastSensor to use the new scoped constants/types (variables named swipe and lastSensor in AnimatedStaircase_VL53L0X.cpp). Ensure all references are updated to the new names/types.
🤖 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 `@platformio.ini`:
- Around line 13-35: The change narrows the PlatformIO default build matrix by
editing the default_envs list (e.g., removing many envs and leaving esp32dev),
which affects CI; restore or document the intended CI behavior and obtain
explicit maintainer approval before merging. Revert the default_envs to the
previous matrix or add a clear comment in platformio.ini explaining the
intentional change and its CI impact, then get an explicit approval from a WLED
maintainer/org member (record the approval in the PR description) referencing
the modified default_envs and the retained envs like esp32dev so reviewers can
verify the intended scope.
In `@usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp`:
- Around line 474-475: The assignments to topThresholdMM and bottomThresholdMM
are taking client JSON values directly (via top[FPSTR(_topThreshold)] /
top[FPSTR(_bottomThreshold)]) and must be range-clamped before storing so
malformed/negative/oversized inputs can't break checkSensors(); extract the
numeric value, validate it is numeric, clamp it to the sensor's valid range
(e.g., 0..SENSOR_MAX_DISTANCE_MM or define a constant like
VL53L0X_MAX_DISTANCE_MM) and then assign the clamped value to topThresholdMM and
bottomThresholdMM (replace the current bitwise-or pattern), ensuring the new
logic is used wherever these thresholds are read (e.g., checkSensors()).
- Around line 178-193: The init failure paths in initialiseSensors() leave
partial hardware state (bottom/top sensors running, XSHUT pins held) before
returning; modify the two failure branches after bottomVL53.init() and
topVL53.init() to explicitly stop and reset any sensor started so far (e.g.,
call bottomVL53.stopContinuous()/reset or power-cycle via
digitalWrite(xshutBottomPin, LOW) and digitalWrite(xshutTopPin, LOW) as
appropriate), clear any I2C address assignments (if applicable), and
release/deallocate pins (the same cleanup resetSensorState() does plus explicit
pin power-down) before returning false so subsequent retries and
allocateMultiplePins() see a clean state. Ensure references to bottomVL53,
topVL53, xshutTopPin, xshutBottomPin, setAddress, startContinuous, and
resetSensorState() are used to locate and implement the unwind logic.
- Around line 221-243: The background task started by ensureTask() accesses and
mutates shared usermod state (fields like enabled, configLoaded,
sensorsInitialised, on/off indices, timers, on, swipe) and calls into
strip/segment APIs (strip.get*, strip.getSegment, setOption, setTransition,
trigger, toggleOnOff) without synchronization, causing races with main-context
methods (readFromConfig(), readFromJsonState(), enable(), cleanup(), MQTT
handlers). Fix by adding a single FreeRTOS mutex (e.g., usermodMutex) or a
task-level serialization mechanism: take the mutex before any reads/writes or
strip/segment calls in the task loop (the while(true) block that calls
initialiseSensors(), checkSensors(), autoPowerOff(), updateSwipe(), and when
computing minSegmentId/maxSegmentId), and also take the same mutex in
readFromConfig(), readFromJsonState(), enable(), cleanup(), and any MQTT
handlers or functions that call initialiseSensors() or strip.* so those
operations are serialized; alternatively suspend the task
(vTaskSuspend/vTaskResume using the task handle created in ensureTask()) around
large critical sections in enable()/cleanup() if fine-grained locking is
infeasible. Ensure the mutex is created once during initialization and always
released on every code path, and prefer using the mutex rather than critical
sections to avoid blocking other WLED internals.
In `@usermods/AnimatedStaircase_VL53L0X/README.md`:
- Around line 33-36: The README's install steps omit activating the
usermod—update the "Before compiling" section to instruct users to add
AnimatedStaircase_VL53L0X to the custom_usermods list in their PlatformIO build
configuration (e.g., in platformio_override.ini or platformio.ini) so the
usermod is actually enabled; reference the exact name
"AnimatedStaircase_VL53L0X" and the key "custom_usermods" and show the placement
(build environment section) where it should be added.
- Line 17: Update the README to stop calling XSHUT pins optional: explicitly
state that both XSHUT pins are mandatory and must be assigned to different GPIOs
(they cannot be the same), because initialiseSensors() in
AnimatedStaircase_VL53L0X.cpp requires both pins to be set for I²C address
assignment and will disable the usermod if either is unset; revise the relevant
lines (the earlier pin description and the examples/usage sections) to reflect
“required” and “must be different” wording so users configure two distinct XSHUT
pins before running initialiseSensors().
---
Nitpick comments:
In `@usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp`:
- Around line 64-69: The publishMqtt routine uses a fixed stack buffer char
subuf[64] and unbounded sprintf_P/strcpy/strcat_P which can overflow if
mqttDeviceTopic grows; change publishMqtt (and the similar code around where
subuf is used again) to build the topic with bounded APIs or a length check:
compute required length (strlen(mqttDeviceTopic) + room for "/motion/%d" or
"/swipe" + null), and either allocate dynamically or use
snprintf/strncpy/strncat (or ensure the buffer constant is increased safely) so
you never write past subuf; update both occurrences that reference subuf and
mqttDeviceTopic to follow this bounded pattern.
- Around line 28-34: Replace the four preprocessor booleans (SWIPE_UP,
SWIPE_DOWN, LOWER, UPPER) with scoped compile-time constants to avoid global
leakage and preserve type safety: remove the `#define` lines and declare either
static constexpr bool SWIPE_UP/ SWIPE_DOWN and static constexpr bool LOWER/
UPPER in the relevant class/namespace or define small scoped enums (e.g., enum
class Swipe { Up, Down } and enum class SensorPos { Lower, Upper }) and update
the variables swipe and lastSensor to use the new scoped constants/types
(variables named swipe and lastSensor in AnimatedStaircase_VL53L0X.cpp). Ensure
all references are updated to the new names/types.
🪄 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: 1adbefd6-8bd4-49b5-b998-a46cec06297b
📒 Files selected for processing (4)
platformio.iniusermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cppusermods/AnimatedStaircase_VL53L0X/README.mdusermods/AnimatedStaircase_VL53L0X/library.json
Comment on lines
+13
to
+35
| default_envs = #nodemcuv2 | ||
| #esp8266_2m | ||
| #esp01_1m_full | ||
| #nodemcuv2_160 | ||
| #esp8266_2m_160 | ||
| #esp01_1m_full_160 | ||
| #nodemcuv2_compat | ||
| #esp8266_2m_compat | ||
| #esp01_1m_full_compat | ||
| esp32dev | ||
| esp32dev_debug | ||
| esp32_eth | ||
| esp32_wrover | ||
| lolin_s2_mini | ||
| esp32c3dev | ||
| esp32c3dev_qio | ||
| esp32S3_wroom2 | ||
| esp32s3dev_16MB_opi | ||
| esp32s3dev_8MB_opi | ||
| esp32s3dev_8MB_qspi | ||
| esp32s3dev_8MB_none | ||
| esp32s3_4M_qspi | ||
| usermods | ||
| #esp32dev_debug | ||
| #esp32_eth | ||
| #esp32_wrover | ||
| #lolin_s2_mini | ||
| #esp32c3dev | ||
| #esp32c3dev_qio | ||
| #esp32S3_wroom2 | ||
| #esp32s3dev_16MB_opi | ||
| #esp32s3dev_8MB_opi | ||
| #esp32s3dev_8MB_qspi | ||
| #esp32s3dev_8MB_none | ||
| #esp32s3_4M_qspi | ||
| #usermods |
Contributor
There was a problem hiding this comment.
PlatformIO default build matrix was narrowed; this must get explicit maintainer approval.
Changing [platformio].default_envs here directly changes CI build coverage (the workflow reads this list and builds exactly those envs), and platformio.ini edits are policy-gated. Please get explicit approval from a maintainer/WLED org member before merge, and confirm the reduced default matrix is intentional for CI/release behavior.
As per coding guidelines: “platformio.ini modifications MUST be approved explicitly by a maintainer or WLED organisation Member; always flag such modifications.”
🤖 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 `@platformio.ini` around lines 13 - 35, The change narrows the PlatformIO
default build matrix by editing the default_envs list (e.g., removing many envs
and leaving esp32dev), which affects CI; restore or document the intended CI
behavior and obtain explicit maintainer approval before merging. Revert the
default_envs to the previous matrix or add a clear comment in platformio.ini
explaining the intentional change and its CI impact, then get an explicit
approval from a WLED maintainer/org member (record the approval in the PR
description) referencing the modified default_envs and the retained envs like
esp32dev so reviewers can verify the intended scope.
Member
There was a problem hiding this comment.
This change is a no-go for merging, sorry. Please revert, and leave platformio.ini untouched.
Contributor
There was a problem hiding this comment.
@softhack007, understood — thank you for the clear confirmation.
@Hoverman1977, please revert all changes to platformio.ini and leave the file untouched. Your usermod additions in usermods/AnimatedStaircase_VL53L0X/ and the platformio_override_example.ini within the usermod folder are the appropriate place for build configuration specific to this usermod, and those don't need any edits to the root platformio.ini.
(ᴗ_ ᴗ。)
🧠 Learnings used
Learnt from: netmindz
Repo: wled/WLED PR: 5311
File: platformio.ini:395-395
Timestamp: 2026-01-31T18:19:28.320Z
Learning: In platformio.ini for WLED, understand that usermods can be pulled as out-of-tree dependencies via lib_deps (external repositories). The custom_usermods setting controls only in-tree usermods; setting custom_usermods = (empty) disables in-tree usermods but does not disable out-of-tree usermods included through library dependencies.
Learnt from: softhack007
Repo: wled/WLED PR: 5048
File: platformio.ini:486-491
Timestamp: 2026-02-10T13:52:36.924Z
Learning: In PlatformIO projects, prefer using build_unflags in the PlatformIO configuration to remove inherited -D defines rather than using GCC's -U option. The -U approach is order-dependent and may be unreliable if the build command order is not guaranteed by PlatformIO. Use build_unflags in platformio.ini to explicitly remove inherited build flags for more robust and maintainable builds.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:788-850
Timestamp: 2026-03-31T13:31:01.488Z
Learning: For PlatformIO projects using `platform = pioarduino/platform-espressif32` with dual frameworks (`framework = arduino, espidf`), do not require an explicit `board_build.sdkconfig = sdkconfig.<env_name>` setting. `sdkconfig.<env_name>` (e.g., `sdkconfig.esp32s3_matter_wifi`) is a per-environment build artifact generated by pioarduino from merged board defaults plus the project-root `sdkconfig.defaults`. The project-root `sdkconfig.defaults` is automatically loaded by ESP-IDF’s CMake build, so the absence of `board_build.sdkconfig` should not be flagged as a missing configuration.
Learnt from: softhack007
Repo: wled/WLED PR: 5456
File: platformio.ini:788-850
Timestamp: 2026-03-31T13:31:01.488Z
Learning: For PlatformIO projects using `pioarduino/platform-espressif32` with dual framework (`framework = arduino, espidf`), treat `sdkconfig.<env_name>` (e.g., `sdkconfig.esp32s3_matter_wifi`) as a per-environment build artifact generated in the project root. PlatformIO will create it from `sdkconfig.defaults` on first build if it doesn’t exist, and will reuse the existing file on subsequent builds. In code review, do not recommend setting `board_build.sdkconfig = sdkconfig.<env_name>` because that file is an output, not required input. Only use `board_build.cmake_extra_args = -DSDKCONFIG_DEFAULTS=...` to select different defaults files when different environments truly need different SDK config defaults. As a sanity check, if `sdkconfig.<env_name>` is deleted and a rebuild regenerates it with the same content, that indicates `sdkconfig.defaults` is the correct source of truth.
Learnt from: netmindz
Repo: wled/WLED PR: 5543
File: platformio.ini:133-136
Timestamp: 2026-04-26T09:03:12.551Z
Learning: For WLED platformio.ini files: do not mark the audioreactive usermod as “unsupported” for ESP8266 build environments. While ESP8266 can’t process audio locally, it may still receive audioreactive sync data over the network (e.g., from an ESP32 or other source), so the outdated readme note about “ESP8266 is not supported” should be interpreted as only local audio processing. Reviewers should allow audioreactive to be included in ESP8266 environments in platformio.ini when network sync-receive is being used.
Comment on lines
+221
to
+243
| while (true) { | ||
| vTaskDelayUntil(&xLastWakeTime, xFrequency); | ||
|
|
||
| if (!self || !self->enabled) continue; | ||
| if (!self->configLoaded) continue; | ||
|
|
||
| if (!self->sensorsInitialised) { | ||
| self->initialiseSensors(); | ||
| continue; | ||
| } | ||
|
|
||
| if (!self->initDone || strip.getMaxSegments() == 0) continue; | ||
| if (strip.getSegment(0).stop == 0) continue; | ||
|
|
||
| self->minSegmentId = strip.getMainSegmentId(); | ||
| self->maxSegmentId = strip.getLastActiveSegmentId() + 1; | ||
|
|
||
| self->checkSensors(); | ||
|
|
||
| if (self->on) self->autoPowerOff(); | ||
|
|
||
| self->updateSwipe(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp"
echo "== File existence =="
ls -l "$FILE"
echo "== Line range around 221-243 =="
nl -ba "$FILE" | sed -n '180,280p'
echo "== Find task creation / handle(s) =="
rg -n "xTaskCreate|xTaskCreatePinnedToCore|vTaskDelayUntil|xLastWakeTime|task|TaskHandle_t" "$FILE"
echo "== Inspect shared state fields & methods called in loop =="
rg -n "checkSensors\\(|initialiseSensors\\(|updateSwipe\\(|autoPowerOff\\(|toggleOnOff\\(|readFromConfig\\(|readFromJsonState\\(|enable\\(|cleanup\\(" "$FILE"
echo "== Inspect implementations of those methods (selected ranges) =="
for sym in initialiseSensors checkSensors updateSwipe autoPowerOff toggleOnOff readFromConfig readFromJsonState enable cleanup; do
echo "--- $sym ---"
# show the first ~120 lines around match
nl -ba "$FILE" | rg -n "^\s*[0-9]+\s+.*$sym\\(" | head -n 5 || true
done
echo "== Show broader context for the ingress/mutation methods =="
for section in "readFromConfig" "readFromJsonState" "enable" "cleanup"; do
echo "### $section context"
line=$(nl -ba "$FILE" | rg -n "^\s*[0-9]+\s+.*$section\\(" | head -n1 | awk '{print $1}' || true)
if [[ -n "${line:-}" ]]; then
start=$((line-60))
end=$((line+120))
nl -ba "$FILE" | sed -n "${start},${end}p"
fi
done
echo "== Search for any mutex/semaphore/queue usage =="
rg -n "SemaphoreHandle_t|xSemaphore|mutex|Mutex|xQueue|QueueHandle_t|xQueueCreate|xSemaphoreCreate|portENTER_CRITICAL|taskENTER_CRITICAL" "$FILE"Repository: wled/WLED
Length of output: 283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp"
echo "== Line range 180-280 (with numbers via python) =="
python3 - <<'PY'
import itertools
path = "usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp"
start, end = 180, 280
with open(path, "r", encoding="utf-8", errors="replace") as f:
for i, line in enumerate(f, 1):
if start <= i <= end:
print(f"{i:5d} {line.rstrip()}")
if i > end:
break
PY
echo "== Find task creation / loop primitives in file =="
rg -n "xTaskCreate|xTaskCreatePinnedToCore|vTaskDelayUntil|xLastWakeTime|TaskHandle_t|while \\(true\\)" "$FILE" || true
echo "== Find loop methods/entrypoints that likely touch shared state =="
rg -n "checkSensors\\(|initialiseSensors\\(|updateSwipe\\(|autoPowerOff\\(|toggleOnOff\\(|readFromConfig\\(|readFromJsonState\\(|enable\\(|cleanup\\(" "$FILE" || true
echo "== Show mutex/semaphore/queue usage in file =="
rg -n "SemaphoreHandle_t|xSemaphore|mutex|Mutex|xQueue|QueueHandle_t|xQueueCreate|xSemaphoreCreate|portENTER_CRITICAL|taskENTER_CRITICAL" "$FILE" || trueRepository: wled/WLED
Length of output: 4247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp"
python3 - <<'PY'
path = "usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp"
ranges = [(1,120),(120,190),(190,270),(270,420),(420,520),(520,620)]
with open(path,"r",encoding="utf-8",errors="replace") as f:
lines = f.readlines()
for a,b in ranges:
print(f"\n===== {a}-{b} =====")
for i in range(a,b+1):
if i-1 < 0 or i-1 >= len(lines): break
print(f"{i:5d} {lines[i-1].rstrip()}")
PYRepository: wled/WLED
Length of output: 18124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp"
rg -n "SemaphoreHandle_t|xSemaphore|xSemaphoreCreate|Mutex|mutex|xQueue|QueueHandle_t|xQueueCreate|portENTER_CRITICAL|taskENTER_CRITICAL|vTaskSuspend|vTaskResume|uxTaskPriority|ulTaskNotify|xTaskNotify" "$FILE" || true
echo "== Check if cleanup/enable pause the task =="
rg -n "cleanup\\(|enable\\(|vTaskDelete|taskHandle|ensureTask\\(" "$FILE" || trueRepository: wled/WLED
Length of output: 510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp"
echo "== FreeRTOS sync primitives used in file =="
rg -n "SemaphoreHandle_t|xSemaphore|xSemaphoreCreate|QueueHandle_t|xQueueCreate|portENTER_CRITICAL|taskENTER_CRITICAL|vTaskSuspend|vTaskResume|xTaskNotify|xTaskNotifyFromISR|xTaskCreate(.*)Notify" "$FILE" || true
echo "== initialiseSensors() call sites =="
rg -n "initialiseSensors\\(" "$FILE"Repository: wled/WLED
Length of output: 272
The background task needs a synchronization boundary.
In usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp, the FreeRTOS task created in ensureTask() reads/writes usermod fields (enabled, configLoaded, sensor read/write/state, onIndex/offIndex, timers, on, swipe, etc.) and calls into WLED/strip/global logic (strip.get*/setTransition/trigger, segment setOption(), stateChanged/colorUpdated, and toggleOnOff()), all without any synchronization. At the same time, main-context entrypoints (readFromConfig(), readFromJsonState(), enable(), cleanup(), and MQTT handler via enable()) mutate the same shared fields and also call initialiseSensors() and strip.* directly, creating real race conditions (including concurrent strip/segment updates and pin/sensor re-init). Add a mutex/queue (or otherwise serialize/suspend the task during config/enable/cleanup changes) so shared state and all strip/WLED calls are not accessed concurrently.
🤖 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/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp` around
lines 221 - 243, The background task started by ensureTask() accesses and
mutates shared usermod state (fields like enabled, configLoaded,
sensorsInitialised, on/off indices, timers, on, swipe) and calls into
strip/segment APIs (strip.get*, strip.getSegment, setOption, setTransition,
trigger, toggleOnOff) without synchronization, causing races with main-context
methods (readFromConfig(), readFromJsonState(), enable(), cleanup(), MQTT
handlers). Fix by adding a single FreeRTOS mutex (e.g., usermodMutex) or a
task-level serialization mechanism: take the mutex before any reads/writes or
strip/segment calls in the task loop (the while(true) block that calls
initialiseSensors(), checkSensors(), autoPowerOff(), updateSwipe(), and when
computing minSegmentId/maxSegmentId), and also take the same mutex in
readFromConfig(), readFromJsonState(), enable(), cleanup(), and any MQTT
handlers or functions that call initialiseSensors() or strip.* so those
operations are serialized; alternatively suspend the task
(vTaskSuspend/vTaskResume using the task handle created in ensureTask()) around
large critical sections in enable()/cleanup() if fine-grained locking is
infeasible. Ensure the mutex is created once during initialization and always
released on every code path, and prefer using the mutex rather than critical
sections to avoid blocking other WLED internals.
coderabbitai
Bot
added
the
documentation
softhack007
reviewed
May 30, 2026
softhack007
added
the
needs_rework
softhack007
requested changes
May 30, 2026
Comment on lines
+13
to
+35
| default_envs = #nodemcuv2 | ||
| #esp8266_2m | ||
| #esp01_1m_full | ||
| #nodemcuv2_160 | ||
| #esp8266_2m_160 | ||
| #esp01_1m_full_160 | ||
| #nodemcuv2_compat | ||
| #esp8266_2m_compat | ||
| #esp01_1m_full_compat | ||
| esp32dev | ||
| esp32dev_debug | ||
| esp32_eth | ||
| esp32_wrover | ||
| lolin_s2_mini | ||
| esp32c3dev | ||
| esp32c3dev_qio | ||
| esp32S3_wroom2 | ||
| esp32s3dev_16MB_opi | ||
| esp32s3dev_8MB_opi | ||
| esp32s3dev_8MB_qspi | ||
| esp32s3dev_8MB_none | ||
| esp32s3_4M_qspi | ||
| usermods | ||
| #esp32dev_debug | ||
| #esp32_eth | ||
| #esp32_wrover | ||
| #lolin_s2_mini | ||
| #esp32c3dev | ||
| #esp32c3dev_qio | ||
| #esp32S3_wroom2 | ||
| #esp32s3dev_16MB_opi | ||
| #esp32s3dev_8MB_opi | ||
| #esp32s3dev_8MB_qspi | ||
| #esp32s3dev_8MB_none | ||
| #esp32s3_4M_qspi | ||
| #usermods |
Member
There was a problem hiding this comment.
This change is a no-go for merging, sorry. Please revert, and leave platformio.ini untouched.
softhack007
reviewed
May 30, 2026
softhack007
reviewed
May 30, 2026
|
|
||
| strip.trigger(); | ||
| stateChanged = true; | ||
| colorUpdated(CALL_MODE_DIRECT_CHANGE); |
Member
There was a problem hiding this comment.
Don't call this function in a background task. Please use a volatile synchronisation flag (or mutex, queue), and move colorUpdated() into the main usermod loop().
softhack007
reviewed
May 30, 2026
| if (bottomSensorRead != bottomSensorState) { | ||
| bottomSensorState = bottomSensorRead; | ||
| sensorChanged = true; | ||
| publishMqtt(true, bottomSensorState ? "on" : "off"); |
Member
There was a problem hiding this comment.
Don't call this function in a background task. Please use a volatile synchronisation flag (or mutex, queue), and move publishMqtt() into the main usermod loop().
Member
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp`:
- Around line 422-437: In cleanup() (and the corresponding config-pin-change
path that currently calls PinManager::deallocatePin around the
xshutTopPin/xshutBottomPin code), assert XSHUT by driving each valid xshutTopPin
and xshutBottomPin low before calling PinManager::deallocatePin so the VL53L0X
sensors are powered down and I2C state is clean; keep the existing checks
(xshutTopPin >= 0 / xshutBottomPin >= 0), perform the GPIO write/drive-low via
the PinManager API used elsewhere in the code, then call
PinManager::deallocatePin and finally resetSensorState() as currently done.
- Around line 366-397: When toggling enable in enable(), reset the runtime swipe
state so a disable/enable cycle doesn't resume a previous swipe or immediately
auto-power-off: explicitly clear the swipe/run flags (set on = false), reset
index bounds (onIndex/offIndex and minSegmentId/maxSegmentId as needed), clear
any sensor latched states created by initialiseSensors() (reset per-sensor latch
flags) and cancel/reinitialize any swipe/power-off timers before returning; do
this in the enable(false) branch (and also ensure enable(true)
initializes/clears these values before starting a new run) so stateChanged and
colorUpdated(CALL_MODE_DIRECT_CHANGE) operate from a clean runtime state.
- Around line 214-251: The call to xTaskCreatePinnedToCore inside ensureTask()
ignores its return value so if task creation fails the usermod stays enabled
without a worker; capture the returned BaseType_t result from
xTaskCreatePinnedToCore (for the task named "StairVL53Task"), check if it is
pdPASS, and if not handle the failure by logging the error, clearing/setting
taskHandle appropriately and disabling the usermod (self->enabled = false) or
scheduling a retry; update references to taskHandle and ensure any subsequent
logic (e.g., check of taskHandle) behaves correctly when task creation fails.
🪄 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: 4fe14170-3045-47e5-a34e-e2f963e8ff84
📒 Files selected for processing (5)
platformio.iniusermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cppusermods/AnimatedStaircase_VL53L0X/README.mdusermods/AnimatedStaircase_VL53L0X/library.jsonusermods/AnimatedStaircase_VL53L0X/platformio_override_example.ini
softhack007
reviewed
May 30, 2026
| self->minSegmentId = strip.getMainSegmentId(); | ||
| self->maxSegmentId = strip.getLastActiveSegmentId() + 1; | ||
|
|
||
| self->checkSensors(); |
Member
There was a problem hiding this comment.
Your design of using a separate task is good to ensure swift responses to sensor changes, but I'm still wondering if its "overkill" for the purpose of this usermod. The core of the task function is that it runs self->checkSensors() every 10 millis. Calling self->autoPowerOff() and self->updateSwipe() is not safe inside the task function, because this changes segment state, triggers UDP and MQTT etc - all without being synchronized with the main wled loop.
So you'd need to add semaphores and move the function call that interact with the wled core back into the usermod::loop() context, otherwise it won't work reliably.
Author
There was a problem hiding this comment.
I think that has been done now in the latest commit - this is my first usermod - and first WLED PR - my testing had been successful locally, but I can only test so much. I did find that it took too long to turn on all the segments when I set the tickrate to a slower iteration which is why I settled on 10.
Member
|
General comment: this PR lacks a description, please add it. The description should also provide some design rationale, especially why you chose to implement a worker task for polling the sensors. https://github.com/wled/WLED?tab=contributing-ov-file#describing-your-pr |
softhack007
reviewed
May 30, 2026
softhack007
reviewed
May 30, 2026
| AnimatedStaircase_VL53L0X | ||
| lib_deps = | ||
| ${env:esp32dev.lib_deps} | ||
| pololu/VL53L0X @ ^1.3.0 |
Member
There was a problem hiding this comment.
this line should not be necessary => move dependencies into library.json
softhack007
reviewed
May 30, 2026
| pololu/VL53L0X @ ^1.3.0 | ||
| build_flags = | ||
| ${env:esp32dev.build_flags} | ||
| -D USERMOD_ANIMATED_STAIRCASE_VL53L0X No newline at end of file |
Member
There was a problem hiding this comment.
normally not needed, either.
…e to custom_usermods. Renamed platformio_override_example.ini as suggested to platformio_override.ini.sample
…tedstaircase-usermod' into Hoverman1977/usermod_AnimatedStaircase_VL53L0X
…h effect when turning on segments - with a large staircase (60 segments) it takes 600ms to turn on at 10ms which is acceptable.
Contributor
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 (1)
usermods/AnimatedStaircase_VL53L0X/README.md (1)
117-234:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the duplicated README content.
The entire README from line 1 is duplicated starting at line 117. This appears to be an accidental copy-paste or merge error. The second copy should be completely removed.
Note: The second copy has slightly better installation instructions at lines 151-153 (mentioning
custom_usermods). Preserve that improvement in the first copy before removing the duplicate.📝 Proposed fix
Remove lines 117-234 entirely, and update line 35 in the first copy to include the custom_usermods instruction from line 151:
Before compiling, you have to make the following modifications: -Copy platformio_override_example.ini to the same folder as platformio.ini +In order to add the usermod to WLED the custom_usermods section in platformio_override.ini needs to include this usermod. + +- Copy platformio_override.ini.sample to the same folder as platformio.ini and rename to platformio_override.iniThen delete all of lines 117-234.
🤖 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/AnimatedStaircase_VL53L0X/README.md` around lines 117 - 234, Remove the duplicated README block (the repeated copy starting mid-file) so only a single README remains; before deleting the duplicate, copy the improved installation sentence about adding the usermod to the custom_usermods section (the line that reads like "In order to add the usermod to WLED the custom_usermods section in platformio_override.ini needs to include this usermod.") into the original Installation/Prerequisites section (the first copy's install text), then delete the entire duplicated block that follows the original so the README contains only the consolidated, non-duplicated content.
🤖 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/AnimatedStaircase_VL53L0X/README.md`:
- Around line 117-234: Remove the duplicated README block (the repeated copy
starting mid-file) so only a single README remains; before deleting the
duplicate, copy the improved installation sentence about adding the usermod to
the custom_usermods section (the line that reads like "In order to add the
usermod to WLED the custom_usermods section in platformio_override.ini needs to
include this usermod.") into the original Installation/Prerequisites section
(the first copy's install text), then delete the entire duplicated block that
follows the original so the README contains only the consolidated,
non-duplicated content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00d79424-96f0-46a0-bdcc-3770f463e5aa
📒 Files selected for processing (3)
usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cppusermods/AnimatedStaircase_VL53L0X/README.mdusermods/AnimatedStaircase_VL53L0X/platformio_override.ini.sample
💤 Files with no reviewable changes (1)
- usermods/AnimatedStaircase_VL53L0X/platformio_override.ini.sample
added 2 commits
May 31, 2026 11:19
…ented sensor shutdown function that gracefully shuts down sensors and deallocates pins.
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
2 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.
Usermod
AnimatedStaircase_VL53L0X
A modification of the AnimatedStaircase mod to be able to use VL53L0X I2C sensors instead of the ultrasonic sensors.
Features
Design rationale
I found that without a worker task the UI and WLED were extremely unresponsive as it appears as though the (continuous) reading of the sensors blocks other code from running. Implementing a worker tasks solved this issue.
I had found that 10ms would allow for large staircase's with many segments to light up smoothly (60 segments at 10ms per segment is 600ms) for the whole staircase. 10ms would also allow for smaller staircases as well as large ones.
When testing at 100ms it was taking 6 seconds or so to light up the entire staircase and I found the most effective lower limit was 10ms where the usermod would not affect other WLED functions yet still work effectively on a large staircase
Tested On
Build Status
Summary by CodeRabbit
New Features
Documentation