Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add remaining 3 phase electrical attributes #339

Merged
merged 13 commits into from
Jan 23, 2025
Merged

Conversation

abmantis
Copy link
Contributor

@abmantis abmantis commented Dec 24, 2024

Follow up to #324

Related HA PR: home-assistant/core#133969

@TheJulianJES TheJulianJES added the entities Issue or PR about (custom) entities label Jan 11, 2025
Copy link

codecov bot commented Jan 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.52%. Comparing base (5e23a49) to head (28386da).
Report is 3 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #339      +/-   ##
==========================================
+ Coverage   96.50%   96.52%   +0.01%     
==========================================
  Files          61       61              
  Lines        9455     9499      +44     
==========================================
+ Hits         9125     9169      +44     
  Misses        330      330              

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

@abmantis abmantis marked this pull request as ready for review January 15, 2025 18:57
zha/application/platforms/sensor/__init__.py Outdated Show resolved Hide resolved
zha/application/platforms/sensor/__init__.py Outdated Show resolved Hide resolved
zha/application/platforms/sensor/__init__.py Outdated Show resolved Hide resolved
@abmantis abmantis requested a review from TheJulianJES January 17, 2025 23:56
@TheJulianJES
Copy link
Contributor

TheJulianJES commented Jan 18, 2025

Instead of trying to set up attribute reporting for attributes that aren't reportable according to spec, just so the EM cluster will poll them with the current implementation, we could also add another ZCL_POLLING_ATTRS list with just the attributes that should be polled and update async_update in the EM cluster handler.

Then, we'd have all attributes that need to be initialized in ZCL_INIT_ATTRS with True. They will be polled later on anyways (in batches of 5), so no need to force poll them individually at startup.
Only attributes that support attribute reporting would be in REPORT_CONFIG.
And all attributes that aren't reportable but which can still update over time would go in ZCL_POLLING_ATTRS.

async_update should then be updated to poll the attributes from ZCL_POLLING_ATTRS, instead of REPORT_CONFIG.
For clarity, I'd also rather duplicate some of the attribute names between ZCL_POLLING_ATTRS and REPORT_CONFIG (instead of async_update combining them and polling those all).


Another thing, do we really need to poll the _max attributes all the time? Do we even display/use them anywhere?

@abmantis
Copy link
Contributor Author

For clarity, I'd also rather duplicate some of the attribute names between ZCL_POLLING_ATTRS and REPORT_CONFIG (instead of async_update combining them and polling those all).

In that case, won't all the attributes in REPORT_CONFIG also be in ZCL_POLLING_ATTRS then, due to the issue you mention of Tuya devices not reporting them?

Another thing, do we really need to poll the _max attributes all the time? Do we even display/use them anywhere?

To be honest I don't know. Since they were already in REPORT_CONFIG, I assumed they were needed and did the same for phases b/c.

@TheJulianJES
Copy link
Contributor

In that case, won't all the attributes in REPORT_CONFIG also be in ZCL_POLLING_ATTRS then, due to the issue you mention of Tuya devices not reporting them?

Yeah, probably all attributes that are in REPORT_CONFIG should also go in ZCL_POLLING_ATTRS for now.
(But not all attributes that are in ZCL_POLLING_ATTRS should go in REPORT_CONFIG.)
Other than separating it for clarity, I could see that the polling attributes change over time, since we might only need to poll some attributes (could peek into Z2M for that). Currently, we just poll all reportable attrs.

If it looks really stupid to duplicate most/all attributes, we can also combine them in async_update for now. Personally, I'd rather have this be "clear" and easily changeable, but if it's bad/overwhelming, we can do it differently.

abmantis added a commit to abmantis/zha that referenced this pull request Jan 19, 2025
As suggested in zigpy#339 (comment) this adds
an `ZCL_POLLING_ATTRS` to define attributes that should be polled
separatelly from the ones that can have reporting config.

This also moves some attributes that do no support reporting config out
of `REPORT_CONFIG`.
@abmantis
Copy link
Contributor Author

Opened a PR with the suggested changes: #354
I'll mark this one as draft until that gets in.

@abmantis abmantis marked this pull request as ready for review January 23, 2025 11:30
@@ -116,7 +143,6 @@ class MeasurementType(enum.IntFlag):
ElectricalMeasurement.AttributeDefs.measurement_type.name: True,
ElectricalMeasurement.AttributeDefs.power_divisor.name: True,
ElectricalMeasurement.AttributeDefs.power_multiplier.name: True,
ElectricalMeasurement.AttributeDefs.power_factor.name: True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think this is almost good to go now, except for one thing:
All attributes that are exclusively in ZCL_POLLING_ATTRS like power_factor (and I guess also the _max attributes) need to be added to ZCL_INIT_ATTRS too, with cache=True, so they're initialized during pairing. Otherwise, the entities will not be created until ZHA is restarted.

(Attributes that are in ATTR_REPORTING should not be duplicated into ZCL_INIT_ATTRS though.)

There are some other PRs I want to get in first, so I'll look into slightly changing this behavior in the future, but to have less conflicts with the (IKEA) divisor/multiplier reporting PR, let's just add the three power_factor and I guess also the ten _max attributes to ZCL_INIT_ATTRS.

@abmantis abmantis requested a review from TheJulianJES January 23, 2025 22:46
Copy link
Contributor

@TheJulianJES TheJulianJES left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@TheJulianJES TheJulianJES merged commit 7292a0c into zigpy:dev Jan 23, 2025
9 checks passed
@abmantis abmantis deleted the volts_3ph branch January 24, 2025 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
entities Issue or PR about (custom) entities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants