avoid calculating group custom clusters at all if unnecessary

ccutrer · Nerivec · commit ebb4385abf17 · 2025-03-02T17:41:32.000+01:00
diff --git a/src/controller/model/group.ts b/src/controller/model/group.ts
@@ -2,7 +2,7 @@ import assert from 'node:assert';
 
 import {logger} from '../../utils/logger';
 import * as Zcl from '../../zspec/zcl';
-import {CustomClusters} from '../../zspec/zcl/definition/tstype';
+import {Cluster, CustomClusters} from '../../zspec/zcl/definition/tstype';
 import ZclTransactionSequenceNumber from '../helpers/zclTransactionSequenceNumber';
 import {DatabaseEntry, KeyValue} from '../tstype';
 import Device from './device';
@@ -186,7 +186,7 @@ export class Group extends Entity {
 
     public async write(clusterKey: number | string, attributes: KeyValue, options?: Options): Promise<void> {
         const optionsWithDefaults = this.getOptionsWithDefaults(options, Zcl.Direction.CLIENT_TO_SERVER);
-        const cluster = Zcl.Utils.getCluster(clusterKey, undefined, this.customClusters);
+        const cluster = this.getCluster(clusterKey);
         const payload: {attrId: number; dataType: number; attrData: number | string | boolean}[] = [];
 
         for (const [nameOrID, value] of Object.entries(attributes)) {
@@ -214,7 +214,7 @@ export class Group extends Entity {
                 'write',
                 cluster.ID,
                 payload,
-                this.customClusters,
+                this._customClusters ?? {},
                 optionsWithDefaults.reservedBits,
             );
 
@@ -230,7 +230,7 @@ export class Group extends Entity {
 
     public async read(clusterKey: number | string, attributes: (string | number)[], options?: Options): Promise<void> {
         const optionsWithDefaults = this.getOptionsWithDefaults(options, Zcl.Direction.CLIENT_TO_SERVER);
-        const cluster = Zcl.Utils.getCluster(clusterKey, undefined, this.customClusters);
+        const cluster = this.getCluster(clusterKey);
         const payload: {attrId: number}[] = [];
 
         for (const attribute of attributes) {
@@ -246,7 +246,7 @@ export class Group extends Entity {
             'read',
             cluster.ID,
             payload,
-            this.customClusters,
+            this._customClusters ?? {},
             optionsWithDefaults.reservedBits,
         );
 
@@ -267,7 +267,7 @@ export class Group extends Entity {
 
     public async command(clusterKey: number | string, commandKey: number | string, payload: KeyValue, options?: Options): Promise<void> {
         const optionsWithDefaults = this.getOptionsWithDefaults(options, Zcl.Direction.CLIENT_TO_SERVER);
-        const cluster = Zcl.Utils.getCluster(clusterKey, undefined, this.customClusters);
+        const cluster = this.getCluster(clusterKey);
         const command = cluster.getCommand(commandKey);
 
         const createLogMessage = (): string => `Command ${this.groupID} ${cluster.name}.${command.name}(${JSON.stringify(payload)})`;
@@ -283,7 +283,7 @@ export class Group extends Entity {
                 command.ID,
                 cluster.ID,
                 payload,
-                this.customClusters,
+                this._customClusters ?? {},
                 optionsWithDefaults.reservedBits,
             );
 
@@ -309,15 +309,11 @@ export class Group extends Entity {
     }
 
     /**
-     * Get custom clusters that all members share.
+     * Calculate, store, and return custom clusters that all members share.
      */
-    get customClusters(): CustomClusters {
-        if (this._customClusters) {
-            return this._customClusters;
-        }
-
+    private calculateCustomClusters(): CustomClusters {
         if (this._members.size === 0) {
-            return {};
+            return (this._customClusters = {});
         }
 
         const membersArray = Array.from(this._members);
@@ -331,8 +327,24 @@ export class Group extends Entity {
             }
         }
 
-        this._customClusters = commonClusters;
-        return this._customClusters;
+        return (this._customClusters = commonClusters);
+    }
+
+    private getCluster(key: string | number): Cluster {
+        if (this._customClusters) {
+            return Zcl.Utils.getCluster(key, undefined, this._customClusters);
+        }
+
+        // At first, don't fully calculate custom clusters
+        const cluster = Zcl.Utils.findCluster(key, undefined, {});
+
+        // If no cluster was found, and we haven't calculated custom clusters,
+        // do so now, and then retry
+        if (!cluster) {
+            return Zcl.Utils.getCluster(key, undefined, this.calculateCustomClusters());
+        }
+
+        return cluster;
     }
 }
 
diff --git a/src/zspec/zcl/utils.ts b/src/zspec/zcl/utils.ts
@@ -120,7 +120,7 @@ function getClusterDefinition(
     key: string | number,
     manufacturerCode: number | undefined,
     customClusters: CustomClusters,
-): {name: string; cluster: ClusterDefinition} {
+): {name: string; cluster: ClusterDefinition} | undefined {
     let name: string | undefined;
 
     if (typeof key === 'number') {
@@ -144,25 +144,23 @@ function getClusterDefinition(
         name = key;
     }
 
-    let cluster =
-        name !== undefined && hasCustomClusters(customClusters)
+    const hasCustomClustersResult = hasCustomClusters(customClusters);
+    const cluster =
+        name !== undefined && hasCustomClustersResult
             ? {
                   ...Clusters[name as ClusterName],
                   ...customClusters[name], // should override Zcl clusters
               }
             : Clusters[name as ClusterName];
 
-    if (!cluster) {
-        if (typeof key === 'number') {
-            name = key.toString();
-            cluster = {attributes: {}, commands: {}, commandsResponse: {}, manufacturerCode: undefined, ID: key};
-        } else {
-            name = undefined;
-        }
+    if (!name || !cluster) {
+        return undefined;
     }
-
-    if (!name) {
-        throw new Error(`Cluster with name '${key}' does not exist`);
+    // If we have customClusters, we have to double check that we didn't end up with
+    // an empty object due to the use of the spread operator above
+    if (hasCustomClustersResult) {
+        for (const k in cluster) return {name, cluster};
+        return undefined;
     }
 
     return {name, cluster};
@@ -281,7 +279,28 @@ function createCluster(name: string, cluster: ClusterDefinition, manufacturerCod
 }
 
 export function getCluster(key: string | number, manufacturerCode: number | undefined, customClusters: CustomClusters): Cluster {
-    const {name, cluster} = getClusterDefinition(key, manufacturerCode, customClusters);
+    let nameAndCluster = getClusterDefinition(key, manufacturerCode, customClusters);
+
+    if (!nameAndCluster) {
+        if (typeof key === 'number') {
+            const name = key.toString();
+            const cluster = {attributes: {}, commands: {}, commandsResponse: {}, manufacturerCode: undefined, ID: key};
+            nameAndCluster = {name, cluster};
+        } else {
+            throw new Error(`Cluster with name '${key}' does not exist`);
+        }
+    }
+
+    const {name, cluster} = nameAndCluster;
+    return createCluster(name, cluster, manufacturerCode);
+}
+
+export function findCluster(key: string | number, manufacturerCode: number | undefined, customClusters: CustomClusters): Cluster | undefined {
+    const nameAndCluster = getClusterDefinition(key, manufacturerCode, customClusters);
+    if (!nameAndCluster) {
+        return undefined;
+    }
+    const {name, cluster} = nameAndCluster;
     return createCluster(name, cluster, manufacturerCode);
 }
 
diff --git a/test/controller.test.ts b/test/controller.test.ts
@@ -5715,6 +5715,60 @@ describe('Controller', () => {
             ),
         );
         expect(mocksendZclFrameToGroup.mock.calls[0][2]).toBeUndefined();
+        // Do another write, to ensure customClusters was cached
+        await group.write('myCustomCluster', {superAttribute: 3}, {});
+        expect(mocksendZclFrameToGroup).toHaveBeenCalledTimes(2);
+        expect(mocksendZclFrameToGroup.mock.calls[0][0]).toBe(2);
+        expect(deepClone(mocksendZclFrameToGroup.mock.calls[1][1])).toStrictEqual(
+            deepClone(
+                Zcl.Frame.create(
+                    Zcl.FrameType.GLOBAL,
+                    Zcl.Direction.CLIENT_TO_SERVER,
+                    true,
+                    undefined,
+                    12,
+                    'write',
+                    9123,
+                    [{attrData: 3, attrId: 0, dataType: 32}],
+                    device.customClusters,
+                ),
+            ),
+        );
+        expect(mocksendZclFrameToGroup.mock.calls[1][2]).toBeUndefined();
+    });
+
+    it('Write to empty group with custom cluster should fail', async () => {
+        await controller.start();
+        const group = await controller.createGroup(2);
+        let error;
+        try {
+            await group.write('myCustomCluster', {superAttribute: 5}, {});
+        } catch (e) {
+            error = e;
+        }
+        expect(error).toStrictEqual(new Error(`Cluster with name 'myCustomCluster' does not exist`));
+    });
+
+    it('Write to group with unsupported custom cluster should fail', async () => {
+        await controller.start();
+
+        await mockAdapterEvents['deviceJoined']({networkAddress: 129, ieeeAddr: '0x129'});
+        const device = controller.getDeviceByIeeeAddr('0x129')!;
+        device.addCustomCluster('myCustomCluster', {
+            ID: 9123,
+            commands: {},
+            commandsResponse: {},
+            attributes: {superAttribute: {ID: 0, type: Zcl.DataType.UINT8}},
+        });
+        const group = await controller.createGroup(2);
+        group.addMember(device.getEndpoint(1)!);
+        let error;
+        try {
+            await group.write('otherCustomCluster', {superAttribute: 5}, {});
+        } catch (e) {
+            error = e;
+        }
+        expect(error).toStrictEqual(new Error(`Cluster with name 'otherCustomCluster' does not exist`));
     });
 
     it('Write to group with unknown attribute should fail', async () => {
