Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose moving as motion sensor #956

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+119 −1
Open

Expose moving as motion sensor #956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf4b05f
Expose moving as motion sensor
torandreroland Nov 16, 2024
a117ab6
Added documentation, test and created changelog entry
torandreroland Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,4 +138,5 @@ inspectionProfiles/
# Other things
reports/
_*.json
**/_*.ts
**/_*.ts
itavero marked this conversation as resolved.
Show resolved Hide resolved
.DS_Store
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link

@coderabbitai coderabbitai bot Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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:

  1. Adding the PR reference
  2. Specifying which devices are affected
  3. 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.

Suggested change
- 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))

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itavero, you can simply append the link to PR #956 at the end of the line, similar to how you usually reference other PRs in the changelog. For instance:

Expose moving state as motion sensor for SmartThings/Samjin Multipurpose sensors (see #956)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not add the details you suggested (as there probably more devices that use this same property). The link seems fine.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itavero Understood! We'll keep it concise and just include the link. Thank you for clarifying.

(_/)
(·_·) …
/ \ … (sigh)


## [1.11.0-beta.6] - 2024-06-30

### Changed
Expand Down
1 change: 1 addition & 0 deletions docs/sensors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ The following table shows the possible exposes entries and the services and char
| `contact` | published | [Contact Sensor](https://developers.homebridge.io/#/service/ContactSensor) | [Contact Sensor State](https://developers.homebridge.io/#/characteristic/ContactSensorState) | |
| `occupancy` | published | [Occupancy Sensor](https://developers.homebridge.io/#/service/OccupancySensor) | [Occupancy Detected](https://developers.homebridge.io/#/characteristic/OccupancyDetected) | |
| `vibration` | published | [Motion Sensor](https://developers.homebridge.io/#/service/MotionSensor) | [Motion Detected](https://developers.homebridge.io/#/characteristic/MotionDetected) | |
| `moving` | published | [Motion Sensor](https://developers.homebridge.io/#/service/MotionSensor) | [Motion Detected](https://developers.homebridge.io/#/characteristic/MotionDetected) | |
itavero marked this conversation as resolved.
Show resolved Hide resolved
| `smoke` | published | [Smoke Sensor](https://developers.homebridge.io/#/service/SmokeSensor) | [Smoke Detected](https://developers.homebridge.io/#/characteristic/SmokeDetected) | |
| `carbon_monoxide` | published | [Carbon Monoxide Sensor](https://developers.homebridge.io/#/service/CarbonMonoxideSensor) | [Carbon Monoxide Detected](https://developers.homebridge.io/#/characteristic/CarbonMonoxideDetected) | |
| `co2` | published | [Carbon Dioxide Sensor](https://developers.homebridge.io/#/service/CarbonDioxideSensor) | [Carbon Dioxide Detected](https://developers.homebridge.io/#/characteristic/CarbonDioxideDetected) / [Carbon Dioxide Level](https://developers.homebridge.io/#/characteristic/CarbonDioxideLevel) | Detection threshold is currently fixed at 1200 ppm |
Expand Down
2 changes: 2 additions & 0 deletions src/converters/basic_sensors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { WaterLeakSensorHandler, GasLeakSensorHandler } from './basic_sensors/le
import { CarbonMonoxideSensorHandler } from './basic_sensors/carbon_monoxide';
import { SmokeSensorHandler } from './basic_sensors/smoke';
import { VibrationSensorHandler } from './basic_sensors/vibration';
import { MovingSensorHandler } from './basic_sensors/moving';
import { PresenceSensorHandler } from './basic_sensors/presence';
import { OccupancySensorHandler } from './basic_sensors/occupancy';
import { IdentifierGenerator } from './basic_sensors/basic';
Expand Down Expand Up @@ -52,6 +53,7 @@ export class BasicSensorCreator implements ServiceCreator {
OccupancySensorHandler,
PresenceSensorHandler,
VibrationSensorHandler,
MovingSensorHandler,
SmokeSensorHandler,
CarbonMonoxideSensorHandler,
WaterLeakSensorHandler,
Expand Down
30 changes: 30 additions & 0 deletions src/converters/basic_sensors/moving.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
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
);
itavero marked this conversation as resolved.
Show resolved Hide resolved
}
Comment on lines +9 to +21
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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;
}
Comment on lines +23 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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)

}
Comment on lines +1 to +30
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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:

  1. The mapping between the "moving" state and motion detection
  2. 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)

44 changes: 44 additions & 0 deletions test/basic_sensors.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,4 +645,48 @@ describe('Basic Sensors', () => {
);
});
});

describe('SmartThings IM6001-MPP01', () => {
// Shared "state"
let deviceExposes: ExposesEntry[] = [];
let harness: ServiceHandlersTestHarness;
let movingSensorId: string;

beforeEach(() => {
// Only test service creation for first test case and reuse harness afterwards
if (deviceExposes.length === 0 && harness === undefined) {
// Load exposes from JSON
deviceExposes = loadExposesFromFile('smartthings/im6001-mpp01.json');
expect(deviceExposes.length).toBeGreaterThan(0);
const newHarness = new ServiceHandlersTestHarness();

// Check service creation
movingSensorId = 'moving_' + hap.Service.MotionSensor.UUID;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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)

newHarness
.getOrAddHandler(hap.Service.MotionSensor, 'moving', movingSensorId)
.addExpectedCharacteristic('moving', hap.Characteristic.MotionDetected);
newHarness.prepareCreationMocks();

newHarness.callCreators(deviceExposes);

newHarness.checkCreationExpectations();
newHarness.checkHasMainCharacteristics();
newHarness.checkExpectedGetableKeys([]);
harness = newHarness;
}
harness?.clearMocks();
});

afterEach(() => {
verifyAllWhenMocksCalled();
resetAllWhenMocks();
});

test('Update moving', (): void => {
expect(harness).toBeDefined();
harness.checkSingleUpdateState('{"moving":false}', movingSensorId, hap.Characteristic.MotionDetected, false);
harness.clearMocks();
harness.checkSingleUpdateState('{"moving":true}', movingSensorId, hap.Characteristic.MotionDetected, true);
});
});
});
36 changes: 36 additions & 0 deletions test/exposes/smartthings/im6001-mpp01.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
[
{
"name": "battery",
"label": "Battery",
"access": 1,
"type": "numeric",
"property": "battery",
"description": "Remaining battery in %, can take up to 24 hours before reported",
"category": "diagnostic",
"unit": "%",
"value_max": 100,
"value_min": 0
},
{
"name": "moving",
"label": "Moving",
"access": 1,
"type": "binary",
"property": "moving",
"description": "Indicates whether the device is moving",
"value_on": true,
"value_off": false
},
itavero marked this conversation as resolved.
Show resolved Hide resolved
{
"name": "linkquality",
"label": "Linkquality",
"access": 1,
"type": "numeric",
"property": "linkquality",
"description": "Link quality (signal strength)",
"category": "diagnostic",
"unit": "lqi",
"value_max": 255,
"value_min": 0
}
]