Expose moving as motion sensor #956
Conversation
WalkthroughThe changes introduce a new sensor handler, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 3
🧹 Outside diff range comments (1)
src/converters/basic_sensors.ts (1)
Line range hint
1-94: Consider documenting motion sensor behavior.Since this change introduces motion sensor capabilities, it would be beneficial to document the expected behavior, especially regarding:
- Motion detection timeout/reset behavior
- Any specific handling for the SmartThings/Samjin Multipurpose sensor
- Integration considerations with home automation systems
Would you like me to help draft documentation for these aspects?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
src/converters/basic_sensors.ts(2 hunks)src/converters/basic_sensors/moving.ts(1 hunks)
🧰 Additional context used
🪛 Biome
src/converters/basic_sensors/moving.ts
[error] 16-16: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 24-24: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 26-26: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🔇 Additional comments (4)
src/converters/basic_sensors/moving.ts (2)
1-4: LGTM! All necessary dependencies are properly imported.
The imports are well-organized and include all required dependencies for implementing the motion sensor handler.
6-7: LGTM! Class structure aligns with PR objectives.
The MovingSensorHandler correctly extends BinarySensorHandler and defines the appropriate expose name for the moving state.
src/converters/basic_sensors.ts (2)
21-21: LGTM! Import follows established patterns.
The import statement for MovingSensorHandler follows the existing convention and is properly placed with other sensor handler imports.
56-56: LGTM! Handler registration looks good.
The MovingSensorHandler is correctly added to the static handlers array, maintaining consistency with other sensor handlers.
Let's verify the MovingSensorHandler implementation:
✅ Verification successful
MovingSensorHandler implementation is correctly structured and integrated
The verification confirms:
- Properly extends BinarySensorHandler with correct inheritance
- Correctly sets
exposesNameas 'moving' - Implements MotionSensor service with appropriate characteristics
- Uses consistent identifier generation pattern
- No conflicting usages of 'moving' property found elsewhere
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MovingSensorHandler implementation and its base class usage
# Check if MovingSensorHandler extends BinarySensorHandler as expected
ast-grep --pattern 'class MovingSensorHandler extends BinarySensorHandler {
$$$
}'
# Verify the exposesName is set to 'moving'
ast-grep --pattern 'static exposesName = $moving'
# Look for any existing usage of 'moving' property in the codebase
rg -l "['|\"]moving['|\"]"
Length of output: 2126
| import { BasicAccessory } from '../interfaces'; | ||
| import { ExposesEntryWithBinaryProperty, ExposesEntryWithProperty } from '../../z2mModels'; | ||
| import { hap } from '../../hap'; | ||
| import { BinarySensorHandler } from './binary'; | ||
|
|
||
| export class MovingSensorHandler extends BinarySensorHandler { | ||
| public static readonly exposesName: string = 'moving'; | ||
|
|
||
| constructor(expose: ExposesEntryWithProperty, otherExposes: ExposesEntryWithBinaryProperty[], accessory: BasicAccessory) { | ||
| super( | ||
| accessory, | ||
| expose as ExposesEntryWithBinaryProperty, | ||
| otherExposes, | ||
| MovingSensorHandler.generateIdentifier, | ||
| 'Motion Sensor (moving)', | ||
| (n, t) => new hap.Service.MotionSensor(n, (MovingSensorHandler.exposesName + ' ' + (t ?? '')).trim()), | ||
| hap.Characteristic.MotionDetected, | ||
| true, | ||
| false | ||
| ); | ||
| } | ||
|
|
||
| static generateIdentifier(endpoint: string | undefined) { | ||
| let identifier = MovingSensorHandler.exposesName + '_' + hap.Service.MotionSensor.UUID; | ||
| if (endpoint !== undefined) { | ||
| identifier += '_' + endpoint.trim(); | ||
| } | ||
| return identifier; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Implementation successfully meets PR objectives.
The MovingSensorHandler implementation effectively exposes the "moving" state as a motion sensor while following established patterns and HomeKit standards. The code is well-structured and maintainable.
Consider documenting the following in the PR description or README:
- The mapping between the "moving" state and motion detection
- Any specific behavior or limitations users should be aware of when using this with the SmartThings/Samjin Multipurpose sensor
🧰 Tools
🪛 Biome
[error] 16-16: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 24-24: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 26-26: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
| constructor(expose: ExposesEntryWithProperty, otherExposes: ExposesEntryWithBinaryProperty[], accessory: BasicAccessory) { | ||
| super( | ||
| accessory, | ||
| expose as ExposesEntryWithBinaryProperty, | ||
| otherExposes, | ||
| MovingSensorHandler.generateIdentifier, | ||
| 'Motion Sensor (moving)', | ||
| (n, t) => new hap.Service.MotionSensor(n, (MovingSensorHandler.exposesName + ' ' + (t ?? '')).trim()), | ||
| hap.Characteristic.MotionDetected, | ||
| true, | ||
| false | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
💡 Codebase verification
Refactor string concatenation to template literals across motion sensor handlers
Found similar string concatenation pattern in other motion sensor handlers. Let's update all instances for consistency:
src/converters/basic_sensors/moving.ts: Change to`${MovingSensorHandler.exposesName} ${t ?? ''}`.trim()src/converters/basic_sensors/vibration.ts: Change to`${VibrationSensorHandler.exposesName} ${t ?? ''}`.trim()
The HomeKit service configuration is consistent across all implementations, using the correct characteristic hap.Characteristic.MotionDetected with proper initialization.
🔗 Analysis chain
Refactor string concatenation to use template literals.
The constructor implementation is correct, but we can improve readability by using template literals.
Apply this change:
- (n, t) => new hap.Service.MotionSensor(n, (MovingSensorHandler.exposesName + ' ' + (t ?? '')).trim()),
+ (n, t) => new hap.Service.MotionSensor(n, `${MovingSensorHandler.exposesName} ${t ?? ''}`.trim()),Let's verify the HomeKit service configuration:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent motion sensor service usage across the codebase
rg -A 2 "new hap\.Service\.MotionSensor"
Length of output: 861
🧰 Tools
🪛 Biome
[error] 16-16: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
| static generateIdentifier(endpoint: string | undefined) { | ||
| let identifier = MovingSensorHandler.exposesName + '_' + hap.Service.MotionSensor.UUID; | ||
| if (endpoint !== undefined) { | ||
| identifier += '_' + endpoint.trim(); | ||
| } | ||
| return identifier; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refactor identifier generation to use template literals.
The identifier generation logic is correct, but we can improve readability by using template literals.
Apply these changes:
- let identifier = MovingSensorHandler.exposesName + '_' + hap.Service.MotionSensor.UUID;
+ let identifier = `${MovingSensorHandler.exposesName}_${hap.Service.MotionSensor.UUID}`;
if (endpoint !== undefined) {
- identifier += '_' + endpoint.trim();
+ identifier = `${identifier}_${endpoint.trim()}`;
}📝 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.
| static generateIdentifier(endpoint: string | undefined) { | |
| let identifier = MovingSensorHandler.exposesName + '_' + hap.Service.MotionSensor.UUID; | |
| if (endpoint !== undefined) { | |
| identifier += '_' + endpoint.trim(); | |
| } | |
| return identifier; | |
| } | |
| static generateIdentifier(endpoint: string | undefined) { | |
| let identifier = `${MovingSensorHandler.exposesName}_${hap.Service.MotionSensor.UUID}`; | |
| if (endpoint !== undefined) { | |
| identifier = `${identifier}_${endpoint.trim()}`; | |
| } | |
| return identifier; | |
| } |
🧰 Tools
🪛 Biome
[error] 24-24: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 26-26: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
|
To do:
|
|
I've updated docs, added test and created changelog entry. OK? |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)docs/sensors.md(1 hunks)test/basic_sensors.spec.ts(1 hunks)test/exposes/smartthings/im6001-mpp01.json(1 hunks)
🧰 Additional context used
🪛 Biome
test/basic_sensors.spec.ts
[error] 664-664: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🔇 Additional comments (6)
test/exposes/smartthings/im6001-mpp01.json (3)
2-13: LGTM: Battery configuration is well-defined
The battery sensor configuration includes all necessary attributes and follows standard patterns. The description helpfully notes the potential 24-hour reporting delay.
24-35: LGTM: Linkquality configuration is complete
The link quality sensor configuration includes all necessary attributes and follows standard patterns with correct LQI range (0-255).
1-36: Verify configuration against similar devices
Let's ensure this configuration is consistent with other SmartThings/Samjin sensor configurations in the codebase.
✅ Verification successful
Let me gather more information about the overall structure and attributes of similar SmartThings devices.
Configuration is consistent with other SmartThings devices
The configuration follows the same structure and attribute patterns as other SmartThings devices in the codebase:
- Identical structure for common attributes (battery, linkquality)
- Consistent attribute ordering and formatting
- Similar binary state definition pattern (value_on/value_off)
- Proper categorization of diagnostic attributes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and compare with other SmartThings sensor configurations
echo "Searching for similar SmartThings sensor configurations..."
fd -e json -p "test/exposes/**/smartthings" --exec cat {} \; | jq -c '.[] | select(.name == "moving" or .type == "binary")'
Length of output: 611
Script:
#!/bin/bash
# Compare full configuration structure with other SmartThings devices
echo "Checking other SmartThings device configurations..."
fd -e json -p "test/exposes/**/smartthings" --exec sh -c 'echo "=== $1 ==="; cat "$1"' sh {} \;
Length of output: 1939
.gitignore (1)
142-142: LGTM!
Adding .DS_Store to .gitignore is a good practice as these are macOS system files that should not be tracked in version control.
docs/sensors.md (1)
24-24: LGTM! Documentation format and technical mapping are accurate.
The new entry for the moving sensor maintains consistent formatting and correctly maps to the Motion Sensor service and Motion Detected characteristic.
test/basic_sensors.spec.ts (1)
649-691: LGTM! Well-structured test suite for the motion sensor.
The test suite follows the established patterns, properly tests both states of the motion sensor, and includes appropriate setup/teardown logic.
🧰 Tools
🪛 Biome
[error] 664-664: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
| @@ -6,6 +6,10 @@ Since version 1.0.0, we try to follow the [Semantic Versioning](https://semver.o | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Added | |||
|
|
|||
| - Expose moving as motion sensor. | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance the changelog entry with more details.
The current entry could be more descriptive. Consider expanding it to match the style of other entries in the changelog by:
- Adding the PR reference
- Specifying which devices are affected
- Including any configuration requirements
-Expose moving as motion sensor.
+Expose moving state as motion sensor for SmartThings/Samjin Multipurpose sensors (see [#956](https://github.com/itavero/homebridge-z2m/pull/956))📝 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.
| - Expose moving as motion sensor. | |
| - Expose moving state as motion sensor for SmartThings/Samjin Multipurpose sensors (see [#956](https://github.com/itavero/homebridge-z2m/pull/956)) |
There was a problem hiding this comment.
Still need to add a link to this PR at the end of this line (as I usually do for lines in the changelog, so people can find more information if they want it).
There was a problem hiding this comment.
I would not add the details you suggested (as there probably more devices that use this same property). The link seems fine.
There was a problem hiding this comment.
@itavero Understood! We'll keep it concise and just include the link. Thank you for clarifying.
(_/)
(·_·) …
/ \ … (sigh)
| const newHarness = new ServiceHandlersTestHarness(); | ||
|
|
||
| // Check service creation | ||
| movingSensorId = 'moving_' + hap.Service.MotionSensor.UUID; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use template literals for string concatenation
Consider using template literals for better readability.
-movingSensorId = 'moving_' + hap.Service.MotionSensor.UUID;
+movingSensorId = `moving_${hap.Service.MotionSensor.UUID}`;📝 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.
| movingSensorId = 'moving_' + hap.Service.MotionSensor.UUID; | |
| movingSensorId = `moving_${hap.Service.MotionSensor.UUID}`; |
🧰 Tools
🪛 Biome
[error] 664-664: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
|
|
🆗 Published SonarCloud and code coverage data. |
|
@itavero: Can this be merged now? |
| @@ -6,6 +6,10 @@ Since version 1.0.0, we try to follow the [Semantic Versioning](https://semver.o | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Added | |||
|
|
|||
| - Expose moving as motion sensor. | |||
There was a problem hiding this comment.
Still need to add a link to this PR at the end of this line (as I usually do for lines in the changelog, so people can find more information if they want it).
Sorry. Didn't notice that my few remarks were still pending to be published. Most of the time I use GitLab, so still have to get used to doing code reviews on GitHub. |
|
All right. Please let me know if there is something I should do. |
Expose moving as motion sensor to enable motion sensor for SmartThings/Samjin Multipurpose sensor.
Summary by CodeRabbit
New Features
MovingSensorHandlerto enhance motion sensor capabilities.BasicSensorCreatorto include the new handler in its management system.movingsensor, allowing devices to function as motion sensors.Documentation
movingsensor and its functionalities.Tests
These changes collectively improve the system's ability to handle motion sensors effectively and enhance user experience.