feat: Load additional cluster definitions from options

[Suxsem]

Related to https://github.com/Koenkk/zigbee-herdsman/issues/933

This PR add a new (optional) option to the controller constructor in order to handle additional (manufacturer-specific) cluster definitions.

An additional PR to Z2M will follow to use this new feature in order to load additional cluster definitions from external files.

[Koenkk]

Instead of adding it to the global clusters, I propose to inject them on the device level, such that the cluster stays device specific.

[Suxsem]

I see what you mean, still the clusters are technically manufacturer specific and not device specific. My original idea was to, in Z2M, collect all external converter files, extract all custom clusters and build the additionalClusterDefinitions object to be passed to the ZH controller constructor.

What do you propose to inject them at device level? I found that in ZH devices are created from database, but I don't think that adding the custom cluster definition to the database is a good idea for memory reasons, what do you think?

[Koenkk]

still the clusters are technically manufacturer specific and not device specific

The problem is that different manufacturer define cluster using the same ID, therefore they need to be per device.

but I don't think that adding the custom cluster definition to the database is a good idea for memory reasons, what do you think?

It's indeed not a good idea, so I was thinking they are injected when Z2M starts using the definition onEvent start.

So what needs to happen:

• Add a method to device.ts which allows to add a custom cluster definition (it's saved to a property, not to the database). It should not only be possible to define new clusters, but also add attributes to already existing clusters.
• Add a device parameter to ZclUtils.getCluster

[Suxsem]

@Koenkk Oh I didn't know about the onEvent definition option! I'm going to re-implement the feature as you suggested, give me a couple of days. Thank you!

[Suxsem]

@Koenkk I have implemented your suggestions about device.ts and ZclUtils.getCluster, but I'm having hard time changing calls to ZclUtils.getCluster.

For example, getCluster is called in zclFrame.ts and buffalocl.ts but I don't know how to retrieve a device object (or the device ieee address in order to call the static method Device.byIeeeAddr) in those classes. Any help?

(After implementation is done I will write test cases)

[Nerivec]

Just a note on changing ZCL lookup function signatures:

how to retrieve a device object (or the device ieee address in order to call the static method Device.byIeeeAddr)

zclFrame & buffaloZcl being called at pretty low levels (stack drivers), it may or may not be IEEE that is available (could be network address).

Also, calling Device.by{x} at such low levels, means it may be called multiple times, since ZclDataPayload, passed by drivers, also contains address for device lookup higher up the Z2M chain.

[Koenkk]

I think the tricky ones is where the ZclFrame is created at the driver level, e.g. this call. I was thinking about pushing the ZclFrame creation up to the controller level, so that would be here (we already do the device lookup there). This also allows us to get rid of the nasty try/catch logic (the ember adapter has exactly the same). The adapter will then only emit what is currently the RawDataPayload

What do you think @Nerivec ?

[Suxsem]

It's a little over my head not knowing the project deeply... Sure it's not better to add the specific clusters to the global clusters list instead of keeping them in the device object? The cluster lookup could also be faster by looking at one list only.

[Koenkk]

Sure it's not better to add the specific clusters to the global clusters list instead of keeping them in the device object?

The problem is that there are conflicting cluster, e.g. manufacturer A defines cluster X with ID 20, manufacturer B defines cluster Y with ID 20, how would you handle this?

[Suxsem]

Manufacturer is already matched when searching for correct cluster in zclutils.getCljster, isn't it?

[Koenkk]

@Suxsem not all have the manufacturer code set

[Suxsem]

Oh I see. So I think we should ensure we have at least the address when getCluster is called in order to find the correct device. As I said I don't fully know the project architecture so I'll live you discuss the best way to proceed, please tell me if there is something I can do to help

[Nerivec]

Just to recap a few things here:

• Moving the fromBuffer logic up to the Controller would be good.
  • Would also likely require moving all (or at least some) waitress logic up to the Controller (matching uses fromBuffer-derived properties in many cases). Probably good too, but refactoring of all stacks involved to some degree 😉
• ZCL logic (basically everything under zcl folder) should remain unaware of Device (or anything else really), to keep the spec separate from the Z2M logic.
• Same manufacturer with two different devices can use the same cluster ID with a completely different set of attributes
  • All attributes should probably be considered optional (same device, after firmware update may bring new ones, but older firmware would not have them)?
  • Can't trust that they will uniquely identify (ID) each attribute properly (especially with devices that are made for a specific hub & things like that)...
  • Can't trust that the same attribute ID will use the same data type... (same as above really just from a different point of view)
  • If I'm not mistaken, that's related to https://github.com/Koenkk/zigbee-herdsman/issues/789

[Koenkk]

@Nerivec perfectly summarised, I will start on the fromBuffer refactor

[sjorge]

• Can't trust that the same attribute ID will use the same data type... (same as above really just from a different point of view)

Just for reference and practical examples. https://github.com/Koenkk/zigbee-herdsman/blob/85447fbc839fd5d0b42fc7708c2b61486368b3f8/src/zcl/definition/cluster.ts#L2681 the IKEA STARKVYND uses the same cluster and attribute ID's for their pm25 measurement but use SinglePerc DataType. So we do infact already encounter these. We currently have some hacks in place to 'make this work' https://github.com/Koenkk/zigbee-herdsman-converters/blob/523c1e7147a2178382c9cb883e047bffdd0c2a18/src/devices/ikea.ts#L1184-L1187

But this is far from ideal was you can't simple reconfigure them using the frontend, or without some knowledge via MQTT. We're just lucky that for the payload parsing logic all number variants are treated the same, uint8, uint16, int8, int16, singlePerc... all end up as a typescript number. If this was written in a more strict type language we would probably not be so lucky.

[Suxsem]

Let's close this PR for now waiting for a better strategy

[Suxsem]

Hi @Koenkk, sorry for asking, but did you managed to think about the best way to proceed with this one or already started?

[Koenkk]

@Suxsem yes, its on my todo list

[Suxsem]

Great glad to hear that, happy to help if there is something specific I could help with. Thank you

[Koenkk]

Started working on this in https://github.com/Koenkk/zigbee-herdsman/pull/1011

[Suxsem]

Awesome news, thank you!!