feat: better generator support for multi-endpoint devices

[ffenix113]

• Fix pressure extender defaults to match ZCL spec
• Better defaults for reporting on temp, pressure and humidity extenders
• Better definition generator support for devices with multiple endpoints

Previously if multiple endpoints had second endpoint generated definition would still try to fetch attribute value from the first endpoint. Fetching attribute value from developer console worked properly when using correct endpoint.

[Koenkk]

Why is this needed? setupAttributes automatically check the clusters.

[ffenix113]

It is true, but if device has, say, 10 endpoints with temperature cluster - then setupAttributes function currently will iterate all endpoints and for each endpoint with temperature cluster it will try to bind it, setup reporting and possibly read the value of the attribute. As temperature cluster is present in 10 endpoints - setupAttributes will be called 10 times. This results in 10^2 * 3 = 300 operations.

If instead endpoint will be provided - setupAttributes will only do necessary operations for each endpoint and cluster exactly once, reducing operations to 10 * 3 = 30.

Plus by providing endpointID to the extender we can set exposed value to be withEndpoint, defining same exposed attributes separately for each endpoint.

Does this sound reasonable?

[Koenkk]

But why are there 10 endpoints with the temperature cluster on the device?

[ffenix113]

Just an arbitrary example.

Still, it is possible that device could have more than one endpoint with same cluster. And if it would be cluster like temperature or pressure(or any numeric one) - would trigger this behavior.

As another example: device has multiple environmental sensors attached, and those sensors have overlapping and distinct clusters. I.e. all sensors can have temperature cluster, but only one of them will have CO2, another one can have VOC, etc. Think like board can have attached Bosch BME280 & Sensirion SCD4X sensors. They both have temp & humidity clusters, but BME280 additionally has pressure, while SCD4X has CO2.

[Koenkk]

If I understand correctly, for your use-case it's important that the generate definition works without requiring any external converters. How would the generate definition decide for which endpoints to setup temperature reporting for if e.g. 10 of the endpoints have the temperature cluster? It sounds like we are fixing something in z2m here while it should be fixed at the device side instead.

[ffenix113]

How would the generate definition decide for which endpoints to setup temperature reporting for if e.g. 10 of the endpoints have the temperature cluster?

If we provide specific endpoint we can then tailor extender for that specific cluster & endpoint(would it be cluster attribute value or endpoint to bind to).

It sounds like we are fixing something in z2m here while it should be fixed at the device side instead.

Maybe we have a bit of misunderstanding.
For my example I chose temp cluster, because I tested with it, but consider other examples of clusters that also rely on this functionality for generated definition:

• onOff
• light
• lock
• electricityMeter

I am suggesting that device can have multiple endpoints with the same cluster, and we should try to support it by generated definitions as well.
For example devices that have multiple endpoints with same cluster:

• FUGA compatible switch from Smarthjemmet.dk
• Namron 4512768
• Aurora Lighting AU-A1ZBDSS

[Koenkk]

I am suggesting that device can have multiple endpoints with the same cluster, and we should try to support it by generated definitions as well.

Yes that would be nice, but instead of having e.g. temperature(endpointID: 1), temperatature(endpointID: 2), we can do something similar to what we already do for onOff. The only thing that needs to be added multiple times is the expose, to/fromZigbee converters don't need to be duplicated.

[ffenix113]

Ah, I see.
Thank you for explanation, will uldate the implementation then!

[ffenix113]

Sorry, took a bit longer then I expected. Now from and to entities are not duplicated.

[ffenix113]

I would prefer it to have fields like extendFn?: (a: T) => ModernExtend, args?: T, but I am not sure if it is possible to achieve this.

[Koenkk]

Why not?

[ffenix113]

I am not strong at Typescript(that's one reason), but from other languages I know that if I would to add a type parameter here, then it would mean that in all uses of this interface there should be a type parameter specified.

For example, if GeneratedExtend would be GeneratedExtend<T>, then what this should look like:

type ExtendGenerator = (endpoints?: Zh.Endpoint[]) => Promise<GeneratedExtend[]>;

? If to provide single type here then we will not be able to have extender functions that will accept different set of argument types.

Maybe the other way is to provide a class that has two methods like

class Provider {
    getModernExtend(): ModernExtend
    getSource(): string
}

and to have this be implemented by other type, that is generic?

What would you suggest?

[Koenkk]

I tried to do it with the generic but failed to do so. For functions like extenderOnOffLight it's also quite complicated as args can be either OnOffArgs or LightArgs. Therefore the provider class sounds like the best solution

[ffenix113]

Updated. Please check if solution is good.

[ffenix113]

Also, while this MR works properly with reported attributes and fetching attributes from dev console, when you try to fetch the attribute value from the Exposes tab in the device - message No converter available for '<attrbiute>_<endpointID>' ("") is displayed as notification.

I think this is related to zigbee2mqtt repo and how it searches for converters, and not this one.

[kirovilya]

oops. It turns out I also made support for multi endpoints... maybe I shouldn’t have done it if it works here too.
then I suggest you look at my code - maybe you’ll find something useful
#6968

[Koenkk]

Why can't this be a Set?

[ffenix113]

Thank you. Updated.

[Koenkk]

For other modernExtend the endpoints passed is an object, e.g.

zigbee-herdsman-converters/src/devices/atsmart.ts

Line 10 in e4b88b8

extend: [onOff({endpoints: {left: 1, center: 2, right: 3}})],

This allows to specify a friendly name for the endpoint

[ffenix113]

Honestly I think that in this particular example the way that onOff handles some of its argument is "excessive".
I would propose to split it in to more extenders, like

[onOff({endpoints: ["left","center","right"]}), deviceEndpoints({left:1, center:2, right:3}), ota(someDefinition)]

, as I think it should not define which endpoints device has, nor, even more so, define the OTA configuration.

What do you think?

[Koenkk]

But then some check should be in place to check endpoints: ["left","center","right"] against deviceEndpoints({left:1, center:2, right:3}), it creates some unnecessary duplication in my opinion.

Update: I think what you propose is better indeed, but we should do some kind of check. If modernExtend also returns the endpoint names we can do it in index.ts at runtime

[ffenix113]

Updated. I am not sure what specific check to do, so I didn't do any. If you would suggest which checks to do - I can give them a go.

I also didn't update current usages of onOff with endpoints, as it would increase the size of this MR. I can do it in the next MR, if this one will be merged.

[Koenkk]

Sounds good! Already merged this to get it into the 1 Feb release.

[Koenkk]

Maybe for a next pr, can remove the endpoint arg completely? If you want a single endpoint, just pass endpoints with a single element