Merge master into production (#minor)

.prettierrc

@@ -0,0 +1,7 @@
+{
+  "semi": true,
+  "trailingComma": "all",
+  "singleQuote": true,
+  "printWidth": 100,
+  "plugins": ["prettier-plugin-jsdoc"]
+}

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,
         }
       );
     });

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<number[]>}
    */
-  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<number[]>}
    */
-  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.<string, unknown>>} - 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,17 +484,19 @@ 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);
   }
 
   /**
    * Command which configures attribute reporting for the given `attributes` on the remote cluster.
    * 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<void>}
    */
-  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<ReadReportingConfiguration[]>} - 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>} - 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<Array>} - 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) {

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);
+  });
+});