From c8eb503ae70a9de40a51f3d01ebae33c89e40012 Mon Sep 17 00:00:00 2001 From: Koen Kanters Date: Sun, 29 Sep 2019 14:35:05 +0200 Subject: [PATCH] Simplify group optimistic mode. https://github.com/Koenkk/zigbee2mqtt/issues/764 --- lib/controller.js | 4 ++-- lib/extension/groups.js | 34 ++++++++++++--------------- lib/state.js | 16 ++----------- lib/util/settings.js | 8 +++---- npm-shrinkwrap.json | 18 +++++++------- package.json | 1 - test/bridgeConfig.test.js | 4 ++-- test/group.test.js | 47 +++++++++++++++++-------------------- test/settings.test.js | 6 ++--- test/stub/zigbeeHerdsman.js | 2 +- 10 files changed, 59 insertions(+), 81 deletions(-) diff --git a/lib/controller.js b/lib/controller.js index 904dcdec..86e65ef1 100644 --- a/lib/controller.js +++ b/lib/controller.js @@ -239,7 +239,7 @@ class Controller { this.callExtensionMethod('onMQTTMessage', [topic, message]); } - async publishEntityState(IDorName, payload) { + async publishEntityState(IDorName, payload, stateChangeReason=null) { const entity = this.zigbee.resolveEntity(IDorName); if (!entity || !entity.settings) { logger.error(`'${IDorName}' does not exist, skipping publish`); @@ -251,7 +251,7 @@ class Controller { const newState = objectAssignDeep.noMutate(currentState, payload); // Update state cache with new state. - this.state.set(entity.settings.ID, newState); + this.state.set(entity.settings.ID, newState, stateChangeReason); if (settings.get().advanced.cache_state) { // Add cached state to payload diff --git a/lib/extension/groups.js b/lib/extension/groups.js index 06aed1cf..f17d2e01 100644 --- a/lib/extension/groups.js +++ b/lib/extension/groups.js @@ -66,6 +66,11 @@ class Groups extends BaseExtension { } async onStateChange(data) { + const reason = 'group_optimistic'; + if (data.reason === reason) { + return; + } + const properties = ['state', 'brightness', 'color_temp', 'color']; const payload = {}; @@ -77,28 +82,19 @@ class Groups extends BaseExtension { if (Object.keys(payload).length) { const entity = this.zigbee.resolveEntity(data.ID); - if (entity.type !== 'device') { - return; - } - const zigbeeGroups = this.zigbee.getGroups(); - for (const zigbeeGroup of zigbeeGroups) { - const settingsGroup = settings.getGroup(zigbeeGroup.groupID); - if (settingsGroup && zigbeeGroup.hasMember(entity.endpoint)) { - if (settingsGroup.optimistic === 'group' || settingsGroup.optimistic === 'group_devices') { - if (!this.state.is(zigbeeGroup.groupID, payload)) { - await this.publishEntityState(zigbeeGroup.groupID, payload); - } - - if (settingsGroup.optimistic === 'group_devices') { - for (const endpoint of zigbeeGroup.getMembers()) { - if (!this.state.is(endpoint.deviceIeeeAddress, payload)) { - await this.publishEntityState(endpoint.deviceIeeeAddress, payload); - } - } - } + if (entity.type === 'device') { + const zigbeeGroups = this.zigbee.getGroups(); + for (const zigbeeGroup of zigbeeGroups) { + const settingsGroup = settings.getGroup(zigbeeGroup.groupID); + if (settingsGroup && settingsGroup.optimistic && zigbeeGroup.hasMember(entity.endpoint)) { + await this.publishEntityState(zigbeeGroup.groupID, payload, reason); } } + } else { + for (const member of entity.group.getMembers()) { + await this.publishEntityState(member.deviceIeeeAddress, payload, reason); + } } } } diff --git a/lib/state.js b/lib/state.js index 24855386..1853c11e 100644 --- a/lib/state.js +++ b/lib/state.js @@ -3,7 +3,6 @@ const data = require('./util/data'); const fs = require('fs'); const objectAssignDeep = require('object-assign-deep'); const events = require('events'); -const equals = require('fast-deep-equal'); const saveInterval = 1000 * 60 * 5; // 5 minutes @@ -69,18 +68,7 @@ class State extends events.EventEmitter { return this.state[ID]; } - is(ID, expected) { - const current = this.get(ID); - if (!current) return false; - - for (const [key, value] of Object.entries(expected)) { - if (!equals(current[key], value)) return false; - } - - return true; - } - - set(ID, state) { + set(ID, state, reason=null) { const toState = objectAssignDeep.noMutate(state); dontCacheProperties.forEach((property) => { if (toState.hasOwnProperty(property)) { @@ -92,7 +80,7 @@ class State extends events.EventEmitter { this.state[ID] = toState; - this.emit('stateChange', {ID, from: fromState, to: toState}); + this.emit('stateChange', {ID, from: fromState, to: toState, reason}); } remove(ID) { diff --git a/lib/util/settings.js b/lib/util/settings.js index a0b3441d..03671472 100644 --- a/lib/util/settings.js +++ b/lib/util/settings.js @@ -222,7 +222,7 @@ const schema = { friendly_name: {type: 'string'}, retain: {type: 'boolean'}, devices: {type: 'array', items: {type: 'string'}}, - optimistic: {type: 'string', enum: ['disable', 'group', 'group_devices']}, + optimistic: {type: 'boolean'}, qos: {type: 'number'}, }, required: ['friendly_name', 'retain'], @@ -339,12 +339,12 @@ function getGroup(IDorName) { const settings = getWithDefaults(); const byID = settings.groups[IDorName]; if (byID) { - return {optimistic: 'group', devices: [], ...byID, ID: Number(IDorName), friendlyName: byID.friendly_name}; + return {optimistic: true, devices: [], ...byID, ID: Number(IDorName), friendlyName: byID.friendly_name}; } for (const [ID, group] of Object.entries(settings.groups)) { if (group.friendly_name === IDorName) { - return {optimistic: 'group', devices: [], ...group, ID: Number(ID), friendlyName: group.friendly_name}; + return {optimistic: true, devices: [], ...group, ID: Number(ID), friendlyName: group.friendly_name}; } } @@ -354,7 +354,7 @@ function getGroup(IDorName) { function getGroups() { const settings = getWithDefaults(); return Object.entries(settings.groups).map(([ID, group]) => { - return {optimistic: 'group', devices: [], ...group, ID: Number(ID), friendlyName: group.friendly_name}; + return {optimistic: true, devices: [], ...group, ID: Number(ID), friendlyName: group.friendly_name}; }); } diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index c167e8a9..2efcf9be 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -1107,9 +1107,9 @@ } }, "commander": { - "version": "2.20.0", - "resolved": "https://registry.npmjs.org/commander/-/commander-2.20.0.tgz", - "integrity": "sha512-7j2y+40w61zy6YC2iRNpUe/NwhNyoXrYpHMrSunaMG64nRnaf96zO/KMQR4OyN/UnE5KLyEBnKHd4aG3rskjpQ==", + "version": "2.20.1", + "resolved": "https://registry.npmjs.org/commander/-/commander-2.20.1.tgz", + "integrity": "sha512-cCuLsMhJeWQ/ZpsFTbE765kvVfoeSddc4nU3up4fV+fDBcfUXnbITJ+JzhkdjzOqhURjZgujxaioam4RM9yGUg==", "dev": true, "optional": true }, @@ -1557,9 +1557,9 @@ } }, "eslint": { - "version": "6.4.0", - "resolved": "https://registry.npmjs.org/eslint/-/eslint-6.4.0.tgz", - "integrity": "sha512-WTVEzK3lSFoXUovDHEbkJqCVPEPwbhCq4trDktNI6ygs7aO41d4cDT0JFAT5MivzZeVLWlg7vHL+bgrQv/t3vA==", + "version": "6.5.0", + "resolved": "https://registry.npmjs.org/eslint/-/eslint-6.5.0.tgz", + "integrity": "sha512-IIbSW+vKOqMatPmS9ayyku4tvWxHY2iricSRtOz6+ZA5IPRlgXzEL0u/j6dr4eha0ugmhMwDTqxtmNu3kj9O4w==", "dev": true, "requires": { "@babel/code-frame": "^7.0.0", @@ -4644,9 +4644,9 @@ "dev": true }, "react-is": { - "version": "16.10.0", - "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.10.0.tgz", - "integrity": "sha512-WRki2sBb7MTpYp7FtDEmSeGKX2vamYyq3rc9o7fKUG+/DHVyJu69NnvJsiSwwhh2Tt8XN40MQHkDBEXwyfxncQ==", + "version": "16.10.1", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.10.1.tgz", + "integrity": "sha512-BXUMf9sIOPXXZWqr7+c5SeOKJykyVr2u0UDzEf4LNGc6taGkQe1A9DFD07umCIXz45RLr9oAAwZbAJ0Pkknfaw==", "dev": true }, "read-pkg": { diff --git a/package.json b/package.json index e5ea3a26..c68a1053 100644 --- a/package.json +++ b/package.json @@ -35,7 +35,6 @@ "dependencies": { "ajv": "*", "debounce": "*", - "fast-deep-equal": "*", "git-last-commit": "*", "js-yaml": "*", "mkdir-recursive": "*", diff --git a/test/bridgeConfig.test.js b/test/bridgeConfig.test.js index 4e092f04..390eedc3 100644 --- a/test/bridgeConfig.test.js +++ b/test/bridgeConfig.test.js @@ -168,7 +168,7 @@ describe('Bridge config', () => { await flushPromises(); expect(MQTT.publish.mock.calls[0][0]).toStrictEqual('zigbee2mqtt/bridge/log'); const payload = JSON.parse(MQTT.publish.mock.calls[0][1]); - expect(payload).toStrictEqual({"message": [{"ID": 1, "friendly_name": "group_1", "retain": false, 'devices': [], optimistic: "group"}, {"ID": 2, "friendly_name": "group_2", "retain": false, "devices": [], optimistic: "group"}], "type": "groups"}); + expect(payload).toStrictEqual({"message": [{"ID": 1, "friendly_name": "group_1", "retain": false, 'devices': [], optimistic: true}, {"ID": 2, "friendly_name": "group_2", "retain": false, "devices": [], optimistic: true}], "type": "groups"}); }); it('Should allow rename devices', async () => { @@ -203,7 +203,7 @@ describe('Bridge config', () => { zigbeeHerdsman.createGroup.mockClear(); MQTT.events.message('zigbee2mqtt/bridge/config/add_group', 'new_group'); await flushPromises(); - expect(settings.getGroup('new_group')).toStrictEqual({"ID": 3, "friendlyName": "new_group", "friendly_name": "new_group", devices: [], optimistic: "group"}); + expect(settings.getGroup('new_group')).toStrictEqual({"ID": 3, "friendlyName": "new_group", "friendly_name": "new_group", devices: [], optimistic: true}); expect(zigbeeHerdsman.createGroup).toHaveBeenCalledTimes(1); expect(zigbeeHerdsman.createGroup).toHaveBeenCalledWith(3) }); diff --git a/test/group.test.js b/test/group.test.js index 4c51e662..18acdadb 100644 --- a/test/group.test.js +++ b/test/group.test.js @@ -248,10 +248,30 @@ describe('Groups', () => { await controller.start(); await flushPromises(); + MQTT.publish.mockClear(); const payload = {data: {onOff: 1}, cluster: 'genOnOff', device, endpoint, type: 'attributeReport', linkquality: 10}; await zigbeeHerdsman.events.message(payload); await flushPromises(); + expect(MQTT.publish).toHaveBeenCalledTimes(2); + expect(MQTT.publish).toHaveBeenCalledWith("zigbee2mqtt/bulb_color", '{"state":"ON","linkquality":10}', {"retain": false, qos: 0}, expect.any(Function)); + expect(MQTT.publish).toHaveBeenCalledWith("zigbee2mqtt/group_1", '{"state":"ON"}', {"retain": false, qos: 0}, expect.any(Function)); + }); + + it('Should publish state change of all members when a group changes its state', async () => { + const device = zigbeeHerdsman.devices.bulb_color; + const endpoint = device.getEndpoint(1); + const group = zigbeeHerdsman.groups.group_1; + group.members.push(endpoint); + settings.set(['groups'], {'1': {friendly_name: 'group_1', retain: false, devices: [device.ieeeAddr]}}); + await controller.start(); + await flushPromises(); + + MQTT.publish.mockClear(); + await MQTT.events.message('zigbee2mqtt/group_1/set', JSON.stringify({state: 'ON'})); + await flushPromises(); + expect(MQTT.publish).toHaveBeenCalledTimes(2); + expect(MQTT.publish).toHaveBeenCalledWith("zigbee2mqtt/bulb_color", '{"state":"ON"}', {"retain": false, qos: 0}, expect.any(Function)); expect(MQTT.publish).toHaveBeenCalledWith("zigbee2mqtt/group_1", '{"state":"ON"}', {"retain": false, qos: 0}, expect.any(Function)); }); @@ -260,7 +280,7 @@ describe('Groups', () => { const endpoint = device.getEndpoint(1); const group = zigbeeHerdsman.groups.group_1; group.members.push(endpoint); - settings.set(['groups'], {'1': {friendly_name: 'group_1', devices: [device.ieeeAddr], optimistic: 'disable', retain: false}}); + settings.set(['groups'], {'1': {friendly_name: 'group_1', devices: [device.ieeeAddr], optimistic: false, retain: false}}); await controller.start(); await flushPromises(); @@ -273,29 +293,4 @@ describe('Groups', () => { expect(MQTT.publish).toHaveBeenCalledTimes(1); expect(MQTT.publish).toHaveBeenCalledWith("zigbee2mqtt/bulb_color", '{"state":"ON","linkquality":10}', {"retain": false, qos: 0}, expect.any(Function)); }); - - it('Should publish group and members state change when a group is in optimistic group_devices', async () => { - const device1 = zigbeeHerdsman.devices.bulb_color; - const device2 = zigbeeHerdsman.devices.bulb; - const group = zigbeeHerdsman.groups.group_1; - group.members.push(device1.getEndpoint(1)); - group.members.push(device2.getEndpoint(1)); - - settings.set(['groups'], {'1': {friendly_name: 'group_1', devices: [device1.ieeeAddr, device2.ieeeAddr], optimistic: 'group_devices', retain: false}}); - await controller.start(); - await flushPromises(); - - MQTT.publish.mockClear(); - controller.state.state = {}; - controller.state.state[device2.ieeeAddr] = {state: 'OFF'}; - const payload = {data: {onOff: 1}, cluster: 'genOnOff', device: device1, endpoint: device1.getEndpoint(1), type: 'attributeReport', linkquality: 10}; - await zigbeeHerdsman.events.message(payload); - await flushPromises(); - - expect(MQTT.publish).toHaveBeenCalledTimes(3); - - expect(MQTT.publish).toHaveBeenCalledWith("zigbee2mqtt/group_1", '{"state":"ON"}', {"retain": false, qos: 0}, expect.any(Function)); - expect(MQTT.publish).toHaveBeenCalledWith("zigbee2mqtt/bulb_color", '{"state":"ON","linkquality":10}', {"retain": false, qos: 0}, expect.any(Function)); - expect(MQTT.publish).toHaveBeenCalledWith("zigbee2mqtt/bulb", '{"state":"ON"}', {"retain": true, qos: 0}, expect.any(Function)); - }); }); diff --git a/test/settings.test.js b/test/settings.test.js index d18e1fa6..e3af2b03 100644 --- a/test/settings.test.js +++ b/test/settings.test.js @@ -203,7 +203,7 @@ describe('Settings', () => { friendlyName: '123', friendly_name: '123', devices: [], - optimistic: 'group', + optimistic: true, }; expect(group).toStrictEqual(expected); @@ -229,7 +229,7 @@ describe('Settings', () => { friendlyName: '123', friendly_name: '123', devices: [], - optimistic: 'group', + optimistic: true, }; expect(group).toStrictEqual(expected); @@ -272,7 +272,7 @@ describe('Settings', () => { const group = settings.getGroup('1'); const expectedGroup = { - optimistic: 'group', + optimistic: true, ID: 1, friendlyName: '123', friendly_name: '123', diff --git a/test/stub/zigbeeHerdsman.js b/test/stub/zigbeeHerdsman.js index 6351fbbd..6e69f05d 100644 --- a/test/stub/zigbeeHerdsman.js +++ b/test/stub/zigbeeHerdsman.js @@ -91,7 +91,7 @@ const returnDevices = []; const devices = { 'coordinator': new Device('Coordinator', '0x00124b00120144ae', 0, 0, [new Endpoint(1, [], [])], false), - 'bulb': new Device('Router', '0x000b57fffec6a5b2', 40369, 4476, [new Endpoint(1, [0,3,4,5,6,8,768,2821,4096], [5,25,32,4096], '0x000b57fffec6a5b2')], true, "Mains (single phase)", "TRADFRI bulb E27 WS opal 980lm"), + 'bulb': new Device('Router', '0x000b57fffec6a5b2', 40369, 4476, [new Endpoint(1, [0,3,4,5,6,8,768,2821,4096], [5,25,32,4096])], true, "Mains (single phase)", "TRADFRI bulb E27 WS opal 980lm"), 'bulb_color': new Device('Router', '0x000b57fffec6a5b3', 40399, 6535, [new Endpoint(1, [0,3,4,5,6,8,768,2821,4096], [5,25,32,4096], '0x000b57fffec6a5b3')], true, "Mains (single phase)", "LLC020"), 'remote': new Device('EndDevice', '0x0017880104e45517', 6535, 4107, [new Endpoint(1, [0], [0,3,4,6,8,5]), new Endpoint(2, [0,1,3,15,64512], [25, 6])], true, "Battery", "RWL021"), 'unsupported': new Device('EndDevice', '0x0017880104e45518', 6536, 0, [new Endpoint(1, [0], [0,3,4,6,8,5])], true, "Battery", "notSupportedModelID"),