Migrated AnimatedStaircase usermod to VL53L0X TOF sensors#5656
Open
Hoverman1977 wants to merge 2 commits into
Open
Migrated AnimatedStaircase usermod to VL53L0X TOF sensors#5656Hoverman1977 wants to merge 2 commits into
Hoverman1977 wants to merge 2 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a new WLED usermod, AnimatedStaircase_VL53L0X, that uses two VL53L0X time-of-flight sensors (with optional XSHUT pins) to drive segmented staircase animations via a FreeRTOS polling task; includes config/state JSON IO, MQTT state publish, README, library manifest, and PlatformIO override/example config while restricting default build envs to esp32dev. ChangesAnimatedStaircase_VL53L0X Usermod Implementation
Documentation, Manifest, and Build Overrides
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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.
Comment on lines
+178
to
+193
| if (!bottomVL53.init()) { | ||
| DEBUG_PRINTLN(F("[StaircaseVL53] Bottom VL53L0X init failed")); | ||
| resetSensorState(); | ||
| return false; | ||
| } | ||
|
|
||
| bottomVL53.setAddress(0x30); | ||
| bottomVL53.startContinuous(); | ||
|
|
||
| digitalWrite(xshutTopPin, HIGH); | ||
| delay(20); | ||
|
|
||
| if (!topVL53.init()) { | ||
| DEBUG_PRINTLN(F("[StaircaseVL53] Top VL53L0X init failed")); | ||
| resetSensorState(); | ||
| return false; |
Contributor
There was a problem hiding this comment.
Unwind the partial sensor init before returning.
Both failure paths call resetSensorState() and return, but they leave the XSHUT pins allocated and, on the top-sensor failure path, can leave the bottom sensor powered and running at 0x30. The worker keeps retrying initialiseSensors(), so recovery now depends on allocateMultiplePins() tolerating already-owned pins. Please explicitly power down and deallocate the partial setup here.
Suggested unwind pattern
if (!bottomVL53.init()) {
DEBUG_PRINTLN(F("[StaircaseVL53] Bottom VL53L0X init failed"));
+ digitalWrite(xshutTopPin, LOW);
+ digitalWrite(xshutBottomPin, LOW);
+ PinManager::deallocatePin(xshutTopPin, PinOwner::UM_AnimatedStaircase);
+ PinManager::deallocatePin(xshutBottomPin, PinOwner::UM_AnimatedStaircase);
resetSensorState();
return false;
}
@@
if (!topVL53.init()) {
DEBUG_PRINTLN(F("[StaircaseVL53] Top VL53L0X init failed"));
+ digitalWrite(xshutTopPin, LOW);
+ digitalWrite(xshutBottomPin, LOW);
+ PinManager::deallocatePin(xshutTopPin, PinOwner::UM_AnimatedStaircase);
+ PinManager::deallocatePin(xshutBottomPin, PinOwner::UM_AnimatedStaircase);
resetSensorState();
return false;
}🤖 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 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.
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.
Comment on lines
+474
to
+475
| topThresholdMM = top[FPSTR(_topThreshold)] | topThresholdMM; | ||
| bottomThresholdMM = top[FPSTR(_bottomThreshold)] | bottomThresholdMM; |
Contributor
There was a problem hiding this comment.
Clamp the config thresholds before storing them.
These values come from client-supplied JSON and are consumed unguarded in checkSensors(). A negative or oversized value coerced into uint16_t can leave a sensor effectively stuck on or off. Please range-clamp them here instead of trusting the incoming config.
As per coding guidelines, "Values retrieved at these ingress points are considered trusted only after the usermod itself has validated and range-clamped them."
🤖 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 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()).
| This mod replaces the original PIR or ultrasonic sensors with two **VL53L0X** I²C time-of-flight sensors. These are more compact, and immune to acoustic interference. Changes include: | ||
|
|
||
| - VL53L0X support for both top and bottom sensors | ||
| - Optional `xshut` pins for I²C address assignment |
Contributor
There was a problem hiding this comment.
XSHUT pins are documented as optional, but code requires them.
The README says XSHUT is optional, but initialiseSensors() fails when either XSHUT pin is unset and disables the usermod (usermods/AnimatedStaircase_VL53L0X/AnimatedStaircase_VL53L0X.cpp, Lines 127-209). Please update wording to make both pins mandatory and explicitly state they must be different.
Also applies to: 40-40, 55-56
🤖 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` at 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().
Comment on lines
+33
to
+36
| Before compiling, you have to make the following modifications: | ||
|
|
||
| Copy platformio_override.ini to the same folder as platformio.ini | ||
|
|
Contributor
There was a problem hiding this comment.
Setup steps miss custom_usermods activation, so install instructions are incomplete.
The install section should explicitly include adding AnimatedStaircase_VL53L0X to custom_usermods in the build config; otherwise users can compile without actually enabling the usermod.
As per coding guidelines: "platformio*.ini: To activate a usermod, add the usermod name to custom_usermods in the build configuration".
🤖 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 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.
coderabbitai
Bot
added
the
documentation
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
1 participant
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.
Summary by CodeRabbit
New Features
Documentation