From 83748505b9050fdae646bee872f7c648b3b6d6bb Mon Sep 17 00:00:00 2001 From: burmistrzak <61958704+burmistrzak@users.noreply.github.com> Date: Sun, 30 Jun 2024 20:38:48 +0200 Subject: [PATCH] Improve UX of `WindowCovering` by using `motor_state` (#854) * Move `this.ignoreNextUpdateIfEqualToTarget` * Stop `updateTimer` only if `doIgnoreIfEqual` is false * Add support for `motor_state` * Check for `motor_state` in `CoverCreator` * Rework check for `motor_state` in `CoverCreator` * Refactor based on feedback * Rework `scaleAndUpdateCurrentPosition` * Cosmetic fix for Sonarcloud * Update changelog * Update docs for window covering --------- Co-authored-by: Arno Moonen --- CHANGELOG.md | 4 ++ docs/cover.md | 5 +- src/converters/cover.ts | 117 +++++++++++++++++++++++++------ test/cover.spec.ts | 103 +++++++++++++++++++++++++++ test/exposes/bosch/bmct-slz.json | 103 +++++++++++++++++++++++++++ 5 files changed, 309 insertions(+), 23 deletions(-) create mode 100644 test/exposes/bosch/bmct-slz.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b29b370..a1200bd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ Since version 1.0.0, we try to follow the [Semantic Versioning](https://semver.o ## [Unreleased] +### Changed + +- Window Covering now uses `motor_state` (if provided) to improve the user experience in the Home.app (see [#852](https://github.com/itavero/homebridge-z2m/issues/852) / [#854](https://github.com/itavero/homebridge-z2m/issues/854)) + ## [1.11.0-beta.5] - 2024-05-21 ### Fixed diff --git a/docs/cover.md b/docs/cover.md index 1310d74c..1588cd32 100644 --- a/docs/cover.md +++ b/docs/cover.md @@ -7,6 +7,7 @@ The table below shows how the different features within this `exposes` entry are | `position` | published, set | [Current Position](https://developers.homebridge.io/#/characteristic/CurrentPosition) (for the value from MQTT),
[Target Position](https://developers.homebridge.io/#/characteristic/TargetPosition) (for the value set from HomeKit) | Required (unless `tilt` is present) | | `tilt` | published, set | [Current Horizontal Tilt Angle](https://developers.homebridge.io/#/characteristic/CurrentHorizontalTiltAngle) (for the value from MQTT),
[Target Horizontal Tilt Angle](https://developers.homebridge.io/#/characteristic/TargetHorizontalTiltAngle) (for the value set from HomeKit)| Optional. Will be used as _Current Position_ if `position` is not available. | -The required [Position State](https://developers.homebridge.io/#/characteristic/PositionState) characteristic is set when the _Target Position_ is changed or when an `position` is received from MQTT (and the movement is assumed to be stopped). +If the device definition also provides a `motor_state`, it will be used to update the required [Position State](https://developers.homebridge.io/#/characteristic/PositionState) characteristic. -If the `position` can be _get_, the plugin will try to get frequent updates, after changing the _Target Position_. If the same position is reported twice, movement is assumed to be stopped. \ No newline at end of file +If `motor_state` is not provided, the [Position State](https://developers.homebridge.io/#/characteristic/PositionState) characteristic is set when the _Target Position_ is changed or when an `position` is received from MQTT (and the movement is assumed to be stopped). +Additionally, if the `position` can be _get_, the plugin will try to get frequent updates, after changing the _Target Position_. If the same position is reported twice, movement is assumed to be stopped. diff --git a/src/converters/cover.ts b/src/converters/cover.ts index 5be09f48..50f59989 100644 --- a/src/converters/cover.ts +++ b/src/converters/cover.ts @@ -27,12 +27,28 @@ export class CoverCreator implements ServiceCreator { exposesHasFeatures(e) && !accessory.isServiceHandlerIdKnown(CoverHandler.generateIdentifier(e.endpoint)) ) - .forEach((e) => this.createService(e as ExposesEntryWithFeatures, accessory)); + .forEach((e) => { + const motorStateExpose = exposes.find( + (m) => + m.endpoint === e.endpoint && + m.name === 'motor_state' && + exposesHasEnumProperty(m) && + exposesIsPublished(m) && + m.values.includes(CoverHandler.MOTOR_STATE_OPENING) && + m.values.includes(CoverHandler.MOTOR_STATE_CLOSING) && + m.values.includes(CoverHandler.MOTOR_STATE_STOPPED) + ); + this.createService(e as ExposesEntryWithFeatures, accessory, motorStateExpose as ExposesEntryWithEnumProperty); + }); } - private createService(expose: ExposesEntryWithFeatures, accessory: BasicAccessory): void { + private createService( + expose: ExposesEntryWithFeatures, + accessory: BasicAccessory, + motorStateExpose: ExposesEntryWithEnumProperty | undefined + ): void { try { - const handler = new CoverHandler(expose, accessory); + const handler = new CoverHandler(expose, accessory, motorStateExpose); accessory.registerServiceHandler(handler); } catch (error) { accessory.log.warn(`Failed to setup cover for accessory ${accessory.displayName} from expose "${JSON.stringify(expose)}": ${error}`); @@ -42,6 +58,9 @@ export class CoverCreator implements ServiceCreator { class CoverHandler implements ServiceHandler { private static readonly STATE_HOLD_POSITION = 'STOP'; + public static readonly MOTOR_STATE_OPENING = 'opening'; + public static readonly MOTOR_STATE_CLOSING = 'closing'; + public static readonly MOTOR_STATE_STOPPED = 'stopped'; private readonly positionExpose: ExposesEntryWithNumericRangeProperty; private readonly tiltExpose: ExposesEntryWithNumericRangeProperty | undefined; private readonly stateExpose: ExposesEntryWithEnumProperty | undefined; @@ -58,12 +77,16 @@ class CoverHandler implements ServiceHandler { private ignoreNextUpdateIfEqualToTarget: boolean; private lastPositionSet = -1; private positionCurrent = -1; + private motorState: string | undefined; + private motorStatePrevious: string | undefined; + private setTargetPositionHandled: boolean; public readonly mainCharacteristics: Characteristic[] = []; constructor( expose: ExposesEntryWithFeatures, - private readonly accessory: BasicAccessory + private readonly accessory: BasicAccessory, + private readonly motorStateExpose: ExposesEntryWithEnumProperty | undefined ) { const endpoint = expose.endpoint; this.identifier = CoverHandler.generateIdentifier(endpoint); @@ -151,11 +174,14 @@ class CoverHandler implements ServiceHandler { getOrAddCharacteristic(this.service, hap.Characteristic.HoldPosition).on('set', this.handleSetHoldPosition.bind(this)); } - if (exposesCanBeGet(this.positionExpose)) { + if (exposesCanBeGet(this.positionExpose) && this.motorStateExpose === undefined) { this.updateTimer = new ExtendedTimer(this.requestPositionUpdate.bind(this), 4000); } this.waitingForUpdate = false; this.ignoreNextUpdateIfEqualToTarget = false; + this.setTargetPositionHandled = false; + this.motorState = undefined; + this.motorStatePrevious = undefined; } identifier: string; @@ -167,12 +193,28 @@ class CoverHandler implements ServiceHandler { if (this.tiltExpose !== undefined && exposesCanBeGet(this.tiltExpose)) { keys.push(this.tiltExpose.property); } + if (this.motorStateExpose !== undefined && exposesCanBeGet(this.motorStateExpose)) { + keys.push(this.motorStateExpose.property); + } return keys; } updateState(state: Record): void { this.monitors.forEach((m) => m.callback(state)); + if (this.motorStateExpose !== undefined && this.motorStateExpose.property in state) { + const latestMotorState = state[this.motorStateExpose.property] as string; + switch (latestMotorState) { + case CoverHandler.MOTOR_STATE_OPENING: + case CoverHandler.MOTOR_STATE_CLOSING: + this.motorState = latestMotorState; + break; + default: + this.motorState = CoverHandler.MOTOR_STATE_STOPPED; + break; + } + } + if (this.positionExpose.property in state) { const latestPosition = state[this.positionExpose.property] as number; @@ -251,15 +293,44 @@ class CoverHandler implements ServiceHandler { this.current_min, this.current_max ); - - this.service.updateCharacteristic(hap.Characteristic.CurrentPosition, characteristicValue); - - if (isStopped) { - // Update target position and position state - // This should improve the UX in the Home.app - this.accessory.log.debug(`${this.accessory.displayName}: cover: assume movement stopped`); - this.service.updateCharacteristic(hap.Characteristic.PositionState, hap.Characteristic.PositionState.STOPPED); - this.service.updateCharacteristic(hap.Characteristic.TargetPosition, characteristicValue); + if (this.motorStateExpose === undefined) { + this.service.updateCharacteristic(hap.Characteristic.CurrentPosition, characteristicValue); + if (isStopped) { + // Update target position and position state + // This should improve the UX in the Home.app + this.accessory.log.debug(`${this.accessory.displayName}: cover: assume movement stopped`); + this.service.updateCharacteristic(hap.Characteristic.PositionState, hap.Characteristic.PositionState.STOPPED); + this.service.updateCharacteristic(hap.Characteristic.TargetPosition, characteristicValue); + } + } else if (this.motorState !== this.motorStatePrevious) { + let newPositionState: number; + let newTargetPosition: number; + switch (this.motorState) { + case CoverHandler.MOTOR_STATE_CLOSING: + newPositionState = hap.Characteristic.PositionState.DECREASING; + newTargetPosition = 0; + this.accessory.log.debug(`${this.accessory.displayName}: cover: closing via motor_state`); + break; + case CoverHandler.MOTOR_STATE_OPENING: + newPositionState = hap.Characteristic.PositionState.INCREASING; + newTargetPosition = 100; + this.accessory.log.debug(`${this.accessory.displayName}: cover: opening via motor_state`); + break; + default: + newPositionState = hap.Characteristic.PositionState.STOPPED; + newTargetPosition = characteristicValue; + this.accessory.log.debug(`${this.accessory.displayName}: cover: stopped via motor_state`); + break; + } + this.service.updateCharacteristic(hap.Characteristic.PositionState, newPositionState); + if (newPositionState === hap.Characteristic.PositionState.STOPPED) { + this.service.updateCharacteristic(hap.Characteristic.CurrentPosition, characteristicValue); + } + if (newPositionState === hap.Characteristic.PositionState.STOPPED || !this.setTargetPositionHandled) { + this.service.updateCharacteristic(hap.Characteristic.TargetPosition, newTargetPosition); + } + this.motorStatePrevious = this.motorState; + this.setTargetPositionHandled = false; } } @@ -276,15 +347,19 @@ class CoverHandler implements ServiceHandler { data[this.positionExpose.property] = target; this.accessory.queueDataForSetAction(data); - // Assume position state based on new target - if (target > this.positionCurrent) { - this.service.updateCharacteristic(hap.Characteristic.PositionState, hap.Characteristic.PositionState.INCREASING); - } else if (target < this.positionCurrent) { - this.service.updateCharacteristic(hap.Characteristic.PositionState, hap.Characteristic.PositionState.DECREASING); + if (this.motorStateExpose === undefined) { + // Assume position state based on new target + if (target > this.positionCurrent) { + this.service.updateCharacteristic(hap.Characteristic.PositionState, hap.Characteristic.PositionState.INCREASING); + } else if (target < this.positionCurrent) { + this.service.updateCharacteristic(hap.Characteristic.PositionState, hap.Characteristic.PositionState.DECREASING); + } else { + this.service.updateCharacteristic(hap.Characteristic.PositionState, hap.Characteristic.PositionState.STOPPED); + } } else { - this.service.updateCharacteristic(hap.Characteristic.PositionState, hap.Characteristic.PositionState.STOPPED); + this.service.updateCharacteristic(hap.Characteristic.TargetPosition, target); + this.setTargetPositionHandled = true; } - // Store last sent position for future reference this.lastPositionSet = target; diff --git a/test/cover.spec.ts b/test/cover.spec.ts index 1f17df2a..bfa7e9d7 100644 --- a/test/cover.spec.ts +++ b/test/cover.spec.ts @@ -413,4 +413,107 @@ describe('Cover', () => { harness.checkHomeKitUpdate(hap.Service.WindowCovering, 'state', true, { state: 'STOP' }); }); }); + + describe('Bosch Light/shutter control unit II', () => { + // Shared "state" + let deviceExposes: ExposesEntry[] = []; + let harness: ServiceHandlersTestHarness; + + 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('bosch/bmct-slz.json'); + expect(deviceExposes.length).toBeGreaterThan(0); + const newHarness = new ServiceHandlersTestHarness(); + + // Check service creation + const windowCovering = newHarness + .getOrAddHandler(hap.Service.WindowCovering) + .addExpectedCharacteristic('position', hap.Characteristic.CurrentPosition, false) + .addExpectedCharacteristic('target_position', hap.Characteristic.TargetPosition, true) + .addExpectedCharacteristic('position_state', hap.Characteristic.PositionState, false) + .addExpectedCharacteristic('state', hap.Characteristic.HoldPosition, true); + newHarness.prepareCreationMocks(); + + const positionCharacteristicMock = windowCovering.getCharacteristicMock('position'); + if (positionCharacteristicMock !== undefined) { + positionCharacteristicMock.props.minValue = 0; + positionCharacteristicMock.props.maxValue = 100; + } + + const targetPositionCharacteristicMock = windowCovering.getCharacteristicMock('target_position'); + if (targetPositionCharacteristicMock !== undefined) { + targetPositionCharacteristicMock.props.minValue = 0; + targetPositionCharacteristicMock.props.maxValue = 100; + } + + newHarness.callCreators(deviceExposes); + + newHarness.checkCreationExpectations(); + newHarness.checkHasMainCharacteristics(); + newHarness.checkExpectedGetableKeys(['position']); + harness = newHarness; + } + harness?.clearMocks(); + }); + + afterEach(() => { + verifyAllWhenMocksCalled(); + resetAllWhenMocks(); + }); + + test('Status update is handled: Position changes', () => { + expect(harness).toBeDefined(); + + // First update (previous state is unknown, so) + harness.checkUpdateState( + '{"position":100, "motor_state":"stopped"}', + hap.Service.WindowCovering, + new Map([ + [hap.Characteristic.CurrentPosition, 100], + [hap.Characteristic.PositionState, hap.Characteristic.PositionState.STOPPED], + [hap.Characteristic.TargetPosition, 100], + ]) + ); + harness.clearMocks(); + }); + + test('HomeKit: Hold position', () => { + expect(harness).toBeDefined(); + + harness.checkHomeKitUpdate(hap.Service.WindowCovering, 'state', true, { state: 'STOP' }); + }); + + test('HomeKit: Change target position', () => { + expect(harness).toBeDefined(); + + // Ignore known stopped position + harness.checkUpdateStateIsIgnored('{"position":100, "motor_state":"stopped"}'); + harness.clearMocks(); + + // Check changing the position to a lower value + harness.checkHomeKitUpdate(hap.Service.WindowCovering, 'target_position', 51, { position: 51 }); + harness.getOrAddHandler(hap.Service.WindowCovering).checkCharacteristicUpdates(new Map([[hap.Characteristic.TargetPosition, 51]])); + harness.clearMocks(); + + harness.checkUpdateState( + '{"position":41, "motor_state":"closing"}', + hap.Service.WindowCovering, + new Map([[hap.Characteristic.PositionState, hap.Characteristic.PositionState.DECREASING]]) + ); + harness.clearMocks(); + + harness.checkUpdateState( + '{"position":51, "motor_state":"stopped"}', + hap.Service.WindowCovering, + new Map([ + [hap.Characteristic.CurrentPosition, 51], + [hap.Characteristic.PositionState, hap.Characteristic.PositionState.STOPPED], + [hap.Characteristic.TargetPosition, 51], + ]) + ); + harness.clearMocks(); + }); + }); }); diff --git a/test/exposes/bosch/bmct-slz.json b/test/exposes/bosch/bmct-slz.json new file mode 100644 index 00000000..b5d15e27 --- /dev/null +++ b/test/exposes/bosch/bmct-slz.json @@ -0,0 +1,103 @@ +[ + { + "access": 7, + "description": "Module controlled by a rocker switch or a button", + "label": "Switch type", + "name": "switch_type", + "property": "switch_type", + "type": "enum", + "values": [ + "button", + "button_key_change", + "rocker_switch", + "rocker_switch_key_change" + ] + }, + { + "access": 1, + "category": "diagnostic", + "description": "Link quality (signal strength)", + "label": "Linkquality", + "name": "linkquality", + "property": "linkquality", + "type": "numeric", + "unit": "lqi", + "value_max": 255, + "value_min": 0 + }, + { + "features": [ + { + "access": 3, + "label": "State", + "name": "state", + "property": "state", + "type": "enum", + "values": [ + "OPEN", + "CLOSE", + "STOP" + ] + }, + { + "access": 7, + "description": "Position of this cover", + "label": "Position", + "name": "position", + "property": "position", + "type": "numeric", + "unit": "%", + "value_max": 100, + "value_min": 0 + } + ], + "type": "cover" + }, + { + "access": 1, + "description": "Shutter motor actual state ", + "label": "Motor state", + "name": "motor_state", + "property": "motor_state", + "type": "enum", + "values": [ + "stopped", + "opening", + "closing" + ] + }, + { + "access": 7, + "description": "Enable/Disable child lock", + "label": "Child lock", + "name": "child_lock", + "property": "child_lock", + "type": "binary", + "value_off": "OFF", + "value_on": "ON" + }, + { + "access": 7, + "description": "Calibration closing time", + "endpoint": "closing_time", + "label": "Calibration", + "name": "calibration", + "property": "calibration_closing_time", + "type": "numeric", + "unit": "s", + "value_max": 90, + "value_min": 1 + }, + { + "access": 7, + "description": "Calibration opening time", + "endpoint": "opening_time", + "label": "Calibration", + "name": "calibration", + "property": "calibration_opening_time", + "type": "numeric", + "unit": "s", + "value_max": 90, + "value_min": 1 + } +] \ No newline at end of file