Usermod: LDR_PIR #4823
Usermod: LDR_PIR #4823
Conversation
WalkthroughA new usermod named Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (7)📓 Common learnings📚 Learning: 2025-04-18T22:27:58.634ZApplied to files:
📚 Learning: 2025-03-29T01:22:54.617ZApplied to files:
📚 Learning: 2025-02-19T12:43:34.199ZApplied to files:
📚 Learning: 2025-04-28T20:51:29.773ZApplied to files:
📚 Learning: 2025-07-31T18:21:49.868ZApplied to files:
📚 Learning: 2025-04-26T12:51:04.993ZApplied to files:
🔇 Additional comments (6)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
usermods/LDR_PIR/LDR_PIR.cpp (3)
166-166: Remove duplicate assignment
lastLightis already assigned toldrValon line 139. This duplicate assignment is redundant.} lastPir=pirState; - lastLight=ldrVal; } else if (pirActivated) {
138-138: Consider making light change threshold configurableThe hardcoded threshold of 32 for detecting significant light changes could be made configurable to allow users to adjust sensitivity based on their environment.
210-211: Consider showing motion state as text for better UI readabilityInstead of showing raw 0/1 values, display "ON"/"OFF" for clearer user understanding.
JsonArray motionArr = user.createNestedArray("Motion"); //name - motionArr.add(pirState); + motionArr.add(pirState ? "ON" : "OFF");usermods/LDR_PIR/README.md (1)
10-14: Add language specification to code blockSpecify the language for syntax highlighting.
Example: -``` +```ini [env:usermod_LDR_PIR_esp32dev]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
usermods/LDR_PIR/LDR_PIR.cpp(1 hunks)usermods/LDR_PIR/README.md(1 hunks)usermods/LDR_PIR/library.json(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: willmmiles
PR: wled/WLED#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.
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
📚 Learning: when implementing mqtt message handling in wled usermods, use `strstr(topic, "/specific/path")` inst...
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
Applied to files:
usermods/LDR_PIR/README.mdusermods/LDR_PIR/LDR_PIR.cpp
📚 Learning: in the wled project, hardcoded usermod ids (like 500 in the wordclockmatrix's getid() method) are in...
Learnt from: willmmiles
PR: wled/WLED#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/LDR_PIR/LDR_PIR.cpp
📚 Learning: in the wled codebase, syslog configuration variables (sysloghost, syslogport, syslogprotocol, syslog...
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
Applied to files:
usermods/LDR_PIR/LDR_PIR.cpp
🪛 markdownlint-cli2 (0.17.2)
usermods/LDR_PIR/README.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
21-21: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
21-21: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
22-22: Table pipe style
Expected: no_leading_or_trailing; Actual: trailing_only; Unexpected trailing pipe
(MD055, table-pipe-style)
22-22: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🔇 Additional comments (10)
usermods/LDR_PIR/library.json (1)
1-4: Configuration looks goodThe library.json follows the standard format for WLED usermods.
usermods/LDR_PIR/LDR_PIR.cpp (8)
1-50: Class structure and member variables look goodThe member variables are well-organized with sensible defaults, and using PROGMEM for string literals is a good practice for memory optimization on ESP devices.
51-64: MQTT initialization implemented correctlyThe topic construction follows WLED conventions and buffer sizes are appropriate.
67-96: Home Assistant discovery implementation is well-structuredThe discovery payload follows Home Assistant conventions correctly with appropriate device metadata and retained messages.
98-105: MQTT publishing method is properly protectedGood practice checking MQTT connection status before publishing to prevent crashes.
113-119: Pin configuration is appropriateCorrect pin modes for LDR (analog input) and PIR (digital input with pullup).
192-197: MQTT connection handler implemented correctlyGood use of flag to prevent duplicate discovery message publishing.
214-250: Configuration serialization and deserialization properly implementedGood error handling and use of default values in configuration loading.
256-268: Static definitions and registration follow WLED conventionsProper use of PROGMEM for string constants and standard usermod registration.
usermods/LDR_PIR/README.md (1)
3-3: Documentation clearly explains the usermod functionalityGood explanation of the combined LDR and PIR sensor logic for conditional preset activation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
usermods/LDR_PIR/LDR_PIR.cpp (1)
140-144: Good fix for ADC resolution platform differences.The conditional compilation to handle different ADC resolutions between ESP32 (12-bit) and ESP8266 (10-bit) correctly addresses the previous review concern.
🧹 Nitpick comments (4)
usermods/LDR_PIR/LDR_PIR.cpp (4)
15-21: Consider making default pin assignments platform-specific.The default pin assignments (photoPin = 1, pirPin = 0) may conflict with critical pins on some platforms. Pin 0 is the boot button on many ESP32 boards and pin 1 is typically TX on ESP8266.
Consider using platform-specific defaults:
-int8_t photoPin = 1; -int8_t pirPin = 0; +#ifdef ESP32 +int8_t photoPin = 36; // ADC1_CH0 +int8_t pirPin = 4; // GPIO4 +#else +int8_t photoPin = A0; // ESP8266 analog pin +int8_t pirPin = 5; // GPIO5 (D1) +#endif
190-193: Redundant motion OFF publication.The code publishes "OFF" for motion every 10 minutes (line 191), but motion is already published as "OFF" when PIR state changes to LOW (line 149). This creates redundant MQTT messages.
Consider removing the redundant publication or adding a condition:
-if (curTime - lastMotion > 600000) { - publishMqtt("motion","OFF"); - lastMotion = curTime; -} +// Remove this block as motion OFF is already published on state change
244-245: Pin validation should reject invalid values.The code allows -1 as default for pins but doesn't validate that positive pin numbers are within valid GPIO ranges for the platform.
Add pin validation in
readFromConfig:configComplete &= getJsonValue(top[FPSTR(_photoPin)], photoPin, -1); configComplete &= getJsonValue(top[FPSTR(_pirPin)], pirPin, -1); + +// Validate pin assignments +#ifdef ESP32 +if (photoPin >= 0 && (photoPin > 39 || photoPin == 6 || photoPin == 7 || photoPin == 8 || photoPin == 9 || photoPin == 10 || photoPin == 11)) { + photoPin = -1; // Invalid pin + configComplete = false; +} +if (pirPin >= 0 && (pirPin > 39 || pirPin == 6 || pirPin == 7 || pirPin == 8 || pirPin == 9 || pirPin == 10 || pirPin == 11)) { + pirPin = -1; // Invalid pin + configComplete = false; +} +#endif
271-272: Missing usermod ID method.Based on WLED patterns, usermods typically implement a
getId()method returning a unique numeric ID, though the learning indicates these may be removed in future versions.Consider adding a
getId()method for consistency:+uint16_t getId() { + return USERMOD_ID_LDR_PIR; // Define this constant appropriately +}Note: Based on retrieved learnings, hardcoded usermod IDs are intentional and may be removed in future versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/LDR_PIR/LDR_PIR.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: willmmiles
PR: wled/WLED#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.
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
📚 Learning: when implementing mqtt message handling in wled usermods, use `strstr(topic, "/specific/path")` inst...
Learnt from: KrX3D
PR: wled/WLED#4237
File: usermods/INA219_v2/INA219_v2.cpp:265-276
Timestamp: 2025-04-18T22:27:58.634Z
Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
Applied to files:
usermods/LDR_PIR/LDR_PIR.cpp
📚 Learning: in the wled project, hardcoded usermod ids (like 500 in the wordclockmatrix's getid() method) are in...
Learnt from: willmmiles
PR: wled/WLED#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/LDR_PIR/LDR_PIR.cpp
📚 Learning: esp8266 and esp32 platforms have different maximum segment name lengths in wled, which can cause tru...
Learnt from: blazoncek
PR: wled/WLED#4482
File: wled00/udp.cpp:147-149
Timestamp: 2025-02-19T12:43:34.199Z
Learning: ESP8266 and ESP32 platforms have different maximum segment name lengths in WLED, which can cause truncation when syncing segment names between devices. This platform difference affects the user experience when using the segment name sync feature.
Applied to files:
usermods/LDR_PIR/LDR_PIR.cpp
📚 Learning: in wled, `bri` is a global variable used for brightness control....
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:430-435
Timestamp: 2025-04-28T20:51:29.773Z
Learning: In WLED, `bri` is a global variable used for brightness control.
Applied to files:
usermods/LDR_PIR/LDR_PIR.cpp
📚 Learning: in the wled codebase, syslog configuration variables (sysloghost, syslogport, syslogprotocol, syslog...
Learnt from: KrX3D
PR: wled/WLED#4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
Applied to files:
usermods/LDR_PIR/LDR_PIR.cpp
🔇 Additional comments (1)
usermods/LDR_PIR/LDR_PIR.cpp (1)
101-104: MQTT topic buffer may overflow here too.Similar buffer overflow risk exists in the
publishMqttmethod.Apply this diff to use safer string handling:
-char subuf[128]; -snprintf_P(subuf, 127, PSTR("%s/%s"), mqttDeviceTopic, topic); -mqtt->publish(subuf, 0, false, state); +String fullTopic = String(mqttDeviceTopic) + "/" + topic; +mqtt->publish(fullTopic.c_str(), 0, false, state);⛔ Skipped due to learnings
Learnt from: KrX3D PR: wled/WLED#4237 File: usermods/INA219_v2/INA219_v2.cpp:265-276 Timestamp: 2025-04-18T22:27:58.634Z Learning: When implementing MQTT message handling in WLED usermods, use `strstr(topic, "/specific/path")` instead of `strcmp_P(topic, PSTR("/specific/path"))` to properly match topics that include the device prefix. The full MQTT topic typically follows the pattern `<mqttDeviceTopic>/specific/path`.
| } | ||
| } | ||
| if (pirState == HIGH) { | ||
| if (offMode && (brightness < thresholdOn) ) { |
There was a problem hiding this comment.
Undefined variable offMode will cause compilation error.
The variable offMode is used but never declared or defined in the class, which will cause a compilation error.
This needs to be defined. Based on context, it appears to check if LEDs are currently off. Consider this fix:
-if (offMode && (brightness < thresholdOn) ) {
+if (!strip.isOn && (brightness < thresholdOn) ) {Or if you need a custom off mode detection:
// In private members section
+bool offMode = false;
// In setup() or loop() where appropriate
+offMode = (strip.getMainSegment().mode == FX_MODE_STATIC && strip.getMainSegment().colors[0] == 0);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In usermods/LDR_PIR/LDR_PIR.cpp at line 153, the variable offMode is used but
not declared or defined, causing a compilation error. Define offMode
appropriately in the class or function scope before its usage, ensuring it
correctly reflects whether the LEDs are currently off, either by adding a
boolean member variable or a local variable initialized based on the LED state.
| if (offMode) { | ||
| pirActivated = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Logic issue with offMode check.
The logic if (offMode) { pirActivated = false; } unconditionally deactivates PIR when off mode is detected, which may not be the intended behavior.
Consider clarifying the intended behavior:
-if (offMode) {
- pirActivated = false;
+if (!strip.isOn) {
+ // Reset PIR activation when strip is turned off externally
+ pirActivated = false;
+ onPresetActive = false;🤖 Prompt for AI Agents
In usermods/LDR_PIR/LDR_PIR.cpp around lines 172 to 173, the current logic
unconditionally sets pirActivated to false whenever offMode is true, which may
not align with the intended behavior. Review the intended conditions under which
pirActivated should be deactivated and modify the if statement accordingly to
only deactivate pirActivated when offMode truly requires it, possibly adding
additional checks or restructuring the logic to reflect the correct behavior.
|
I used |
This usermod uses both a PIR and a LDR to activate an "On" preset when motion is detected and the reading on the LDR is below a configurable threshold. This allows motion to active LEDs only when it is dark.
Summary by CodeRabbit
New Features
Documentation
Chores