Koenkk / zigbee-herdsman

A Node.js Zigbee library
MIT License
454 stars 278 forks source link

[Fix for #978] Cluster defined with a key are not found anymore #999

Closed LaurentChardin closed 1 month ago

LaurentChardin commented 1 month ago

After #978, the Zcl.ZclFrame.create is using cluster.name instead of cluster.ID.

However, the cluster key is actually expected from the method signature.

This has as the following consequence: consecutive calls to getCluster will change the key from a number to a string, and failing on line 61 of utils.ts, while the key was still valid.

This is needed when testing clusters that are not defined (yet) in cluster.ts.

Fix: use cluster.ID instead of cluster.name in order to support custom clusters.

LaurentChardin commented 1 month ago

So i see the test failing here.

To put some context, i was trying this change because of the extend in my device definition:

            binary({
                name: 'eOnTimer',
                cluster: 64518,
                attribute: {ID: 0x0003, type: 0x10},
                description: 'Enable (0x01) / Disable (0x00) use of onTimer.',
                valueOn: ['ON', 0x01],
                valueOff: ['OFF', 0x00],
                entityCategory: 'config'
            }),

Cluster 64518 is not defined in cluster.ts but zclCommand was failing because 64518 becomes '64518', due to chain called to utils.getCluster. By using cluster.id instead of cluster.name in the Zcl.ZclFrame.create call, the call is executed properly.

But it seems the use of cluster.name was intentional in order to detect Cluster ID collision by forcing a lookup in the main referential of cluster.ts, assuming cluster.name is unique and cluster.ID is not.

Cycling back on the issue on how to use and configure Clusters without touching the lib. I suspect some device definition will fail with the latest PR, as i have found this pattern in other devices. For exemple the SonOff definitions is using this pattern to use device clusters not present in Cluster.ts:

For example, line 737, devices/sonoff.ts:

            binary({
                name: 'child_lock',
                cluster: 0xFC11,
                attribute: {ID: 0x0000, type: 0x10},
                description: 'Enables/disables physical input on the device',
                valueOn: ['LOCK', 0x01],
                valueOff: ['UNLOCK', 0x00],
            }),

i suspect new error messages will happen as well for those brands as well.

@Koenkk any thoughts ?

LaurentChardin commented 1 month ago

Ok found it back to https://github.com/Koenkk/zigbee-herdsman/issues/686

I understand the context, but then it kinda breaks the on-the-fly cluster definition.

I will just go back and adjust cluster.ts for the moment then. And i suspect other examples i have seen in ZHC will have to be changed as well.