Skip to content

Conversation

@skykingjwc
Copy link

@skykingjwc skykingjwc commented Jan 16, 2026

Added config options and logic to invert the output of PIR sensors for the Animated Staircase usermod. Some sensors go low when the beam is broken. This PR adds support for those modules.

Summary by CodeRabbit

  • New Features
    • Added support for inverting motion sensor logic for both top and bottom sensors in animated staircase mode, enabling flexible sensor pin configuration that persists across restarts.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 16, 2026

Walkthrough

Adds support for inverted PIR/trigger pin logic by introducing two new configuration flags (topAPinInvert, bottomAPinInvert), a helper function to read pins with optional inversion, and updates to configuration persistence and sensor reading logic.

Changes

Cohort / File(s) Summary
Animated Staircase PIR Inversion
usermods/Animated_Staircase/Animated_Staircase.cpp
Adds topAPinInvert and bottomAPinInvert member variables; introduces readPIRPin(pin, invert) helper method; defines manifest keys _topPIRorTrigger_pin_invert and _bottomPIRorTrigger_pin_invert; updates addToConfig() to serialize invert flags; updates readFromConfig()/readFromJsonState() to deserialize invert flags; modifies checkSensors() to use the new helper function for per-sensor inversion support.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main change: adding support for inverting PIR pin logic in the Animated Staircase usermod.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
usermods/Animated_Staircase/Animated_Staircase.cpp (3)

163-167: Fix inconsistent indentation.

Line 165 has extra leading whitespace that doesn't match the function body's indentation style.

🔧 Suggested fix
     bool readPIRPin(int8_t pin, bool invert) {
       if (pin < 0) return false;
-        bool v = digitalRead(pin);
+      bool v = digitalRead(pin);
       return invert ? !v : v;
     }

175-184: Redundant pin validation can be simplified.

The readPIRPin function already checks for pin < 0 and returns false, making the outer ternary check redundant.

♻️ Suggested simplification
         bottomSensorRead = bottomSensorWrite ||
           (!useUSSensorBottom ?
-            (bottomPIRorTriggerPin<0 ? false : readPIRPin(bottomPIRorTriggerPin, bottomAPinInvert)) :
+            readPIRPin(bottomPIRorTriggerPin, bottomAPinInvert) :
             ultrasoundRead(bottomPIRorTriggerPin, bottomEchoPin, bottomMaxDist*59)  // cm to us
           );
         topSensorRead = topSensorWrite ||
           (!useUSSensorTop ?
-            (topPIRorTriggerPin<0 ? false : readPIRPin(topPIRorTriggerPin, topAPinInvert)) :
+            readPIRPin(topPIRorTriggerPin, topAPinInvert) :
             ultrasoundRead(topPIRorTriggerPin, topEchoPin, topMaxDist*59)   // cm to us
           );

478-479: Unused variables can be removed.

oldTopAPinInvert and oldBottomAPinInvert are stored but never referenced in the change detection logic (lines 528-533). Since invert flag changes don't require pin reallocation or setup() reconfiguration, these variables appear unnecessary.

🧹 Suggested cleanup
       bool oldUseUSSensorTop = useUSSensorTop;
       bool oldUseUSSensorBottom = useUSSensorBottom;
-      bool oldTopAPinInvert = topAPinInvert;
-      bool oldBottomAPinInvert = bottomAPinInvert;
       int8_t oldTopAPin = topPIRorTriggerPin;

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d1d9dec and a7d4731.

📒 Files selected for processing (1)
  • usermods/Animated_Staircase/Animated_Staircase.cpp
🔇 Additional comments (4)
usermods/Animated_Staircase/Animated_Staircase.cpp (4)

28-29: LGTM!

The new configuration flags are properly initialized to false, maintaining backward compatibility with existing setups where PIR sensors use active-high logic.


96-97: LGTM!

The static const char declarations follow the existing pattern for configuration keys.


465-466: LGTM!

The new invert flags are correctly persisted to the configuration following the existing pattern.


577-581: LGTM!

The PROGMEM string definitions are correctly added and follow the existing pattern for configuration keys.

✏️ Tip: You can disable this entire section by setting review_details to false in your review 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant