Koenkk / zigbee2mqtt

Zigbee 🐝 to MQTT bridge 🌉, get rid of your proprietary Zigbee bridges 🔨
https://www.zigbee2mqtt.io
GNU General Public License v3.0
11.72k stars 1.64k forks source link

[Feature request]: Improve Home Assistant integration by reusing other entity behavior #22098

Open schauveau opened 5 months ago

schauveau commented 5 months ago

Is your feature request related to a problem? Please describe

I have multiple plugs with energy monitoring and also a PJ-1203A (Bidirectional energy meter with 80A current clamp). The former are working fine in Home Assistant but for the later, I noticed that I cannot display statistic graphs for the power_a, power_b and power_ab entities. I eventually figured out that this is because those entities are missing in the Home Assistant integration.

https://github.com/Koenkk/zigbee2mqtt/blob/ffc2ff1d0ea8173776d9bb7491f8435b6050892b/lib/extension/homeassistant.ts#L803

Adding the missing entities in the lookup table works fine (until I update zigbee2mqtt) but there are other devices that use different names for similar values (e.g. power_l1,power_phase_a, load, ... ) so a generic solution could be welcome. The annoying thing is that there is no way to control the HA integration from the converter itself.

I understand that Home Assistant features should not be exposed in zigbee-herdsman-converters but I believe that there is an easy solution.

Describe the solution you'd like

The integration in homeassistant.ts is making extensive use of lookup tables using the entity name as key. This is not a good approach because a lot of converters (including external ones) are using names that are not listed there.

My proposal is to introduce a new field behavior and an associated member in all Expose objects:

     withBehavior(behavior: string) {
        this.behavior = behavior
     }

In homeassistant.ts , a few occurrences of lookup[firstExpose.name ] must be transformed into lookup[firstExpose.behavior || firstExpose.name ].

The idea is that the keys in homeassistant.ts are now treated as behaviors and the default behavior is the name of the entity. The changes are very small and should not break any existing converter.

Any entity or predefined expose helper can then declared to follow a known behavior foobar using .withBehavior('foobar')

It could be worth spending a few minutes or hours to clean up the lookup tables since some entries are now redundant (e.g. add .withBehavior('voltage') in voltageWithPhase() and then remove voltage_phase_b and voltage_phase_c from homeassistant.ts ). As a second thought, obsolete behaviors should probably be kept to avoid breaking external converters that may still be using them (emit a warning?).

Describe alternatives you've considered

The HA integration could be tuned using the unit (as for Wh and kWh) but this is not very flexible.

Additional context

none

Koenkk commented 4 months ago

Can't we solve this by stripping the endpoint from the name? E.g. if you would have name == power_l1 and endpoint == l1, stripping the endpoint would give you power.

schauveau commented 4 months ago

Yes and No. That could work when the suffix comes from the endpoint but this is not always the case. The PJ-12304 is using Tuya datapoints so there is no endpoint.

The proposed feature is more flexible especially ifbehavior was initialized with the initial name in the Expose objhect constructor. Something like new Numeric('power', access.STATE). withEndpoint('l1') would automatically set the behavior to power.

Similarly, a new method Base.withSuffix could be added to append a suffix to the name.

    withSuffix(suffix: string) {
       this.name = this.name+"_"+suffix;
    } 

For example voltage_phase_b is currently implemented with

     voltage_phase_b: () => new Numeric('voltage_phase_b', access.STATE).withLabel('Voltage phase B').withUnit('V').withDescription('Measured electrical potential value on phase B'),

It could become either

    voltage_phase_b: () => new Numeric('voltage', access.STATE)..withSuffix('b').withLabel('Voltage phase B').withUnit('V').withDescription('Measured electrical potential value on phase B'),

or

    voltage_phase_b: () => new Numeric('voltage_phase_b', access.STATE).withBehavior('voltage').withLabel('Voltage phase B').withUnit('V').withDescription('Measured electrical potential value on phase B'),

Both would result in name=voltage_phase_b and behavior=voltage.

Koenkk commented 4 months ago

The PJ-12304 is using Tuya datapoints so there is no endpoint.

I haven't tried, but I think it should give no problems when using the withEndpoint() here. I'm a bit hesitant to add all kinds of attributes to exposes which are just for HA

schauveau commented 4 months ago

Do not see the behavior as something specific to HA. This is more a kind of hint about the real nature of the attribute.

Relying on the attribute name is really not nice since that requires that all possible names should be hard-coded in homeassistant.ts.

Consider for instance the recent TIC device: https://github.com/Koenkk/zigbee-herdsman-converters/blob/cd1638889237d50de2f62e1fed490d22294e4d15/src/devices/gmmts.ts#L219 . Do you really want to add those 104 new attributes to homeassistant.ts? It is far easier to add a behavior column in the table and then do a .withBehavior(...) in the converter code.

Koenkk commented 4 months ago

Do you really want to add those 104 new attributes to homeassistant.ts?

Definitely not! But how would you see this? There is a specific set of withBehavior values that can be set? So e.g. withBehavior('temperature') would replace temperature and local_temperature. I think we still need a lot of hardcoded values in HA + refactor all current exposes to use withBehavior()