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"] +} 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..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) { @@ -956,23 +971,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 +1104,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); + }); +});