Multiple PIR sensors triggering WLED presets with individual on/off delays. #5179
Conversation
Updated README to improve formatting and clarity.
WalkthroughAdds a new PIR sensor switch package: a simple PIRsensorSwitch (standby/highlight, interrupt/debounce, brightness preservation, off‑timer) and a full MotionDetectionUsermod (multi‑PIR, per‑action presets/delays, contributor linking, FIFO). Also adds a library manifest and two documentation files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (4)
usermods/PIR_sensor_switch_v2/PIR_Highlight_Standby (2)
190-199: Inconsistent brace alignment reduces readability.The opening brace on line 192 is not aligned with its closing counterpart, making the control flow harder to follow.
bool handleOffTimer() { if (m_offTimerStart > 0) { - if ((millis() - m_offTimerStart > m_switchOffDelay) || bri == 0 ) { // now also checking for manual power off during highlight mode - switchStrip(false); - m_offTimerStart = 0; - return true; + if ((millis() - m_offTimerStart > m_switchOffDelay) || bri == 0) { + switchStrip(false); + m_offTimerStart = 0; + return true; } } return false; }
60-82: Consider making the PIR pin configurable.The
PIRsensorPinis hardcoded asconst uint8_t PIRsensorPin = 13. Unlike the newerMotionDetectionUsermodwhich supports runtime pin configuration, this class requires recompilation to change the pin. Consider aligning with the approach in the main implementation if this class is intended to be used alongside it.usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp (2)
239-245: Remove redundant comments about removed code.These comments explain code that was removed, which adds noise without value.
// if action just became active, enqueue onPreset; always clear offStartMs if (actions[a].activeCount == 1) { - // OPTIMIZATION: Removed redundant offStartMs = 0 if (actions[a].onPreset) enqueuePreset(actions[a].onPreset); - } else { - // OPTIMIZATION: Removed redundant offStartMs = 0 }
515-613: Large inline JavaScript block for UI customization.The
appendConfigData()method contains substantial embedded JavaScript (~100 lines). While functional, this approach makes the code harder to maintain and debug. Consider documenting the expected DOM structure or extracting common patterns if this grows further.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
usermods/PIR_sensor_switch_v2/Usermod Info Page (3x PIRs, 3x Actions example).jpgis excluded by!**/*.jpgusermods/PIR_sensor_switch_v2/Usermod Settings Page (3x PIRs, 3x Actions example).jpgis excluded by!**/*.jpg
📒 Files selected for processing (5)
usermods/PIR_sensor_switch_v2/PIR_Highlight_Standby(1 hunks)usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp(1 hunks)usermods/PIR_sensor_switch_v2/library.json(1 hunks)usermods/PIR_sensor_switch_v2/readme.md(1 hunks)usermods/PIR_sensor_switch_v2/readme_old.md(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: willmmiles
Repo: wled/WLED PR: 4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Applied to files:
usermods/PIR_sensor_switch_v2/PIR_Highlight_Standbyusermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp
📚 Learning: 2025-11-30T15:29:00.726Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4456
File: usermods/deep_sleep/deep_sleep.cpp:224-230
Timestamp: 2025-11-30T15:29:00.726Z
Learning: In WLED, `touchThreshold` is a global variable defined in wled00/wled.h and is accessible to all usermods. It stores the touch sensitivity threshold for touch-capable GPIO pins on ESP32 variants and should not be flagged as undefined when used in usermods.
Applied to files:
usermods/PIR_sensor_switch_v2/PIR_Highlight_Standby
🪛 Cppcheck (2.18.0)
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp
[error] 42-42: There is an unknown macro here somewhere. Configuration is required. If PROGMEM is a macro then please configure it.
(unknownMacro)
🪛 LanguageTool
usermods/PIR_sensor_switch_v2/readme_old.md
[grammar] ~12-~12: Ensure spelling is correct
Context: ... timer. Usermod can also be temporarily disbled/enabled from the info page by clicking ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~24-~24: Ensure spelling is correct
Context: ... which lacks such potentiometer).
Usermod installation
NOTE: Usermod has b...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~28-~28: Ensure spelling is correct
Context: ... from outside. For example from another usermod
To query or change the PIR sensor state ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~98-~98: Use a hyphen to join words.
Context: ...fo dialog)
2022-11
- Added compile time option for off timer.
- Added Home...
(QB_NEW_EN_HYPHEN)
[grammar] ~109-~109: Use a hyphen to join words.
Context: ...oticz/in`)
2024-02
- Added compile time option to expand number of PIR sens...
(QB_NEW_EN_HYPHEN)
usermods/PIR_sensor_switch_v2/readme.md
[style] ~9-~9: Using “real” as an adverb is considered informal. Consider using “really” or “very”.
Context: ...ors and actions, macro chains get messy real quick, and moreover we still cannot eas...
(REAL_REALLY)
[grammar] ~62-~62: Ensure spelling is correct
Context: ... more than 8 PIRs or Actions.
- PIR to Acion contributor tracking ensures Actions be...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~70-~70: Ensure spelling is correct
Context: ...do so bit remember to adjust the custom webapge formatting also.
Hope its useful :)
...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~70-~70: The adverb ‘also’ is commonly used to connect clauses and isn’t usually used at the end of a phrase or before a conjunction. Consider replacing it with a more formal alternative.
Context: ...to adjust the custom webapge formatting also.
Hope its useful :)
(ALSO_AS_WELL)
🔇 Additional comments (6)
usermods/PIR_sensor_switch_v2/library.json (1)
1-6: LGTM!Standard PlatformIO library manifest for the usermod.
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp (5)
50-52: Array initialization only sets the first element explicitly.Using
= { true }or= { PIR_SENSOR_PIN }only initializes the first element; remaining elements are zero-initialized. This appears intentional (default: first PIR enabled at default pin, others disabled at pin -1/0), but worth confirming this matches the expected behavior.
87-98: LGTM: FIFO implementation is correct.The circular buffer implementation with duplicate-detection and overflow handling (drop oldest) is well-designed for preventing preset flooding.
175-207: LGTM: Setup handles pin allocation and failure gracefully.The setup correctly allocates pins via PinManager, handles platform differences (ESP8266 vs ESP32), and disables PIRs when allocation fails.
420-430: Index mapping logic accepts both 0-based and 1-based keys inconsistently.The logic attempts to handle both
action0(0-based) andaction1(1-based) key styles, but the conditions overlap. For example, ifACTION_MAX=2, thenaction1matches both the 0-based check (idx >= 0 && idx < 2) mapping to index 1, and the 1-based check (idx >= 1 && idx <= 2) mapping to index 0. The first matching condition wins, soaction1maps to index 1 (0-based interpretation), which may not match user expectations if they intended 1-based indexing.Consider documenting the expected key format or simplifying to support only one style consistently.
618-623: LGTM: Usermod registration follows WLED conventions.The
getId()returns the standardUSERMOD_ID_PIRSWITCHand usesREGISTER_USERMODmacro correctly. Based on learnings, hardcoded usermod IDs are intentional in WLED.
| if (m_offTimerStart > 0) { | ||
| uiDomString = ""; | ||
| unsigned int offSeconds = (m_switchOffDelay - (millis() - m_offTimerStart)) / 1000; | ||
| if (offSeconds >= 3600) { | ||
| uiDomString += (offSeconds / 3600); | ||
| uiDomString += " hours "; | ||
| offSeconds %= 3600; | ||
| } | ||
| if (offSeconds >= 60) { | ||
| uiDomString += (offSeconds / 60); | ||
| offSeconds %= 60; | ||
| } else if (uiDomString.length() > 0){ | ||
| uiDomString += 0; | ||
| } | ||
| if (uiDomString.length() > 0){ | ||
| uiDomString += " min "; | ||
| } | ||
| uiDomString += (offSeconds); | ||
| infoArr.add(uiDomString + " sec"); |
There was a problem hiding this comment.
Potential unsigned underflow in timer calculation.
If the timer has already elapsed (elapsed time > m_switchOffDelay), the subtraction m_switchOffDelay - (millis() - m_offTimerStart) will underflow, producing a large value instead of 0.
if (m_offTimerStart > 0) {
uiDomString = "";
- unsigned int offSeconds = (m_switchOffDelay - (millis() - m_offTimerStart)) / 1000;
+ unsigned long elapsed = millis() - m_offTimerStart;
+ unsigned int offSeconds = (elapsed >= m_switchOffDelay) ? 0 : (m_switchOffDelay - elapsed) / 1000;
if (offSeconds >= 3600) {📝 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 (m_offTimerStart > 0) { | |
| uiDomString = ""; | |
| unsigned int offSeconds = (m_switchOffDelay - (millis() - m_offTimerStart)) / 1000; | |
| if (offSeconds >= 3600) { | |
| uiDomString += (offSeconds / 3600); | |
| uiDomString += " hours "; | |
| offSeconds %= 3600; | |
| } | |
| if (offSeconds >= 60) { | |
| uiDomString += (offSeconds / 60); | |
| offSeconds %= 60; | |
| } else if (uiDomString.length() > 0){ | |
| uiDomString += 0; | |
| } | |
| if (uiDomString.length() > 0){ | |
| uiDomString += " min "; | |
| } | |
| uiDomString += (offSeconds); | |
| infoArr.add(uiDomString + " sec"); | |
| if (m_offTimerStart > 0) { | |
| uiDomString = ""; | |
| unsigned long elapsed = millis() - m_offTimerStart; | |
| unsigned int offSeconds = (elapsed >= m_switchOffDelay) ? 0 : (m_switchOffDelay - elapsed) / 1000; | |
| if (offSeconds >= 3600) { | |
| uiDomString += (offSeconds / 3600); | |
| uiDomString += " hours "; | |
| offSeconds %= 3600; | |
| } | |
| if (offSeconds >= 60) { | |
| uiDomString += (offSeconds / 60); | |
| offSeconds %= 60; | |
| } else if (uiDomString.length() > 0){ | |
| uiDomString += 0; | |
| } | |
| if (uiDomString.length() > 0){ | |
| uiDomString += " min "; | |
| } | |
| uiDomString += (offSeconds); | |
| infoArr.add(uiDomString + " sec"); |
🤖 Prompt for AI Agents
In usermods/PIR_sensor_switch_v2/PIR_Highlight_Standby around lines 273 to 291,
the subtraction m_switchOffDelay - (millis() - m_offTimerStart) can underflow if
the elapsed time exceeds the delay; compute elapsed first as an unsigned long,
check if elapsed >= m_switchOffDelay and set remainingSeconds to 0 in that case,
otherwise compute remaining = m_switchOffDelay - elapsed and convert to seconds
for the UI; replace the current direct subtraction with this safe elapsed-check
pattern to prevent underflow and ensure correct remaining time is displayed.
| if (root["PIRenabled"] != nullptr) { | ||
| if (root["PIRenabled"] && !m_PIRenabled) { | ||
| attachInterrupt(digitalPinToInterrupt(PIRsensorPin), ISR_PIRstateChange, CHANGE); | ||
| newPIRsensorState(true, true); | ||
| } | ||
| else if(m_PIRenabled) { | ||
| detachInterrupt(PIRsensorPin); | ||
| } | ||
| m_PIRenabled = root["PIRenabled"]; | ||
| } |
There was a problem hiding this comment.
Logic error: Interrupt detached incorrectly when PIR is already enabled.
The else if(m_PIRenabled) branch executes whenever m_PIRenabled is currently true, regardless of the new JSON value. This means if the user sends {"PIRenabled": true} while the sensor is already enabled, the interrupt gets incorrectly detached.
if (root["PIRenabled"] != nullptr) {
- if (root["PIRenabled"] && !m_PIRenabled) {
+ bool newState = root["PIRenabled"].as<bool>();
+ if (newState && !m_PIRenabled) {
attachInterrupt(digitalPinToInterrupt(PIRsensorPin), ISR_PIRstateChange, CHANGE);
newPIRsensorState(true, true);
}
- else if(m_PIRenabled) {
+ else if(!newState && m_PIRenabled) {
detachInterrupt(PIRsensorPin);
}
- m_PIRenabled = root["PIRenabled"];
+ m_PIRenabled = newState;
}📝 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 (root["PIRenabled"] != nullptr) { | |
| if (root["PIRenabled"] && !m_PIRenabled) { | |
| attachInterrupt(digitalPinToInterrupt(PIRsensorPin), ISR_PIRstateChange, CHANGE); | |
| newPIRsensorState(true, true); | |
| } | |
| else if(m_PIRenabled) { | |
| detachInterrupt(PIRsensorPin); | |
| } | |
| m_PIRenabled = root["PIRenabled"]; | |
| } | |
| if (root["PIRenabled"] != nullptr) { | |
| bool newState = root["PIRenabled"].as<bool>(); | |
| if (newState && !m_PIRenabled) { | |
| attachInterrupt(digitalPinToInterrupt(PIRsensorPin), ISR_PIRstateChange, CHANGE); | |
| newPIRsensorState(true, true); | |
| } | |
| else if(!newState && m_PIRenabled) { | |
| detachInterrupt(PIRsensorPin); | |
| } | |
| m_PIRenabled = newState; | |
| } |
🤖 Prompt for AI Agents
In usermods/PIR_sensor_switch_v2/PIR_Highlight_Standby around lines 323-332, the
branch detaches the interrupt whenever m_PIRenabled is true regardless of the
new JSON value; change the logic to compare the incoming root["PIRenabled"]
value to the current m_PIRenabled and only attach the ISR when new==true &&
current==false and only detach when new==false && current==true, then update
m_PIRenabled to the new value; also use digitalPinToInterrupt(PIRsensorPin)
consistently when attaching and detaching the interrupt.
|
|
||
| ## Web interface | ||
|
|
||
| The info page in the web interface shows the remaining time of the off timer. Usermod can also be temporarily disbled/enabled from the info page by clicking PIR button. |
There was a problem hiding this comment.
Fix typo: "disbled" → "disabled"
-The info page in the web interface shows the remaining time of the off timer. Usermod can also be temporarily disbled/enabled from the info page by clicking PIR button.
+The info page in the web interface shows the remaining time of the off timer. Usermod can also be temporarily disabled/enabled from the info page by clicking PIR button.📝 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.
| The info page in the web interface shows the remaining time of the off timer. Usermod can also be temporarily disbled/enabled from the info page by clicking PIR button. | |
| The info page in the web interface shows the remaining time of the off timer. Usermod can also be temporarily disabled/enabled from the info page by clicking PIR button. |
🧰 Tools
🪛 LanguageTool
[grammar] ~12-~12: Ensure spelling is correct
Context: ... timer. Usermod can also be temporarily disbled/enabled from the info page by clicking ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In usermods/PIR_sensor_switch_v2/readme_old.md around line 12, there's a typo:
"disbled" should be "disabled"; update the sentence to read "...Usermod can also
be temporarily disabled/enabled from the info page by clicking PIR button." and
save the file.
|
|
||
| void togglePIRSensor() { | ||
| #ifdef USERMOD_PIR_SENSOR_SWITCH | ||
| PIRsensorSwitch *PIRsensor = (PIRsensorSwitch::*) UsermodManager::lookup(USERMOD_ID_PIRSWITCH); |
There was a problem hiding this comment.
Fix invalid C++ cast syntax in example code.
The ::* syntax is for pointer-to-member declarations, not for casting. This should be a regular pointer cast.
- PIRsensorSwitch *PIRsensor = (PIRsensorSwitch::*) UsermodManager::lookup(USERMOD_ID_PIRSWITCH);
+ PIRsensorSwitch *PIRsensor = (PIRsensorSwitch*) UsermodManager::lookup(USERMOD_ID_PIRSWITCH);📝 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.
| PIRsensorSwitch *PIRsensor = (PIRsensorSwitch::*) UsermodManager::lookup(USERMOD_ID_PIRSWITCH); | |
| PIRsensorSwitch *PIRsensor = (PIRsensorSwitch*) UsermodManager::lookup(USERMOD_ID_PIRSWITCH); |
🤖 Prompt for AI Agents
In usermods/PIR_sensor_switch_v2/readme_old.md around line 56, the example uses
an invalid C++ cast "PIRsensorSwitch::*" (pointer-to-member syntax) when casting
the lookup result; replace it with a proper pointer cast such as using C-style
cast "(PIRsensorSwitch*) UsermodManager::lookup(USERMOD_ID_PIRSWITCH)" or,
preferably, a C++ cast
"static_cast<PIRsensorSwitch*>(UsermodManager::lookup(USERMOD_ID_PIRSWITCH))" so
the code compiles and the intent is clear.
|
thanks for contributing. the code looks unfinished, is it a draft still? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/PIR_sensor_switch_v2/readme.md (1)
66-66: Capitalize "PIRs" for consistency.Use "PIRs" instead of "pirs" to match the document's capitalization convention used elsewhere.
-- To add more pirs or actions, just change the defines in the cpp file (defaults) or the platformio.ini / override +- To add more PIRs or actions, just change the defines in the cpp file (defaults) or the platformio.ini / override
🧹 Nitpick comments (1)
usermods/PIR_sensor_switch_v2/readme.md (1)
70-70: Rephrase to remove trailing "also".The phrase ending with "also" reads awkwardly. Consider rephrasing for better flow.
-- If anyone wants to add the other options from the @gegu & @blazoncek usermod then feel free to do so but remember to adjust the custom webpage formatting also. +- If anyone wants to add the other options from the @gegu & @blazoncek usermod then feel free to do so, but remember to adjust the custom webpage formatting as well.Alternatively:
-- If anyone wants to add the other options from the @gegu & @blazoncek usermod then feel free to do so but remember to adjust the custom webpage formatting also. +- If anyone wants to add the other options from the @gegu & @blazoncek usermod then feel free to do so but remember to adjust the custom webpage formatting accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/PIR_sensor_switch_v2/readme.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
usermods/PIR_sensor_switch_v2/readme.md
[grammar] ~62-~62: Use a hyphen to join words.
Context: ...need more than 8 PIRs or Actions.
- PIR to Action contributor tracking ensures A...
(QB_NEW_EN_HYPHEN)
[grammar] ~62-~62: Use a hyphen to join words.
Context: ...d more than 8 PIRs or Actions.
- PIR to Action contributor tracking ensures Acti...
(QB_NEW_EN_HYPHEN)
[style] ~70-~70: The adverb ‘also’ is commonly used to connect clauses and isn’t usually used at the end of a phrase or before a conjunction. Consider replacing it with a more formal alternative.
Context: ...to adjust the custom webpage formatting also.
Hope it's useful :)
(ALSO_AS_WELL)
🪛 markdownlint-cli2 (0.18.1)
usermods/PIR_sensor_switch_v2/readme.md
3-3: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
Updated the readme to improve formatting and clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
usermods/PIR_sensor_switch_v2/readme.md (1)
3-3: Use heading syntax instead of emphasis.Line 3 should use a level-2 heading (##) rather than bold emphasis for the author attribution, following markdown conventions and fixing the MD036 linting violation.
-**Rawframe** +## Rawframe
🧹 Nitpick comments (1)
usermods/PIR_sensor_switch_v2/readme.md (1)
62-62: Hyphenate compound modifier for clarity."PIR to Action" should be hyphenated as "PIR-to-Action" when used as a compound modifier before "contributor tracking."
-- PIR to Action contributor tracking ensures Actions behave correctly with multiple PIRs +- PIR-to-Action contributor tracking ensures Actions behave correctly with multiple PIRs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/PIR_sensor_switch_v2/readme.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
usermods/PIR_sensor_switch_v2/readme.md
[grammar] ~62-~62: Use a hyphen to join words.
Context: ...need more than 8 PIRs or Actions.
- PIR to Action contributor tracking ensures A...
(QB_NEW_EN_HYPHEN)
[grammar] ~62-~62: Use a hyphen to join words.
Context: ...d more than 8 PIRs or Actions.
- PIR to Action contributor tracking ensures Acti...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
usermods/PIR_sensor_switch_v2/readme.md
3-3: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
usermods/PIR_sensor_switch_v2/readme.md (1)
62-62: Hyphenate compound adjective "PIR-to-Action" for consistency.The term "PIR to Action" should use hyphens as "PIR-to-Action" to align with the hyphenation pattern used for "many-to-many" on line 33 and follow standard compound modifier conventions.
-- PIR to Action contributor tracking ensures Actions behave correctly with multiple PIRs +- PIR-to-Action contributor tracking ensures Actions behave correctly with multiple PIRs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/PIR_sensor_switch_v2/readme.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
usermods/PIR_sensor_switch_v2/readme.md
[grammar] ~62-~62: Use a hyphen to join words.
Context: ...need more than 8 PIRs or Actions.
- PIR to Action contributor tracking ensures A...
(QB_NEW_EN_HYPHEN)
[grammar] ~62-~62: Use a hyphen to join words.
Context: ...d more than 8 PIRs or Actions.
- PIR to Action contributor tracking ensures Acti...
(QB_NEW_EN_HYPHEN)
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp (1)
51-58: MakepirActionsinitialization explicit for robustness
pirActionsis never explicitly initialized; it currently relies on the fact thatMotionDetectionUsermodis a static instance and thus gets zero-initialized beforereadFromConfig()runs. If this class is ever instantiated with automatic storage (e.g., tests or future refactors), the bitmasks would start with indeterminate values and the OR/AND updates inreadFromConfig()could yield garbage link state.This is not a bug today, but a brittle assumption. Consider explicitly clearing the array in the constructor or
setup():class MotionDetectionUsermod : public Usermod { private: @@ uint8_t pirActions[PIR_SENSOR_MAX]; @@ public: - MotionDetectionUsermod() {} + MotionDetectionUsermod() { + for (int i = 0; i < PIR_SENSOR_MAX; i++) { + pirActions[i] = 0; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Applied to files:
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp
🪛 Cppcheck (2.18.0)
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If PROGMEM is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (1)
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp (1)
611-615: VerifyUSERMOD_ID_PIRSWITCHuniqueness across PIR usermods
getId()returnsUSERMOD_ID_PIRSWITCH. Given that this package also introduces a legacy/simple PIR usermod in another file (per PR description/summary), there is a risk that both usermods might share the same ID if both are compiled in the same build configuration.If two different usermods return the same ID, usermod management and any ID‑based logic (including config migration or API usage keyed by ID) can become ambiguous.
Please verify in
const.hand the other PIR usermod implementation that:
- Either only one of these usermods is ever compiled at a time, or
- Each usermod has a distinct ID constant.
If they currently collide, assign a new constant in
const.hfor this MotionDetection usermod and updategetId()accordingly.
🧩 Analysis chain🏁 Script executed: Length of output: 29224 @rawframe, I'll provide a comprehensive review of the Critical Issues1. Off-Timer Not Cleared on Rising Edge
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp (3)
212-213: Consider extracting poll interval to named constant.The 200ms polling interval is hardcoded. For maintainability, consider defining it as a constant.
+#ifndef PIR_POLL_INTERVAL_MS + #define PIR_POLL_INTERVAL_MS 200 +#endif + // In loop(): - if (millis() - lastLoop < 200) return; + if (millis() - lastLoop < PIR_POLL_INTERVAL_MS) return;
373-475: LGTM: State management with proper cleanup.The JSON state parsing correctly handles enable/disable toggling for both PIRs and actions, with appropriate cleanup of contributor state and timers. The index mapping logic supports both 0-based and 1-based action keys.
Optional: Consider adding rate limiting or debouncing to prevent rapid enable/disable toggling from external API calls, which could cause timing inconsistencies.
512-610: Consider refactoring JavaScript generation for maintainability.The JavaScript injection spans 100 lines of concatenated strings within
oappend(F(...))calls. While functional, this approach makes the code difficult to read, debug, and maintain.Options:
- Split into multiple smaller
oappend(F(...))calls with logical grouping (CSS, helpers, main logic, cleanup)- Move JavaScript to a separate resource file served by the web server
- Add inline comments to mark major sections within the JavaScript string
Given the "Chill" review mode, this is flagged as optional since the code works correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-29T01:22:54.617Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4623
File: usermods/word-clock-matrix/word-clock-matrix.cpp:332-332
Timestamp: 2025-03-29T01:22:54.617Z
Learning: In the WLED project, hardcoded usermod IDs (like 500 in the WordClockMatrix's getId() method) are intentionally used as part of a strategy to avoid modifying core code. There are plans to remove numeric usermod IDs entirely in the future.
Applied to files:
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp
🪛 Cppcheck (2.18.0)
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If PROGMEM is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (5)
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp (5)
119-163: LGTM: Config parsing is robust.The configuration parsing handles buffer safety with
snprintf, uses explicit type casts for JSON boolean parsing, and includes overflow prevention for the off-delay calculation.
169-201: LGTM: Proper pin management and initialization.The setup correctly handles pin allocation with fallback on failure, uses platform-specific pinMode configuration for ESP8266 vs ESP32, and initializes runtime state without overwriting persisted configuration.
279-367: LGTM: UI generation is functional.The JSON info generation correctly builds UI elements with buffer safety (
snprintf) and includes underflow protection for timer calculations. The HTML string concatenation is verbose but clear and functional.
481-506: LGTM: Configuration persistence is correct.The configuration serialization properly saves all PIR and action settings with buffer safety. The enabled state persistence ensures configuration survives reboots.
615-620: LGTM: Standard usermod registration.The usermod ID and registration follow WLED conventions. Based on learnings, hardcoded usermod IDs are intentional and part of the project's strategy.
| #ifndef PIR_SENSOR_MAX | ||
| #define PIR_SENSOR_MAX 2 | ||
| #endif | ||
|
|
||
| #ifndef ACTION_MAX | ||
| #define ACTION_MAX 2 | ||
| #endif |
There was a problem hiding this comment.
Add compile-time validation for bitmask size constraint.
The bitmask pirActions uses uint8_t (8 bits), limiting support to 8 actions maximum. However, ACTION_MAX is configurable without validation. If ACTION_MAX > 8, bitmask operations at lines 136, 226, 245, 310, and 493 will fail silently.
Add a compile-time assertion after the definitions:
+#ifndef ACTION_MAX
+ #define ACTION_MAX 2
+#endif
+
+#if ACTION_MAX > 8
+ #error "ACTION_MAX must be <= 8 due to uint8_t bitmask size constraint in pirActions"
+#endif
+
#ifndef PRESET_FIFO_SIZE
#define PRESET_FIFO_SIZE 16
#endifAlternatively, if support for more than 8 actions is desired, change pirActions to uint16_t or uint32_t and update the constraint accordingly.
Also applies to: 57-57
🤖 Prompt for AI Agents
usermods/PIR_sensor_switch_v2/PIR_sensor_switch_v2.cpp lines 27-33 (and also at
line 57): ACTION_MAX can exceed the 8 bits of the uint8_t bitmask used for
pirActions, causing silent failures; add a compile-time validation and/or widen
the bitmask type. Either add a static_assert immediately after these #defines to
enforce ACTION_MAX <= 8, or change the pirActions declaration to
uint16_t/uint32_t and update the assertion (and any masking/shift logic) to use
the new width; ensure all bitwise operations and masks at lines 136, 226, 245,
310, and 493 are adjusted to the chosen integer size.
| if (actions[a].activeCount == 1) { | ||
| // OPTIMIZATION: Removed redundant offStartMs = 0 | ||
| if (actions[a].onPreset) enqueuePreset(actions[a].onPreset); | ||
| } else { | ||
| // OPTIMIZATION: Removed redundant offStartMs = 0 | ||
| } |
There was a problem hiding this comment.
Critical: Off-timer not cleared when action becomes active.
When a PIR triggers while an off-timer is counting (activeCount transitions from 0→1), offStartMs is not reset. This causes the off-preset to fire incorrectly despite active motion.
Scenario:
- PIR1 goes LOW →
activeCount = 0,offStartMs = millis()(timer starts at line 251) - PIR2 goes HIGH before timer expires →
activeCount = 1(line 230) - Line 234 condition is true, but
offStartMsis not cleared (line 235 comment claims "redundant") - Timer check at line 263 can still fire and run the off-preset despite
activeCount > 0
Apply this diff to clear offStartMs when the action becomes or remains active:
// if action just became active, enqueue onPreset; always clear offStartMs
if (actions[a].activeCount == 1) {
- // OPTIMIZATION: Removed redundant offStartMs = 0
+ actions[a].offStartMs = 0;
if (actions[a].onPreset) enqueuePreset(actions[a].onPreset);
} else {
- // OPTIMIZATION: Removed redundant offStartMs = 0
+ actions[a].offStartMs = 0;
}Also apply this diff to the unified timer check:
if (actions[a].offPreset) enqueuePreset(actions[a].offPreset);
}
} else {
- // OPTIMIZATION: Removed redundant offStartMs = 0
+ actions[a].offStartMs = 0;
}Also applies to: 267-269
|
| */ | ||
|
|
||
| #include "wled.h" | ||
|
|
There was a problem hiding this comment.
missing an include here, also missing the .h in you header
There was a problem hiding this comment.
it compiles ok. my coding and github useage isnt great so im not sure what else is needed here? it doesn't seem much different the existing usermod of pir_sensor_switch thats already in wled repo.
| @@ -0,0 +1,110 @@ | |||
| # PIR sensor switch | |||
There was a problem hiding this comment.
why have two readme files?
There was a problem hiding this comment.
this is the older readme from the existing usermod of pir_sensor_switch thats already in wled repo. it was kept in the same way that usermod kept my old highlight standby pir from years ago
| ## Notes | ||
|
|
||
| - To add more PIRs or Actions, just change the defines in the cpp file (defaults) or the platformio.ini / override | ||
| - PIR_SENSOR_MAX |
There was a problem hiding this comment.
it says a limit of 8 in the readme
| * | ||
| * Using a usermod: | ||
| * 1. Copy the usermod into the sketch folder (same folder as wled00.ino) | ||
| * 2. Register the usermod by adding #include "usermod_filename.h" in the top and registerUsermod(new MyUsermodClass()) in the bottom of usermods_list.cpp |
There was a problem hiding this comment.
not edited in by me but is in the existing usermod of pir_sensor_switch thats already in wled repo.
|
I see what you did now, but why mix in the old code and the old readme? if this is a new, independent usermod keeping the old stuff just makes it really convoluted and confusing. edit: |
Sure, there no need for the standbyhighlight file or the older readme. I only kept it there in the same way the other pir usermod kept my old codes readme with more added (highlightstandby) and I figured a similar trail was the convention here. |
|
without knowing this nor the other PIR usermod (so please excuse my ignorance): why do we need two PIR usermods? is it possible to unify them? |
2 participants
What It Does:
Multiple PIR sensors triggering WLED presets with individual on/off delays.
Screenshots (default config of 3x PIRs & 3x Actions) :
ugh the WLED UI
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.