diff --git a/lib/Cluster.js b/lib/Cluster.js index d59953f..348028f 100644 --- a/lib/Cluster.js +++ b/lib/Cluster.js @@ -494,17 +494,25 @@ class Cluster extends EventEmitter { debug(this.logId, 'read attributes result', response); const { attributeArrayStatusDataType } = this.constructor; const result = attributeArrayStatusDataType.fromBuffer(response.attributes, 0); - if (!result.length) break; - - result.forEach((a) => { - attrIds.delete(a.id); - if (a.status === 'SUCCESS') { - resultObj[idToName[a.id]] = a.value; - if (numericIds.has(a.id) && idToName[a.id] !== a.id) { - resultObj[a.id] = a.value; + + const remaining = attrIds.size; + result.forEach((record) => { + // Devices do answer with attributes that were not asked for. Dropping those keeps them + // out of the result, where they would land under an undefined key. + if (!attrIds.delete(record.id)) return; + if (record.status === 'SUCCESS') { + resultObj[idToName[record.id]] = record.value; + if (numericIds.has(record.id) && idToName[record.id] !== record.id) { + resultObj[record.id] = record.value; } } }); + + // Asking again would repeat the request that just made no progress. + if (attrIds.size === remaining) { + debug(this.logId, 'read attributes gave up, unanswered:', [...attrIds]); + break; + } } return resultObj; diff --git a/test/readAttributes.js b/test/readAttributes.js new file mode 100644 index 0000000..24a6728 --- /dev/null +++ b/test/readAttributes.js @@ -0,0 +1,67 @@ +'use strict'; + +const { describe, it } = require('node:test'); +const assert = require('node:assert'); + +const BoundCluster = require('../lib/BoundCluster'); +const OnOffCluster = require('../lib/clusters/onOff'); +const { createSingleClusterDevice } = require('./util/virtualDevice'); + +const ON_OFF = OnOffCluster.ATTRIBUTES.onOff.id; +const ON_TIME = OnOffCluster.ATTRIBUTES.onTime.id; + +/** A device whose onOff cluster answers every read with the records `answer(requestedIds)`. */ +function deviceAnswering(answer) { + return createSingleClusterDevice( + OnOffCluster, + new (class extends BoundCluster { + async readAttributes({ attributes }) { + const buf = Buffer.alloc(255); + const len = OnOffCluster.attributeArrayStatusDataType.toBuffer(buf, answer(attributes), 0); + return { attributes: buf.subarray(0, len) }; + } + })(), + ); +} + +// The device that only ever answers with unrequested ids is in test/virtualDevice.js. +describe('Cluster readAttributes', function () { + it('reads what it asked for across several responses', async function () { + const requests = []; + const { zclNode } = deviceAnswering((ids) => { + requests.push([...ids]); + // One attribute per response, as a device splitting its answer over several frames does. + const id = ids[0]; + return [{ id, status: 'SUCCESS', value: id === ON_OFF ? true : 5 }]; + }); + + const result = await zclNode.endpoints[1].clusters.onOff.readAttributes(['onOff', 'onTime']); + + assert.deepStrictEqual(requests, [[ON_OFF, ON_TIME], [ON_TIME]]); + assert.deepStrictEqual(result, { onOff: true, onTime: 5 }); + }); + + it('keeps attributes that were not asked for out of the result', async function () { + const { zclNode } = deviceAnswering(() => [ + { id: ON_OFF, status: 'SUCCESS', value: true }, + { id: ON_TIME, status: 'SUCCESS', value: 5 }, + ]); + + const result = await zclNode.endpoints[1].clusters.onOff.readAttributes(['onOff']); + + assert.deepStrictEqual(result, { onOff: true }); + }); + + it('stops on a response that holds no attributes at all', async function () { + let reads = 0; + const { zclNode } = deviceAnswering(() => { + reads++; + return []; + }); + + const result = await zclNode.endpoints[1].clusters.onOff.readAttributes(['onOff']); + + assert.strictEqual(reads, 1); + assert.deepStrictEqual(result, {}); + }); +}); diff --git a/test/virtualDevice.js b/test/virtualDevice.js index e9f19f8..436e68e 100644 --- a/test/virtualDevice.js +++ b/test/virtualDevice.js @@ -97,34 +97,30 @@ describe('Virtual device', function () { }); describe('Virtual device: known bugs on master', function () { - it( - 'readAttributes stops when the device answers with unrequested ids', - { todo: 'fixed by #258' }, - async function () { - // Whatever is asked, the plug answers with onTime only - const { zclNode, intercept } = createSingleClusterDevice( - OnOffCluster, - new (class extends BoundCluster { - async readAttributes() { - const buf = Buffer.alloc(255); - const records = [{ id: ON_TIME, status: 'SUCCESS', value: 5 }]; - const len = OnOffCluster.attributeArrayStatusDataType.toBuffer(buf, records, 0); - return { attributes: buf.subarray(0, len) }; - } - })(), - ); - let reads = 0; - intercept((frame) => { - // Guard: on master the read loops forever. - if (frame.from === 'zclNode' && ++reads > 3) throw new Error('read loop'); - }); + it('readAttributes stops when the device answers with unrequested ids', async function () { + // Whatever is asked, the plug answers with onTime only + const { zclNode, intercept } = createSingleClusterDevice( + OnOffCluster, + new (class extends BoundCluster { + async readAttributes() { + const buf = Buffer.alloc(255); + const records = [{ id: ON_TIME, status: 'SUCCESS', value: 5 }]; + const len = OnOffCluster.attributeArrayStatusDataType.toBuffer(buf, records, 0); + return { attributes: buf.subarray(0, len) }; + } + })(), + ); + let reads = 0; + intercept((frame) => { + // Guard: on master the read loops forever. + if (frame.from === 'zclNode' && ++reads > 3) throw new Error('read loop'); + }); - const result = await zclNode.endpoints[1].clusters.onOff.readAttributes(['onOff']); + const result = await zclNode.endpoints[1].clusters.onOff.readAttributes(['onOff']); - assert.strictEqual(reads, 1); - assert.deepStrictEqual(result, {}); - }, - ); + assert.strictEqual(reads, 1); + assert.deepStrictEqual(result, {}); + }); it( 'readAttributes([]) on metering splits the request to fit a frame',