added support for VL53L0X TOF sensors #4931
Conversation
WalkthroughAdds optional dual VL53L0X distance-sensor support to Animated_Staircase with new pins, configuration fields, initialization (addressing via XSHUT), continuous ranging, error handling, and debug outputs. Sensor reading flow is updated to prefer VL53L0X when enabled and healthy, falling back to existing ultrasound/PIR logic otherwise. 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
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 5
🧹 Nitpick comments (5)
usermods/Animated_Staircase/Animated_Staircase.cpp (5)
455-457: Mark XSHUT pins as outputs in PinManager allocation.They’re driven as outputs later; reflect that here for consistency with PinManager’s ownership semantics.
Apply this diff:
- { topXSHUTPin, false}, - { botXSHUTPin, false}, + { topXSHUTPin, true }, + { botXSHUTPin, true },
188-215: Tighten range checks; simplify boolean logic.Use a named min-range and a single boolean expression; keep 999 sentinel if desired.
Apply this diff:
- bool vl53l0xRead(bool top, unsigned int maxDist) { - int16_t mm = 0; + bool vl53l0xRead(bool top, unsigned int maxDist) { + static const int16_t kMinRangeMm = 20; + int16_t mm = 0; @@ - if (mm>20) { - return (mm < maxDist ? true : false); - } else { - return false; - } + return (mm > kMinRangeMm) && (mm < (int16_t)maxDist); }
358-375: Avoid pinMode on -1 for PIR/US pins.Pre-existing, but now gated under
!useV53l0x. Guard with>=0checks before pinMode.Apply this diff:
- if (!useUSSensorBottom) - pinMode(bottomPIRorTriggerPin, INPUT_PULLUP); + if (!useUSSensorBottom && bottomPIRorTriggerPin >= 0) + pinMode(bottomPIRorTriggerPin, INPUT_PULLUP); else { - pinMode(bottomPIRorTriggerPin, OUTPUT); - pinMode(bottomEchoPin, INPUT); + if (bottomPIRorTriggerPin >= 0) pinMode(bottomPIRorTriggerPin, OUTPUT); + if (bottomEchoPin >= 0) pinMode(bottomEchoPin, INPUT); } @@ - if (!useUSSensorTop) - pinMode(topPIRorTriggerPin, INPUT_PULLUP); + if (!useUSSensorTop && topPIRorTriggerPin >= 0) + pinMode(topPIRorTriggerPin, INPUT_PULLUP); else { - pinMode(topPIRorTriggerPin, OUTPUT); - pinMode(topEchoPin, INPUT); + if (topPIRorTriggerPin >= 0) pinMode(topPIRorTriggerPin, OUTPUT); + if (topEchoPin >= 0) pinMode(topEchoPin, INPUT); }
707-721: Fix UI labels and messages (“VL53L0X”, consistent failure text).Minor copy polish.
Apply this diff:
- if (useV53l0x) { - JsonArray topRange = user.createNestedArray(F("V53L0X Top Range")); - JsonArray botRange = user.createNestedArray(F("V53L0X Bottom Range")); + if (useV53l0x) { + JsonArray topRange = user.createNestedArray(F("VL53L0X Top Range")); + JsonArray botRange = user.createNestedArray(F("VL53L0X Bottom Range")); @@ - if (botSensorFailed) { - botRange.add(F("Bottom Sensor Failed!")); + if (botSensorFailed) { + botRange.add(F("Sensor Failed!"));
431-435: Call startContinuous() only for sensors that initialized successfully.Currently called unconditionally; skip for failed sensors.
If you adopt the setID() refactor above, this is already addressed. Otherwise, add guards around the start calls.
📜 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
🧬 Code graph analysis (1)
usermods/Animated_Staircase/Animated_Staircase.cpp (2)
usermods/pixels_dice_tray/pixels_dice_tray.cpp (1)
DEBUG_PRINTLN(107-149)usermods/VL53L0X_gestures/VL53L0X_gestures.cpp (1)
enabled(63-104)
| #define LOXTOP_ADDRESS 0x41 // Default address should be 0x41 | ||
| #define LOXBOT_ADDRESS 0x42 | ||
|
|
There was a problem hiding this comment.
Fix VL53L0X address constants (default is 0x29) and pick unique addresses for dual sensors.
The comment “Default address should be 0x41” is incorrect; VL53L0X default is 0x29. Also, define two non-conflicting addresses explicitly for dual setup.
Apply this diff:
- #define LOXTOP_ADDRESS 0x41 // Default address should be 0x41
- #define LOXBOT_ADDRESS 0x42
+ // Default VL53L0X I2C address is 0x29; reassign to unique addresses when using two sensors
+ #define LOXTOP_ADDRESS 0x30
+ #define LOXBOT_ADDRESS 0x31📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define LOXTOP_ADDRESS 0x41 // Default address should be 0x41 | |
| #define LOXBOT_ADDRESS 0x42 | |
| // Default VL53L0X I2C address is 0x29; reassign to unique addresses when using two sensors | |
| #define LOXTOP_ADDRESS 0x30 | |
| #define LOXBOT_ADDRESS 0x31 |
🤖 Prompt for AI Agents
In usermods/Animated_Staircase/Animated_Staircase.cpp around lines 22 to 24, the
VL53L0X address comment and defines are wrong — the sensor's default 7-bit I2C
address is 0x29 and you must pick two unique addresses for a dual-sensor setup;
update the comment to state the correct default (0x29) and change the defines to
two distinct addresses (for example LOX_TOP_ADDRESS 0x29 and LOX_BOTTOM_ADDRESS
0x2A) so the sensors do not conflict on the bus.
There was a problem hiding this comment.
It looks like @coderabbitai is correct. Examples I have use the same I2C addresses as suggested.
There was a problem hiding this comment.
@blazoncek Thank you for confirming! It's great to have validation from your examples that the VL53L0X default address is indeed 0x29, and that using distinct addresses like 0x30 and 0x31 (or similar non-conflicting pairs) is the correct approach for dual-sensor setups. This will ensure the sensors don't interfere with each other on the I2C bus.
| if (useV53l0x && (!(botSensorFailed)) ) { | ||
| bottomSensorRead = bottomSensorWrite || vl53l0xRead(false, (bottomMaxDist * 10)); | ||
| } else { | ||
| bottomSensorRead = bottomSensorWrite || | ||
| (!useUSSensorBottom ? | ||
| (bottomPIRorTriggerPin<0 ? false : digitalRead(bottomPIRorTriggerPin)) : | ||
| ultrasoundRead(bottomPIRorTriggerPin, bottomEchoPin, bottomMaxDist*59) // cm to us | ||
| ); | ||
| topSensorRead = topSensorWrite || | ||
|
|
||
| } | ||
|
|
||
| if (useV53l0x && (!(topSensorFailed))) { | ||
| topSensorRead = topSensorWrite || vl53l0xRead(true, (topMaxDist * 10)); | ||
| } else { | ||
| topSensorRead = topSensorWrite || | ||
| (!useUSSensorTop ? | ||
| (topPIRorTriggerPin<0 ? false : digitalRead(topPIRorTriggerPin)) : | ||
| ultrasoundRead(topPIRorTriggerPin, topEchoPin, topMaxDist*59) // cm to us | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider degrading to fallback on repeated timeouts.
If timeoutOccurred() happens repeatedly, mark the sensor failed to avoid continual I2C churn and rely on PIR/US fallback.
If agreeable, I can draft a small rolling timeout counter and trip *_SensorFailed after N consecutive timeouts.
🤖 Prompt for AI Agents
In usermods/Animated_Staircase/Animated_Staircase.cpp around lines 231 to 250,
the VL53L0X reads can repeatedly time out causing continual I2C churn; add a
small rolling timeout counter per sensor (e.g., topTimeoutCount and
bottomTimeoutCount) that increments when timeoutOccurred() or vl53l0xRead
indicates a timeout, resets to zero on a successful read, and when the counter
reaches a configurable threshold N set the corresponding
topSensorFailed/bottomSensorFailed to true so code falls back to PIR/US; ensure
counters are checked/updated immediately after each vl53l0xRead call and persist
across loop iterations (initialize near sensor state), and consider resetting
the failed flag and the counter if a future manual reinitialization or
successful probe is performed.
| void setID() { | ||
| // all reset | ||
| digitalWrite(topXSHUTPin, LOW); | ||
| digitalWrite(botXSHUTPin, LOW); | ||
| delay(10); | ||
| // all unreset | ||
| digitalWrite(topXSHUTPin, HIGH); | ||
| digitalWrite(botXSHUTPin, HIGH); | ||
| delay(10); | ||
|
|
||
| digitalWrite(botXSHUTPin, LOW); | ||
| delay(10); | ||
|
|
||
| //Only one address needs to be set, other sensor will use default I2C address | ||
| loxBot.setAddress(LOXBOT_ADDRESS); | ||
| delay(10); | ||
| digitalWrite(botXSHUTPin, HIGH); | ||
| // initing LOX1 | ||
| if(!loxBot.init(false)) { | ||
| DEBUG_PRINTLN(F("VL053L0X: Failed to boot Bottom VL53L0X")); | ||
| botSensorFailed = true; | ||
| } | ||
| delay(10); | ||
|
|
||
| // activating LOX2 | ||
|
|
||
| //initing LOX2 | ||
| if(!loxTop.init(false)) { | ||
| DEBUG_PRINTLN(F("VL053L0X: Failed to boot Top VL53L0X")); | ||
| topSensorFailed = true; | ||
| } | ||
|
|
||
| loxTop.setTimeout(500); | ||
| loxBot.setTimeout(500); | ||
| loxTop.startContinuous(); | ||
| loxBot.startContinuous(); | ||
| } |
There was a problem hiding this comment.
Address-assignment sequence is incorrect (setAddress before init; device held in reset; bus collision risk).
- Both sensors are released to default 0x29 simultaneously, then one is put back to reset. This can cause brief address collision.
- setAddress() is called before init(), and while the target sensor is in reset — likely readdressing the wrong device.
Reorder: hold both in reset; bring up one sensor, init it, readdress it; then bring up the other, init and readdress; start continuous only on successful init.
Apply this diff:
void setID() {
- // all reset
- digitalWrite(topXSHUTPin, LOW);
- digitalWrite(botXSHUTPin, LOW);
- delay(10);
- // all unreset
- digitalWrite(topXSHUTPin, HIGH);
- digitalWrite(botXSHUTPin, HIGH);
- delay(10);
-
- digitalWrite(botXSHUTPin, LOW);
- delay(10);
-
- //Only one address needs to be set, other sensor will use default I2C address
- loxBot.setAddress(LOXBOT_ADDRESS);
- delay(10);
- digitalWrite(botXSHUTPin, HIGH);
- // initing LOX1
- if(!loxBot.init(false)) {
- DEBUG_PRINTLN(F("VL053L0X: Failed to boot Bottom VL53L0X"));
- botSensorFailed = true;
- }
- delay(10);
-
- // activating LOX2
-
- //initing LOX2
- if(!loxTop.init(false)) {
- DEBUG_PRINTLN(F("VL053L0X: Failed to boot Top VL53L0X"));
- topSensorFailed = true;
- }
-
- loxTop.setTimeout(500);
- loxBot.setTimeout(500);
- loxTop.startContinuous();
- loxBot.startContinuous();
+ // Guard pins
+ if (topXSHUTPin < 0 || botXSHUTPin < 0) {
+ DEBUG_PRINTLN(F("VL53L0X: XSHUT pins not configured."));
+ topSensorFailed = botSensorFailed = true;
+ return;
+ }
+ // Reset both
+ digitalWrite(topXSHUTPin, LOW);
+ digitalWrite(botXSHUTPin, LOW);
+ delay(10);
+
+ // Bring up TOP first, init, then readdress
+ digitalWrite(topXSHUTPin, HIGH);
+ delay(10);
+ if (!loxTop.init(false)) {
+ DEBUG_PRINTLN(F("VL53L0X: Failed to boot Top VL53L0X"));
+ topSensorFailed = true;
+ } else {
+ loxTop.setAddress(LOXTOP_ADDRESS); // move away from default 0x29
+ loxTop.setTimeout(500);
+ loxTop.startContinuous();
+ }
+
+ // Bring up BOTTOM, init, then readdress
+ digitalWrite(botXSHUTPin, HIGH);
+ delay(10);
+ if (!loxBot.init(false)) {
+ DEBUG_PRINTLN(F("VL53L0X: Failed to boot Bottom VL53L0X"));
+ botSensorFailed = true;
+ } else {
+ loxBot.setAddress(LOXBOT_ADDRESS);
+ loxBot.setTimeout(500);
+ loxBot.startContinuous();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void setID() { | |
| // all reset | |
| digitalWrite(topXSHUTPin, LOW); | |
| digitalWrite(botXSHUTPin, LOW); | |
| delay(10); | |
| // all unreset | |
| digitalWrite(topXSHUTPin, HIGH); | |
| digitalWrite(botXSHUTPin, HIGH); | |
| delay(10); | |
| digitalWrite(botXSHUTPin, LOW); | |
| delay(10); | |
| //Only one address needs to be set, other sensor will use default I2C address | |
| loxBot.setAddress(LOXBOT_ADDRESS); | |
| delay(10); | |
| digitalWrite(botXSHUTPin, HIGH); | |
| // initing LOX1 | |
| if(!loxBot.init(false)) { | |
| DEBUG_PRINTLN(F("VL053L0X: Failed to boot Bottom VL53L0X")); | |
| botSensorFailed = true; | |
| } | |
| delay(10); | |
| // activating LOX2 | |
| //initing LOX2 | |
| if(!loxTop.init(false)) { | |
| DEBUG_PRINTLN(F("VL053L0X: Failed to boot Top VL53L0X")); | |
| topSensorFailed = true; | |
| } | |
| loxTop.setTimeout(500); | |
| loxBot.setTimeout(500); | |
| loxTop.startContinuous(); | |
| loxBot.startContinuous(); | |
| } | |
| void setID() { | |
| // Guard pins | |
| if (topXSHUTPin < 0 || botXSHUTPin < 0) { | |
| DEBUG_PRINTLN(F("VL53L0X: XSHUT pins not configured.")); | |
| topSensorFailed = botSensorFailed = true; | |
| return; | |
| } | |
| // Reset both | |
| digitalWrite(topXSHUTPin, LOW); | |
| digitalWrite(botXSHUTPin, LOW); | |
| delay(10); | |
| // Bring up TOP first, init, then readdress | |
| digitalWrite(topXSHUTPin, HIGH); | |
| delay(10); | |
| if (!loxTop.init(false)) { | |
| DEBUG_PRINTLN(F("VL53L0X: Failed to boot Top VL53L0X")); | |
| topSensorFailed = true; | |
| } else { | |
| loxTop.setAddress(LOXTOP_ADDRESS); // move away from default 0x29 | |
| loxTop.setTimeout(500); | |
| loxTop.startContinuous(); | |
| } | |
| // Bring up BOTTOM, init, then readdress | |
| digitalWrite(botXSHUTPin, HIGH); | |
| delay(10); | |
| if (!loxBot.init(false)) { | |
| DEBUG_PRINTLN(F("VL53L0X: Failed to boot Bottom VL53L0X")); | |
| botSensorFailed = true; | |
| } else { | |
| loxBot.setAddress(LOXBOT_ADDRESS); | |
| loxBot.setTimeout(500); | |
| loxBot.startContinuous(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In usermods/Animated_Staircase/Animated_Staircase.cpp around lines 399-435, the
I2C address sequence is wrong: both sensors are briefly released together,
setAddress is called before init while the device may still be in reset, risking
address collision and readdressing the wrong device. Fix by holding both XSHUT
LOW, then release only the first sensor (e.g., bot) HIGH, init it and only after
successful init call setAddress(LOXBOT_ADDRESS); then release the second sensor
(top) HIGH, init it and set its address if needed; set timeouts and call
startContinuous() only for sensors that successfully initialized, and mark
failures accordingly.
| if (!PinManager::allocateMultiplePins(pins, 6, PinOwner::UM_AnimatedStaircase)) { | ||
| topPIRorTriggerPin = -1; | ||
| topEchoPin = -1; | ||
| bottomPIRorTriggerPin = -1; | ||
| bottomEchoPin = -1; | ||
| enabled = false; | ||
| //V53L0X | ||
| topXSHUTPin = -1; | ||
| botXSHUTPin = -1; | ||
| } |
There was a problem hiding this comment.
Return early on pin allocation failure to avoid using invalid pins.
After failure you proceed to configure pins (potentially -1). Bail out from setup.
Apply this diff:
if (!PinManager::allocateMultiplePins(pins, 6, PinOwner::UM_AnimatedStaircase)) {
topPIRorTriggerPin = -1;
topEchoPin = -1;
bottomPIRorTriggerPin = -1;
bottomEchoPin = -1;
enabled = false;
//V53L0X
topXSHUTPin = -1;
botXSHUTPin = -1;
+ DEBUG_PRINTLN(F("Animated Staircase: pin allocation failed, disabling."));
+ return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!PinManager::allocateMultiplePins(pins, 6, PinOwner::UM_AnimatedStaircase)) { | |
| topPIRorTriggerPin = -1; | |
| topEchoPin = -1; | |
| bottomPIRorTriggerPin = -1; | |
| bottomEchoPin = -1; | |
| enabled = false; | |
| //V53L0X | |
| topXSHUTPin = -1; | |
| botXSHUTPin = -1; | |
| } | |
| if (!PinManager::allocateMultiplePins(pins, 6, PinOwner::UM_AnimatedStaircase)) { | |
| topPIRorTriggerPin = -1; | |
| topEchoPin = -1; | |
| bottomPIRorTriggerPin = -1; | |
| bottomEchoPin = -1; | |
| enabled = false; | |
| //V53L0X | |
| topXSHUTPin = -1; | |
| botXSHUTPin = -1; | |
| DEBUG_PRINTLN(F("Animated Staircase: pin allocation failed, disabling.")); | |
| return; | |
| } |
| if (useV53l0x) { | ||
| pinMode(topXSHUTPin, OUTPUT); | ||
| pinMode(botXSHUTPin, OUTPUT); | ||
| DEBUG_PRINTLN(F("VL053L0X: Shutdown pins inited...")); | ||
| digitalWrite(topXSHUTPin, LOW); | ||
| digitalWrite(botXSHUTPin, LOW); | ||
| DEBUG_PRINTLN(F("VL053L0X: Both in reset mode...(pins are low)")); | ||
| DEBUG_PRINTLN(F("VL053L0X: Starting...")); | ||
| setID(); | ||
| showDebugData = millis(); | ||
| } |
There was a problem hiding this comment.
Guard pinMode/digitalWrite against invalid XSHUT pins when VL53L0X is enabled.
Calling pinMode/digitalWrite with -1 is undefined on some MCUs.
Apply this diff:
- if (useV53l0x) {
- pinMode(topXSHUTPin, OUTPUT);
- pinMode(botXSHUTPin, OUTPUT);
+ if (useV53l0x) {
+ if (topXSHUTPin < 0 || botXSHUTPin < 0) {
+ DEBUG_PRINTLN(F("VL53L0X: Missing XSHUT pins, disabling VL53L0X path."));
+ topSensorFailed = botSensorFailed = true;
+ } else {
+ pinMode(topXSHUTPin, OUTPUT);
+ pinMode(botXSHUTPin, OUTPUT);
DEBUG_PRINTLN(F("VL053L0X: Shutdown pins inited..."));
digitalWrite(topXSHUTPin, LOW);
digitalWrite(botXSHUTPin, LOW);
DEBUG_PRINTLN(F("VL053L0X: Both in reset mode...(pins are low)"));
DEBUG_PRINTLN(F("VL053L0X: Starting..."));
setID();
showDebugData = millis();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (useV53l0x) { | |
| pinMode(topXSHUTPin, OUTPUT); | |
| pinMode(botXSHUTPin, OUTPUT); | |
| DEBUG_PRINTLN(F("VL053L0X: Shutdown pins inited...")); | |
| digitalWrite(topXSHUTPin, LOW); | |
| digitalWrite(botXSHUTPin, LOW); | |
| DEBUG_PRINTLN(F("VL053L0X: Both in reset mode...(pins are low)")); | |
| DEBUG_PRINTLN(F("VL053L0X: Starting...")); | |
| setID(); | |
| showDebugData = millis(); | |
| } | |
| if (useV53l0x) { | |
| if (topXSHUTPin < 0 || botXSHUTPin < 0) { | |
| DEBUG_PRINTLN(F("VL53L0X: Missing XSHUT pins, disabling VL53L0X path.")); | |
| topSensorFailed = botSensorFailed = true; | |
| } else { | |
| pinMode(topXSHUTPin, OUTPUT); | |
| pinMode(botXSHUTPin, OUTPUT); | |
| DEBUG_PRINTLN(F("VL053L0X: Shutdown pins inited...")); | |
| digitalWrite(topXSHUTPin, LOW); | |
| digitalWrite(botXSHUTPin, LOW); | |
| DEBUG_PRINTLN(F("VL053L0X: Both in reset mode...(pins are low)")); | |
| DEBUG_PRINTLN(F("VL053L0X: Starting...")); | |
| setID(); | |
| showDebugData = millis(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In usermods/Animated_Staircase/Animated_Staircase.cpp around lines 471 to 481,
the code calls pinMode() and digitalWrite() for topXSHUTPin and botXSHUTPin
unconditionally which is undefined if a pin is -1; guard each pin operation with
a check (e.g., pin >= 0) before calling pinMode/digitalWrite, perform the LOW
writes only for pins that pass the check, and keep or adjust the DEBUG_PRINTLN
messages to reflect whether each pin was initialized or skipped.
There was a problem hiding this comment.
Thank you!
This was on my desk now for over 2 years (shame on me!) and I never got to implement it as I don't use this usermod.
However, I would suggest to follow @coderabbitai's recommendations. It has improved considerably recently.
Just for the reference, I was looking at this.
| #define LOXTOP_ADDRESS 0x41 // Default address should be 0x41 | ||
| #define LOXBOT_ADDRESS 0x42 | ||
|
|
There was a problem hiding this comment.
It looks like @coderabbitai is correct. Examples I have use the same I2C addresses as suggested.
added support for VL53L0X TOF sensors via I2C
Summary by CodeRabbit