Set up reporting on AC attribute divisor and multiplier

[puddly]

Should resolve zigpy/zha-device-handlers#3374

I'll test it out with a real plug when I get one.

[puddly]

I put these at the very beginning just in case the device's attribute reports are sent in the order of the reporting config. If this is the case, you may need to re-join the device to get this to work.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.52%. Comparing base (7292a0c) to head (4757673).
Report is 4 commits behind head on dev.

Additional details and impacted files

@@           Coverage Diff           @@
##              dev     #348   +/-   ##
=======================================
  Coverage   96.52%   96.52%           
=======================================
  Files          61       61           
  Lines        9499     9506    +7     
=======================================
+ Hits         9169     9176    +7     
  Misses        330      330           

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

[abmantis]

Should also resolve: home-assistant/core#129723

[puddly]

I unfortunately haven't had time to drive and pick one of these plugs up yet so if anyone has one to test, let me know if this PR is enough.

[folfy]

I unfortunately haven't had time to drive and pick one of these plugs up yet so if anyone has one to test, let me know if this PR is enough.

Oh sure, I'll try to merge it into my production environment and test it during this week still! Thanks for the potential fix :)

[folfy]

@puddly:
I installed your branch on my HA testing instance, and I can confirm that the fix is actually working. There ist still sometimes a brief moment (<1s), where for example I change the load from 0 -> 2kW, and ~200W is shown temporarily, but I think that's up to the sensors reporting behaviour essentially.

You set the divisor / multiplier attribute reporting to "immediate" -> Wouldn't it maybe be better/more consistent, to set the same value as for the attribute itself, i.e. REPORT_CONFIG_OP? I mean just a bit worried we might flood the network a lot, if a sensor would toggle a lot between x10 and x1 for example, and the attribute/value isn't might not even gonna update as fast.

Important note for users: They have to reconfigure the plugs ZHA entity, in order to update the reporting configuration (otherwise the fix won't help).

[puddly]

Wouldn't it maybe be better/more consistent, to set the same value as for the attribute itself

Reporting is based on a "minimum change", subject to a minimum and maximum reporting interval (i.e. don't report more often than once every "min" seconds and report at least once every "max" seconds). Since the reporting config is based on the value of the actual attribute, using the same config for multiple attributes won't really synchronize things.

If a device is somehow jittering between 999W and 1001W, I don't think there's a way around it: the divisor and multiplier attributes must be accurate for the power reading to be in any way useful. If they ever are not perfectly synchronized, the power reading will be off by a factor of 10.

There ist still sometimes a brief moment (<1s), where for example I change the load from 0 -> 2kW, and ~200W is shown temporarily, but I think that's up to the sensors reporting behaviour essentially.

Could you record a debug log of the attribute reports ZHA receives when this happens? Maybe the order in which the attribute reports are received is causing issues. Or we aren't reacting properly to them all coming in at once.

[TheJulianJES]

All attributes in REPORT_CONFIG also get polled by the EM cluster in batches of 5 per read. So, the order matters somewhat for that.
I don't think it should affect this, but still something to keep in mind.

Like async_update in the cluster checks for unsupported_attributes, so it's possible that the divider and power attribute are read in different batches and/or updated in the wrong order.

[folfy]

@TheJulianJES That's actually rly interesting. But then if they're changing frequently (like changing load every few seconds, right after it updated in HA), it's a just a matter of push-updates, I mean they shouldn't be polled in this case, right?

[TheJulianJES]

ZHA currently still polls them regardless (every 30 to 45 seconds), as they match the PolledElectricalMeasurement sensor class. Polling should be disabled by adding the model name to models of the ElectricalMeasurement sensor class.

The reason ZHA polls by default is that quite a few smart plugs don't actually support attribute reporting (Tuya included, of course). Theoretically, we could make polling opt-in or opt-out via a quirk_id/exposes_features in quirks. That might be worth looking into. Peeking into Z2M should tell us which devices actually need to be polled.

But I'm not saying that the issue you're seeing is definitely related to the polling behavior.

[TheJulianJES]

I've rebased the PR for now, as it was conflicting quite a bit.

With #354, we've also separated the attributes that we're polling for the EM cluster from the REPORT_CONFIG.
Previously, REPORT_CONFIG also contained the _max attributes which do not support attribute reporting. Those attribute values are only included in the extra state attributes for HA entities. IMO, we should not really poll them all the time, nor add those values to the extra states attributes at all. I doubt anyone really uses them and most devices do not seem to support them?

To keep the same behavior in this PR from before the rebase, I've also added all multipliers and divisors to ZCL_POLLING_ATTRS, so they're polled constantly. Tbh, I'm not sure if we even need this behavior.
As these are already included in REPORT_CONFIG (which acts like they're in ZCL_INIT_ATTRS with cache=False), they should be read on every network startup. Otherwise, the attribute reports should arrive. As these only really change at runtime from IKEA plugs, we might be able to drop this commit: c6da8c6.

---

Also to keep in mind that attributes in REPORT_CONFIG/ZCL_INIT_ATTRS are read on startup, even if the attributes were already marked as unsupported. We might want to expand that behavior to not reinitialize/read attributes on startup if they're already in unsupported_attributes.

And in a future PR, we might want to include power_factor into polling. It's not reportable and even before the EM related PRs, we never polled it (or tried to set up attribute reporting for it).

[abmantis]

And in a future PR, we might want to include power_factor into polling. It's not reportable and even before the EM related PRs, we never polled it (or tried to set up attribute reporting for it).

I was adding that in #339 since it adds phases b/c of the power factor too.

[TheJulianJES]

Ok, let's merge #339 first then (if the last changes are made).
If this conflicts again (likely), I'll rebase this again.

Looking at Z2M, I also wanna disable polling completely for this device, as it seems to fully support attribute reports. We'll still poll the attributes once when HA/ZHA is restarted. To disable polling, it's enough to just add the model here:

zha/zha/application/platforms/sensor/__init__.py

Line 619 in 3cab6ab

models={"VZM31-SN", "SP 234", "outletv4"},

That might reduce/eliminate the jumping between old/new divisor for the power, assuming the device sends the attribute reports in the correct order. With our polling implementation, I guess the order can't be guaranteed.

In the future, instead of adding all models in ZHA, we can use quirk_id/exposes_features to either have EM polling opt-out via quirks or completely opt-in (mostly for Tuya plugs).

[folfy]

@TheJulianJES I can test it and make the logs for @puddly - To drop polling I can just revert commit c6da8c6 ?

[TheJulianJES]

That will disable polling for those specific attributes only, but for all devices, so if you have some Tuya plugs, they might not work after doing that.
If you want to test it this way, you can remove all attributes (including active_power) from ZCL_POLLING_ATTRS. That should completely disable EM polling and only rely on attribute reports, like Z2M does (for this plug).

What I'd recommend is just adding "INSPELNING Smart plug" here:

zha/zha/application/platforms/sensor/__init__.py

Line 619 in 3cab6ab

models={"VZM31-SN", "SP 234", "outletv4"},

That should disable polling completely, only for the IKEA plug.

[TheJulianJES]

Rebased again. Also completely disabled polling for the IKEA plug now, as it supports attribute reporting (1d4e423).

And also removed polling for the multipliers/divisors for all plugs (4757673).
This is a behavioral change from the initial PR (as all attributes in REPORT_CONFIG were always polled then), but it's the right change, as plugs that change these attributes at runtime should report them and not rely on these attributes being polled constantly. This also reduces network traffic when you have lots of plugs that are polled.

---

One downside of this PR is still that attributes in REPORT_CONFIG are always polled once on startup (acts like they're in ZCL_INIT_ATTRS with cache=False), but this is something to look at in another PR, as the same issue also exists for the PhB and PhC entities that very few devices actually support.

For now, I think this should be good to go.