Add support for rms_current_ph_b/c

[abmantis]

Add support for 3 phase current attributes from the ElectricalMeasurement cluster.
To avoid adding the 2 extra entities to all devices with an ElectricalMeasurement cluster, this only adds them if the attributes have a value. The downside is that this only adds them after a restart/reload. With #311 this could be improved to match on a 3phase feature instead.

Related PR on HA Core: home-assistant/core#132871

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.51%. Comparing base (fd6cf2f) to head (7c3627f).

Additional details and impacted files

@@           Coverage Diff           @@
##              dev     #324   +/-   ##
=======================================
  Coverage   96.50%   96.51%           
=======================================
  Files          61       61           
  Lines        9439     9457   +18     
=======================================
+ Hits         9109     9127   +18     
  Misses        330      330           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[puddly]

Does this one also need _skip_creation_if_none = True?

Just so I understand correctly:

1. For existing devices with these attributes you will have to click the "re-configure" button (and probably reload ZHA) to get them to show up.
2. If you don't do that, there won't be any disabled entities.
3. For new joins, they just work properly?

[abmantis]

Does this one also need _skip_creation_if_none = True?

ElectricalMeasurementRMSCurrentPhC inherits from ElectricalMeasurementRMSCurrentPhB, so no need to repeat the attribute.

Just so I understand correctly:

1. For existing devices with these attributes you will have to click the "re-configure" button (and probably reload ZHA) to get them to show up.

2. If you don't do that, there won't be any disabled entities.

3. For new joins, they just work properly?

It works a bit differently: it adds the entities when the integration gets loaded if there is a value for the attribute already. So, this means that:

1. If the user already had a 3 phase device, after this update the new entities would just show up (because the ph_b and ph_c attributes already have values on the database).
2. For new joins the two new entities will not show until the user reboots HA or reloads ZHA. This is because when the device is added the attributes have no values.

We already used this approach for a few other entities like this .

Ideally, ZHA would add entities as soon as the attribute gets a value, but I don't think we support that ATM.
Also we could use exposes_feaures to add the entities for devices that expose a 3_phase feature, instead of waiting for a value on the attr.

I am planning to eventually work on the exposes_feaures feature, but IMO it is better to have support for the 3ph devices now, even if it is not perfect, than delaying it even more.

[puddly]

Looks good to me. @dmulcahey @TheJulianJES what do you think?

Only nitpick is the inheritance of phase C from phase B. I think it would be clearer to be explicit with _skip_creation_if_none = True.

[abmantis]

Only nitpick is the inheritance of phase C from phase B. I think it would be clearer to be explicit with _skip_creation_if_none = True.

That's what I had initially but changed it to reduce the number of duplicated attributes. I am fine either way so I'll revert back.

[TheJulianJES]

Added some small comments.

I think we can do this generally, with the only real caveat being that we will poll four additional attributes on startup for each device with an EM cluster, even if most devices never support ph_b and ph_c.
.. It should be fine though (for now)? Users will large networks can disable the "initialize attributes" on startup option, but that's also not a great solution.
We might be able to just never poll unsupported attributes on startup again. (If we want to do this, it's for a future PR and I don't consider that to be blocking this.)

[TheJulianJES]

Similar to the comment here, this will also cause four additional attributes to be read at every startup per "smart plug", even if the device doesn't support the attributes.

I guess we can't really avoid this for now and maybe it doesn't really matter overall, but it might be worth looking into only polling attributes if they're not marked as unsupported on startup in the future? Since a couple of versions, the startup attribute polling can at least be disabled completely from the ZHA config UI.
Or expand "quirk id / exposes features" to also cover "rare" devices (even if they conform to ZCL spec) for phase b and c devices?

[abmantis]

Since there 3 phase devices are not super common, maybe the exposes features quirk would be fine for this.
We could also make it so that for devices that do not have the exposes features flag we only setup attr reporting after we get at least one value (similar to what we have with the _skip_creation_if_none). But I am not sure if a device will send anything before attr reporting, so that may just not work at all.

[abmantis]

We might be able to just never poll unsupported attributes on startup again.

You mean not polling if we never got a value after the device joins?

[TheJulianJES]

You mean not polling if we never got a value after the device joins?

If we poll an attribute the first time a device joins, the device can report that attribute as "unsupported" and zigpy will add it to cluster.unsupported_attributes. This is persistent between restarts (saved in zigpy db), so theoretically, we could just adapt this check to only get attributes if they're not in self.cluster.unsupported_attributes here:

zha/zha/zigbee/cluster_handlers/__init__.py

Lines 458 to 460 in fd6cf2f

	cached = [a for a, cached in self.ZCL_INIT_ATTRS.items() if cached]
	uncached = [a for a, cached in self.ZCL_INIT_ATTRS.items() if not cached]
	uncached.extend([cfg["attr"] for cfg in self.REPORT_CONFIG])

[TheJulianJES]

Looks good. Thanks!