feat: Allow using custom clusters with groups #1318

ccutrer · 2025-02-12T18:05:41Z

But only if ALL devices in the group support the cluster. It's highly dependent on individual devices if they will support read/write/command for custom clusters. In my testing, my Inovelli switches are responding to writing attributes, but not commands (such as LED commands).

But only if ALL devices in the group support the cluster. It's highly dependendt on individual devices if they will support read/write/command for custom clusters. In my testing, my Inovelli switches are responding to writing attributes, but not commands (such as LED commands).

src/controller/model/group.ts

Nerivec · 2025-02-12T18:29:03Z

src/controller/model/group.ts

+    /**
+     * Get custom clusters that all members share.
+     */
+    get customClusters(): CustomClusters {


I can foresee sync issues with this logic.

Just with members of the group being out of date, or something else? At first I didn't cache this at all, but I didn't want to run this calculation every time a read/write/command is sent. I could go back to that.

Definitely needs some kind of caching due to the amount of calls that could be generated, but it needs to be invalidated by some events. I can think of at least OTA updates (otherwise it breaks without a restart), and re-interview (and re-configure?), basically anything that can change the clusters of a device.

Another thought. We should also be looking out for potential race conditions since this is triggered from various places.

Is there any documentation on the threading model so I can get more familiar with it? If not, that's fine, I'm happy to go code diving and figure it out.

Nerivec · 2025-02-12T18:33:24Z

src/controller/model/group.ts

+
+        const commonClusters: CustomClusters = {};
+        for (const clusterName in customClusters) {
+            if (Array.from(this._members).every((member) => clusterName in member.getDevice().customClusters)) {


Should optimize this for performance. And needs testing with large-ish groups, to see the impact on first exec (could "freeze" for extended periods, worse than it already does in some cases).

What would you recommend? I'm not a JS/TS guy. But I can definitely do performance testing here. I have a group with 85 members in my network that I can send a write to. What do you want me to measure? But not for a custom cluster, though it shouldn't matter, as currently structured custom clusters for the group are calculated regardless of the first write to the group. BUT... I could change that to only calculate the custom clusters for the group if we couldn't find the cluster at first in a read/write/command. That would make this change essentially a no-op for 99.9% of users.

I'm not overly familiar with Z2M group logic, Koenkk should be able to answer that better than me.

I'd first look into optimizing the number of nested loops as much as possible (avoiding the array conversion would be a good start).
No-oping for majority of users is also always preferred when possible.

As for testing, we need to compare what the first execution adds (i.e. when it's first cached) to current time for the same function call, make sure it's not going to create a bottleneck (mostly for low resource scenario).

Koenkk · 2025-02-12T20:28:20Z

src/controller/model/group.ts

+     */
+    get customClusters(): CustomClusters {
+        if (this._customClusters) {
+            return this._customClusters;


This value should be re-calculated once the group members change. Maybe make a private computeCustomClusters which is called when first called and when group members change?

This goes back to what I mentioned here #1318 (comment)

yes, that's exactly what it's already doing, but @Nerivec brought up a good point that the clusters available on device/endpoint could change without group membership changing, so I need to handle several of those cases as well

…sters

Nerivec · 2025-02-16T23:46:20Z

src/controller/model/group.ts

@@ -27,6 +28,7 @@ export class Group extends Entity {
    private databaseID: number;
    public readonly groupID: number;
    private readonly _members: Set<Endpoint>;
+    private _customClusters: CustomClusters | undefined;
    get members(): Endpoint[] {
        return Array.from(this._members).filter((e) => e.getDevice());


@Koenkk remember the reasoning behind this logic? (not related to the PR, but odd)

I think to filter out deleted devices

Nerivec · 2025-02-16T23:47:25Z

src/controller/model/group.ts

@@ -27,6 +28,7 @@ export class Group extends Entity {
    private databaseID: number;
    public readonly groupID: number;
    private readonly _members: Set<Endpoint>;
+    private _customClusters: CustomClusters | undefined;


I think we should name this _commonCustomClusters to avoid confusion (could be all clusters of all devices with current name).

Nerivec · 2025-02-16T23:51:22Z

src/controller/model/group.ts

+    /**
+     * Calculate, store, and return custom clusters that all members share.
+     */
+    private calculateCustomClusters(): CustomClusters {


calculate doesn't seem to quite fit. Maybe identifyCommonCustomClusters?

Nerivec · 2025-02-17T00:09:31Z

src/zspec/zcl/utils.ts

This shouldn't need to be changed.

Also, considering the changes, and the fact tests are still passing, looks like there are gaps in coverage that are not being properly detected (hence, could be misbehaving from previous logic and we wouldn't know).

Nerivec · 2025-02-17T00:14:45Z

src/controller/model/group.ts

@@ -27,6 +28,7 @@ export class Group extends Entity {
    private databaseID: number;
    public readonly groupID: number;
    private readonly _members: Set<Endpoint>;


@Koenkk I think between the logic in members and now the new logic from this PR, it would be more efficient to just use an array here, and have a check in add instead of using a Set that ends up often converted to an array... or we get rid of Array-specific functions, use for (const member of this._members) and keep the Set.

I'm fine with that!

Nerivec reviewed Feb 12, 2025

View reviewed changes

src/controller/model/group.ts Outdated Show resolved Hide resolved

Nerivec reviewed Feb 12, 2025

View reviewed changes

Koenkk reviewed Feb 12, 2025

View reviewed changes

ccutrer added 3 commits February 16, 2025 16:27

use undefined instead of null for not-yet-calculated group custom clu…

7d8cc3c

…sters

hoist array conversion out of loop in Group.customClusters

747bfad

avoid calculating group custom clusters at all if unnecessary

f66199d

ccutrer force-pushed the custom-clusters-to-groups branch from a5c090f to f66199d Compare February 16, 2025 23:43

Nerivec reviewed Feb 16, 2025

View reviewed changes

Nerivec reviewed Feb 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow using custom clusters with groups #1318

feat: Allow using custom clusters with groups #1318

ccutrer commented Feb 12, 2025

Nerivec Feb 12, 2025

ccutrer Feb 12, 2025

Nerivec Feb 12, 2025

Nerivec Feb 13, 2025

ccutrer Feb 13, 2025

Nerivec Feb 12, 2025

ccutrer Feb 12, 2025

Nerivec Feb 12, 2025

Koenkk Feb 12, 2025

Nerivec Feb 12, 2025

ccutrer Feb 12, 2025

Nerivec Feb 16, 2025

Koenkk Feb 17, 2025

Nerivec Feb 16, 2025

Nerivec Feb 16, 2025

Nerivec Feb 17, 2025

Nerivec Feb 17, 2025

Koenkk Feb 17, 2025

feat: Allow using custom clusters with groups #1318

Are you sure you want to change the base?

feat: Allow using custom clusters with groups #1318

Conversation

ccutrer commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment