From 1f1ce8c6eb0c032a5c4449a4bc8506f17dc60e6b Mon Sep 17 00:00:00 2001 From: Robin Bolscher Date: Thu, 4 Dec 2025 14:10:30 +0100 Subject: [PATCH 1/3] chore: add .prettierrc --- .prettierrc | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .prettierrc diff --git a/.prettierrc b/.prettierrc new file mode 100644 index 0000000..77e6475 --- /dev/null +++ b/.prettierrc @@ -0,0 +1,7 @@ +{ + "semi": true, + "trailingComma": "all", + "singleQuote": true, + "printWidth": 100, + "plugins": ["prettier-plugin-jsdoc"] +} From 0e96b9e69f78ae5e1c75cfc17bb7ff169411eb2c Mon Sep 17 00:00:00 2001 From: Robin Bolscher Date: Thu, 4 Dec 2025 14:18:16 +0100 Subject: [PATCH 2/3] fix: start response timeout only after frame has been sent It started before the frame was actually sent, which in some cases can take some time due to queueing. This would then eat some time form the timeout causing it to timeout too soon. --- README.md | 7 +- lib/Cluster.js | 64 +++++++++++------ test/testClusterResponseTimeout.js | 109 +++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+), 24 deletions(-) create mode 100644 test/testClusterResponseTimeout.js diff --git a/README.md b/README.md index 393823d..ef8f1fb 100644 --- a/README.md +++ b/README.md @@ -87,9 +87,10 @@ class MyDevice extends Homey.Device { // Zigbee specification and refuses to send a default response. waitForResponse: true, - // This is an optional property that allows for adjusting the response - // timeout (25000ms) before the command is considered rejected. - timeout: 10000, + // This is an optional property that allows for adjusting the default response + // timeout (10000ms) before the command is considered rejected. + // The timeout starts after sending the frame and receiving a low-level ack. + timeout: 5000, } ); }); diff --git a/lib/Cluster.js b/lib/Cluster.js index 89936fd..295d426 100644 --- a/lib/Cluster.js +++ b/lib/Cluster.js @@ -956,23 +956,6 @@ class Cluster extends EventEmitter { return this._nextTrxSeqNr; } - async _awaitPacket(trxSequenceNumber, timeout = 25000) { - if (this._trxHandlers[trxSequenceNumber]) { - throw new Error(`already waiting for this trx: ${trxSequenceNumber}`); - } - return new Promise((resolve, reject) => { - const t = setTimeout(() => { - delete this._trxHandlers[trxSequenceNumber]; - reject(new Error('Timeout: Expected Response')); - }, timeout); - this._trxHandlers[trxSequenceNumber] = async frame => { - delete this._trxHandlers[trxSequenceNumber]; - resolve(frame); - clearTimeout(t); - }; - }); - } - // / START STATIC METHODS // Adds command proxy stubs to a proto object which is one level higher. @@ -1106,15 +1089,54 @@ class Cluster extends EventEmitter { return this.sendFrame(payload); } - // Check if a valid timeout override is provided - let responseTimeout; + // Timeout between getting a low-level ack and receiving the ZCL response + let responseTimeout = 10_000; + + // Check if a timeout override is provided if (typeof opts.timeout === 'number') { responseTimeout = opts.timeout; } + if (this._trxHandlers[payload.trxSequenceNumber]) { + throw new Error(`already waiting for this trx: ${payload.trxSequenceNumber}`); + } + + let rejectAwaitResponse; + let resolveAwaitResponse; + let awaitResponseTimeout; const [response] = await Promise.all([ - this._awaitPacket(payload.trxSequenceNumber, responseTimeout), - this.sendFrame(payload), + new Promise((resolve, reject) => { + rejectAwaitResponse = reject; + resolveAwaitResponse = resolve; + + this._trxHandlers[payload.trxSequenceNumber] = async frame => { + delete this._trxHandlers[payload.trxSequenceNumber]; + if (awaitResponseTimeout) clearTimeout(awaitResponseTimeout); + resolveAwaitResponse(frame); + }; + }), + + // Send the frame and when the frame is actually sent start the timeout for receiving + // the response, sending the frame could be queued or delayed for other reasons + // so only start the timeout when it is certain that the frame is sent. + this.sendFrame(payload) + .then(() => { + if (typeof responseTimeout !== 'number') return; + + // Make sure the response is still expected + if (this._trxHandlers[payload.trxSequenceNumber] == null) return; + + awaitResponseTimeout = setTimeout(() => { + delete this._trxHandlers[payload.trxSequenceNumber]; + rejectAwaitResponse(new Error('Timeout: Expected Response')); + }, responseTimeout); + }) + .catch(err => { + delete this._trxHandlers[payload.trxSequenceNumber]; + if (awaitResponseTimeout) clearTimeout(awaitResponseTimeout); + rejectAwaitResponse(err); + throw err; + }), ]); if (response instanceof this.constructor.defaultResponseArgsType) { diff --git a/test/testClusterResponseTimeout.js b/test/testClusterResponseTimeout.js new file mode 100644 index 0000000..bbacde8 --- /dev/null +++ b/test/testClusterResponseTimeout.js @@ -0,0 +1,109 @@ +// eslint-disable-next-line max-classes-per-file,lines-around-directive +'use strict'; + +const assert = require('assert'); +const sinon = require('sinon'); + +const Node = require('../lib/Node'); +const BoundCluster = require('../lib/BoundCluster'); +require('../lib/clusters/onOff'); + +const sandbox = sinon.createSandbox(); + +describe('Cluster Response Timeout', function() { + let receivingNode; + let sendingNode; + + beforeEach(function() { + sendingNode = new Node({ + // Forward frames to receiving node + sendFrame: (...args) => { + receivingNode.handleFrame(...args); + }, + endpointDescriptors: [ + { + endpointId: 1, + inputClusters: [6], + }, + ], + }); + + receivingNode = new Node({ + // Forward frames to sending node + sendFrame: (...args) => { + sendingNode.handleFrame(...args); + }, + endpointDescriptors: [ + { + endpointId: 1, + inputClusters: [6], + }, + ], + }); + }); + + afterEach(function() { + sandbox.restore(); + }); + + it('should only start timeout after frame is sent', async function() { + const originalSendFrame = sendingNode.endpoints[1].sendFrame.bind(sendingNode.endpoints[1]); + sandbox.stub(sendingNode.endpoints[1], 'sendFrame').callsFake(async (...args) => { + // Delay sendFrame by 150ms (longer than timeout) + await new Promise(resolve => setTimeout(resolve, 150)); + return originalSendFrame(...args); + }); + + receivingNode.endpoints[1].bind('onOff', new class extends BoundCluster { + + toggle() { + // Respond immediately + } + + }()); + + // Start the command with 100ms timeout, if timeout starts immediately it will fire before + // sendFrame completes and cause the command to fail + await sendingNode.endpoints[1].clusters.onOff.toggle(undefined, { timeout: 100 }); + }); + + it('should clear timeout when response is received', async function() { + const clearTimeoutSpy = sandbox.spy(global, 'clearTimeout'); + + receivingNode.endpoints[1].bind('onOff', new class extends BoundCluster { + + async toggle() { + // Respond after sendFrame resolved + return new Promise(resolve => setImmediate(resolve)); + } + + }()); + + await sendingNode.endpoints[1].clusters.onOff.toggle(undefined, { timeout: 5000 }); + + // Should have called clearTimeout to prevent timeout firing + assert(clearTimeoutSpy.called, 'clearTimeout should be called when response received'); + }); + + it('should not wait for response when waitForResponse is false', async function() { + const sendingNodeSendFrameSpy = sandbox.spy(sendingNode, 'sendFrame'); + + receivingNode.endpoints[1].bind('onOff', new class extends BoundCluster { + + toggle() { + + // Respond with default response + } + + }()); + + // Send command without waiting for response + const result = await sendingNode.endpoints[1].clusters.onOff.toggle({ waitForResponse: false }); + + // Should return undefined since we're not waiting + assert.strictEqual(result, undefined); + + // Only one frame should be sent (the command itself) + assert.strictEqual(sendingNodeSendFrameSpy.callCount, 1); + }); +}); From 021d73b5d1cadd35caf7478f02a9ed93ae2ffbca Mon Sep 17 00:00:00 2001 From: Robin Bolscher Date: Thu, 4 Dec 2025 14:57:58 +0100 Subject: [PATCH 3/3] chore: add opts.timeout to all Cluster requests --- lib/Cluster.js | 57 +++++++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/lib/Cluster.js b/lib/Cluster.js index 295d426..a3457f5 100644 --- a/lib/Cluster.js +++ b/lib/Cluster.js @@ -340,16 +340,18 @@ class Cluster extends EventEmitter { * discover any manufacture- specific * commands. * - * @param {object} [opts=] - * @param {number} [opts.startValue=0] - * @param {number} [opts.maxResults=250] + * @param {object} [params=] + * @param {number} [params.startValue=0] + * @param {number} [params.maxResults=250] + * @param {object} [opts=] - Optional parameters + * @param {number} [opts.timeout] - Optional timeout in milliseconds for waiting for response * @returns {Promise} */ - async discoverCommandsGenerated({ startValue = 0, maxResults = 250 } = {}) { + async discoverCommandsGenerated({ startValue = 0, maxResults = 250 } = {}, opts) { const { commandIds } = await super.discoverCommandsGenerated({ startValue, maxResults, - }); + }, opts); const res = commandIds.map(cId => ((this.constructor.commandsById[cId] || []) .filter(c => !c.global) @@ -375,16 +377,18 @@ class Cluster extends EventEmitter { * a manufacturer-specific cluster. A manufacturer ID in this field of 0xffff (wildcard) will * discover any manufacture- specific commands. * - * @param {object} [opts=] - * @param {number} [opts.startValue=0] - * @param {number} [opts.maxResults=255] + * @param {object} [params=] + * @param {number} [params.startValue=0] + * @param {number} [params.maxResults=255] + * @param {object} [opts=] - Optional parameters + * @param {number} [opts.timeout] - Optional timeout in milliseconds for waiting for response * @returns {Promise} */ - async discoverCommandsReceived({ startValue = 0, maxResults = 255 } = {}) { + async discoverCommandsReceived({ startValue = 0, maxResults = 255 } = {}, opts) { const { commandIds } = await super.discoverCommandsReceived({ startValue, maxResults, - }); + }, opts); const res = commandIds.map(cId => ((this.constructor.commandsById[cId] || []) .filter(c => !c.global) @@ -400,7 +404,8 @@ class Cluster extends EventEmitter { * Command which reads a given set of attributes from the remote cluster. * Note: do not mix regular and manufacturer specific attributes. * @param {string[]} attributeNames - * @param {{timeout: number}} [opts=] + * @param {object} [opts=] - Optional parameters + * @param {number} [opts.timeout] - Optional timeout in milliseconds for waiting for response * @returns {Promise>} - Object with values (e.g. `{ onOff: true }`) */ async readAttributes(attributeNames, opts) { @@ -453,9 +458,11 @@ class Cluster extends EventEmitter { * Note: do not mix regular and manufacturer specific attributes. * @param {object} attributes - Object with attribute names as keys and their values (e.g. `{ * onOff: true, fakeAttributeName: 10 }`. + * @param {object} [opts=] - Optional parameters + * @param {number} [opts.timeout] - Optional timeout in milliseconds for waiting for response * @returns {Promise<*|{attributes: *}>} */ - async writeAttributes(attributes = {}) { + async writeAttributes(attributes = {}, opts) { const arr = Object.keys(attributes).map(n => { const attr = this.constructor.attributes[n]; if (!attr) { @@ -477,7 +484,7 @@ class Cluster extends EventEmitter { debug(this.logId, 'write attributes', attributes, manufacturerId ? `manufacturer specific id ${manufacturerId}` : ''); - return super.writeAttributes({ attributes: data, manufacturerId }); + return super.writeAttributes({ attributes: data, manufacturerId }, opts); } /** @@ -485,9 +492,11 @@ class Cluster extends EventEmitter { * Note: do not mix regular and manufacturer specific attributes. * @param {object} attributes - Attribute reporting configuration (e.g. `{ onOff: { * minInterval: 0, maxInterval: 300, minChange: 1 } }`) + * @param {object} [opts=] - Optional parameters + * @param {number} [opts.timeout] - Optional timeout in milliseconds for waiting for response * @returns {Promise} */ - async configureReporting(attributes = {}) { + async configureReporting(attributes = {}, opts) { const req = []; // eslint-disable-next-line guard-for-in,no-restricted-syntax for (const attributeName in attributes) { @@ -524,7 +533,7 @@ class Cluster extends EventEmitter { debug(this.logId, 'configure reporting', req, manufacturerId ? `manufacturer specific id ${manufacturerId}` : ''); - const { reports } = await super.configureReporting({ reports: req, manufacturerId }); + const { reports } = await super.configureReporting({ reports: req, manufacturerId }, opts); debug(this.logId, 'configured reporting', reports); for (const result of reports) { @@ -554,10 +563,12 @@ class Cluster extends EventEmitter { * interesting). * Note: do not mix regular and manufacturer specific attributes. * @param {Array} attributes - Array with number/strings (either attribute id, or attribute name). + * @param {object} [opts=] - Optional parameters + * @param {number} [opts.timeout] - Optional timeout in milliseconds for waiting for response * @returns {Promise} - Returns array with * ReadReportingConfiguration objects per attribute. */ - async readReportingConfiguration(attributes = []) { + async readReportingConfiguration(attributes = [], opts) { const req = []; // Loop all the provided attributes @@ -601,7 +612,7 @@ class Cluster extends EventEmitter { const { reports } = await super.readReportingConfiguration({ attributes: req, manufacturerId, - }); + }, opts); debug(this.logId, 'read reporting configuration result', reports); // Return the parsed reports @@ -624,15 +635,17 @@ class Cluster extends EventEmitter { * in a ZigBee cluster or 1 to discover manufacturer specific attributes in either a standard * or a manufacturer specific cluster. * + * @param {object} [opts=] - Optional parameters + * @param {number} [opts.timeout] - Optional timeout in milliseconds for waiting for response * @returns {Promise} - Array with string or number values (depending on if the * attribute * is implemented in zigbee-clusters or not). */ - async discoverAttributes() { + async discoverAttributes(opts) { const { attributes } = await super.discoverAttributes({ startValue: 0, maxResults: 255, - }); + }, opts); const result = []; for (const attr of attributes) { @@ -660,16 +673,18 @@ class Cluster extends EventEmitter { * or a manufacturer- specific cluster. A manufacturer ID in this field of 0xffff (wildcard) * will discover any manufacture-specific attributes. * + * @param {object} [opts=] - Optional parameters + * @param {number} [opts.timeout] - Optional timeout in milliseconds for waiting for response * @returns {Promise} - Returns an array with objects with attribute names as keys and * following object as values: `{name: string, id: number, acl: { readable: boolean, writable: * boolean, reportable: boolean } }`. Note that `name` is optional based on whether the * attribute is implemented in zigbee-clusters. */ - async discoverAttributesExtended() { + async discoverAttributesExtended(opts) { const { attributes } = await super.discoverAttributesExtended({ startValue: 0, maxResults: 250, - }); + }, opts); const result = []; for (const attr of attributes) {