feat(matter): adds HoldTime attribute to the Matter Occupancy endpoint #12146
Conversation
👋 Hello SuGlider, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results 90 files 90 suites 30m 17s ⏱️ For more details on these failures, see this check. Results for commit 743de9f. ♻️ This comment has been updated with latest results. |
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
My test with HA worked too. |
pedrominatel
left a comment
pedrominatel
left a comment
There was a problem hiding this comment.
Nice work @SuGlider. I wish all the README's be like this!
There was a problem hiding this comment.
Pull request overview
This PR adds HoldTime and HoldTimeLimits attributes to the Matter Occupancy Sensor endpoint, allowing control over how long the sensor maintains an "occupied" state after detection. The implementation includes a callback mechanism for Matter Controller updates and demonstrates HoldTime expiration logic in a comprehensive example.
Key changes:
- Added HoldTime attribute support with getter/setter methods and validation against configurable limits
- Implemented custom AttributeAccessInterface wrapper to intercept HoldTime writes and trigger user callbacks
- Added complete example demonstrating HoldTime functionality with persistence across reboots using Preferences
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| libraries/Matter/src/MatterEndpoints/MatterOccupancySensor.h | Adds HoldTime/HoldTimeLimits API methods, callback typedef, private member variables, and friend class declaration |
| libraries/Matter/src/MatterEndpoints/MatterOccupancySensor.cpp | Implements HoldTime functionality with attribute creation, validation, event loop scheduling, and custom AttributeAccessInterface wrapper |
| libraries/Matter/keywords.txt | Adds keywords for new types and methods (OccupancySensorType_t, HoldTimeChangeCB, setHoldTime, getHoldTime, setHoldTimeLimits, onHoldTimeChange, sensor type literals) |
| libraries/Matter/examples/MatterOccupancyWithHoldTime/ci.yml | Configures CI with huge_app partition scheme and Matter data model requirement |
| libraries/Matter/examples/MatterOccupancyWithHoldTime/README.md | Comprehensive documentation covering features, setup, HoldTime behavior, PIR sensor integration, and troubleshooting |
| libraries/Matter/examples/MatterOccupancyWithHoldTime/MatterOccupancyWithHoldTime.ino | Complete example with simulated occupancy sensor, HoldTime persistence, callback handling, and commissioning flow |
| docs/en/matter/ep_occupancy_sensor.rst | Documents new HoldTime API methods with examples and usage guidance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Simulated Occupancy Sensor with HoldTime support | ||
| // When occupancy is detected, it holds the "occupied" state for HoldTime seconds | ||
| // After HoldTime expires, it automatically switches to "unoccupied" | ||
| // | ||
| // Behavior for different HoldTime vs detectionInterval relationships: | ||
| // - holdTime_ms < detectionInterval: State switches to unoccupied after HoldTime, then waits for next detection | ||
| // - holdTime_ms == detectionInterval: If detections keep coming, timer resets (continuous occupancy) | ||
| // - holdTime_ms > detectionInterval: If detections keep coming, timer resets (continuous occupancy) | ||
| // If detections stop, HoldTime expires after the last detection | ||
| bool simulatedHWOccupancySensor() { | ||
|
|
||
| static bool occupancyState = false; | ||
| static uint32_t lastDetectionTime = 0; | ||
| static uint32_t lastDetectionEvent = millis(); | ||
| const uint32_t detectionInterval = 120000; // Simulate detection every 2 minutes | ||
|
|
||
| // Get current HoldTime from the sensor (can be changed by Matter Controller) | ||
| uint16_t holdTime_ms = OccupancySensor.getHoldTime() * 1000; // Convert seconds to milliseconds | ||
|
|
||
| // Check HoldTime expiration FIRST (before processing new detections) | ||
| // This ensures HoldTime can expire even if a detection occurs in the same iteration | ||
| if (occupancyState && (millis() - lastDetectionTime > holdTime_ms)) { | ||
| occupancyState = false; | ||
| // Reset detection interval counter so next detection can happen immediately | ||
| // This makes the simulation more responsive after the room becomes unoccupied | ||
| lastDetectionEvent = millis(); | ||
| Serial.println("HoldTime expired. Switching to unoccupied state."); | ||
| } | ||
|
|
||
| // Simulate periodic occupancy detection (e.g., motion detected) | ||
| // Check this AFTER HoldTime expiration so new detections can immediately re-trigger occupancy | ||
| if (millis() - lastDetectionEvent > detectionInterval) { | ||
| // New detection event occurred | ||
| lastDetectionEvent = millis(); | ||
|
|
||
| if (!occupancyState) { | ||
| // Transition from unoccupied to occupied - start hold timer | ||
| occupancyState = true; | ||
| lastDetectionTime = millis(); | ||
| Serial.printf("Occupancy detected! Holding state for %u seconds (HoldTime)\n", OccupancySensor.getHoldTime()); | ||
| } else { | ||
| // Already occupied - new detection extends the hold period by resetting the timer | ||
| // This simulates continuous occupancy (person still present) | ||
| lastDetectionTime = millis(); | ||
| Serial.printf("Occupancy still detected. Resetting hold timer to %u seconds (HoldTime)\n", OccupancySensor.getHoldTime()); | ||
| } | ||
| } | ||
|
|
||
| return occupancyState; | ||
| } |
There was a problem hiding this comment.
This function is defined at the global scope outside of setup() and loop(), but it appears before loop() in the file. While this is valid C++, it may be clearer to place helper functions either before setup() or after loop() to maintain a consistent code structure. Consider moving this function declaration before setup() or adding a forward declaration.
| // Update member variable immediately | ||
| holdTime_seconds = _holdTime_seconds; | ||
| log_v("HoldTime scheduled for update with value %u seconds", _holdTime_seconds); |
There was a problem hiding this comment.
There's a potential race condition here. The member variable holdTime_seconds is updated immediately after scheduling the lambda, but the actual Matter attribute update happens asynchronously in the event loop. If setHoldTime() is called multiple times in quick succession, or if a Matter Controller writes to HoldTime while a local update is pending, the member variable and the actual attribute value could become out of sync. Consider updating the member variable inside the lambda callback after successful attribute update, or add synchronization to prevent concurrent updates.
| const uint32_t detectionInterval = 120000; // Simulate detection every 2 minutes | ||
|
|
||
| // Get current HoldTime from the sensor (can be changed by Matter Controller) | ||
| uint16_t holdTime_ms = OccupancySensor.getHoldTime() * 1000; // Convert seconds to milliseconds |
There was a problem hiding this comment.
Potential integer overflow when converting seconds to milliseconds. The return value from getHoldTime() is uint16_t (max 65535), and multiplying by 1000 could overflow if the value exceeds 65 seconds. For example, HOLD_TIME_MAX is set to 3600 seconds (1 hour), which would result in 3,600,000 milliseconds - this exceeds the uint16_t range and would wrap around. Declare holdTime_ms as uint32_t instead of assigning the result to a uint16_t variable to prevent overflow.
| if (holdTimeMax_seconds > 0) { | ||
| // Limits are set, validate the new value | ||
| if (_holdTime_seconds < holdTimeMin_seconds) { | ||
| log_e("HoldTime (%u) is below minimum (%u seconds)", _holdTime_seconds, holdTimeMin_seconds); | ||
| return false; | ||
| } | ||
| if (_holdTime_seconds > holdTimeMax_seconds) { | ||
| log_e("HoldTime (%u) exceeds maximum (%u seconds)", _holdTime_seconds, holdTimeMax_seconds); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The validation logic checks if holdTimeMax_seconds > 0 to determine if limits are set. However, this creates an ambiguity where a max value of 0 would be interpreted as "no limits set" rather than an actual maximum of 0 seconds. Consider using a separate boolean flag to track whether limits have been explicitly set, or document that a max value of 0 means "no validation" to avoid confusion.
| MatterOccupancySensor* sensor = static_cast<MatterOccupancySensor*>(priv_data); | ||
| // Verify it's actually a MatterOccupancySensor by checking if it's started | ||
| if (sensor != nullptr && sensor->started) { | ||
| return sensor; | ||
| } |
There was a problem hiding this comment.
The verification that checks sensor->started to validate if the private data is a valid MatterOccupancySensor is fragile. If a sensor is stopped (started = false) but still exists, or if other endpoint types have similar boolean members, this check could fail or provide false positives. Consider using a more robust type identification mechanism, such as storing a magic number or type ID in the class, or using dynamic_cast if RTTI is available.
| attribute_t *hold_time_attr = create_hold_time(cluster, holdTime_seconds); | ||
| if (hold_time_attr == nullptr) { | ||
| log_e("Failed to create HoldTime attribute"); | ||
| // Continue anyway, as HoldTime is optional | ||
| } else { | ||
| log_d("HoldTime attribute created with value %u seconds", holdTime_seconds); | ||
| } |
There was a problem hiding this comment.
The HoldTime attribute is created with an initial value from holdTime_seconds, but if HoldTimeLimits are set later via setHoldTimeLimits(), there's no guarantee that the initial HoldTime value is within those limits. This could create an inconsistent state where the attribute exists with a value outside the limits that will be set later. Consider either creating the attribute with a value of 0, or documenting that users should call setHoldTimeLimits() before begin() if they want custom limits.
| return false; | ||
| } | ||
| if (_holdTime_seconds > holdTimeMax_seconds) { | ||
| log_e("HoldTime (%u) exceeds maximum (%u seconds)", _holdTime_seconds, holdTimeMax_seconds); |
There was a problem hiding this comment.
The error message format string is missing the units for the maximum value. The message reads "HoldTime (%u) exceeds maximum (%u seconds)" where only the second value has "seconds" specified. For consistency and clarity, either include "seconds" for both values or use a format like "HoldTime %u seconds exceeds maximum %u seconds".
Description of Change
This PR adds HoldTime and HoldTimeLimits attributes to Arduino Matter Occupancy Sensor endpoint.
It also adds a user callback for the device get notified that the Matter Controller has changed such parameter.
It is possible for the device to change both HoldTime and HoldTimeLimits using the Arduino Matter API.
Test Scenarios
Tested using Home Assistant with WiFi using the ESP32-C6.
The test uses the provided exmaple.
Related links
Closes #12135