Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: dev
Are you sure you want to change the base?
Add remaining 3 phase electrical attributes #339
Changes from all commits
2e2e336
abfd574
a924e3b
30bfcbc
7fab2e3
a0296f5
3dd43d8
0c64f1b
8d50f83
9a737c9
15ff910
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why
power_factor
was here, but I think reporting needs to be configured for it tooThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to ZCL spec, it is not reportable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, apparently we also set up attribute reporting for some attributes that don't apparently don't even support it before this PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed reporting for all the ones that are unsupported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the "good thing" is that we already poll all supported attributes at runtime, so that's why
power_factor
and others work in the first place.But for the ones that change over time, where attribute reporting can't be set up (according to spec), we should just add them to the
ZCL_INIT_ATTRS
dict withcache=True
, as we don't need to poll them when starting up ZHA (theunsupported_attributes
check isn't done there), but we do need to poll them at runtime, which is done through the cluster handler, so there is one big attribute read for multiple attributes.But we should still move some attributes (where attribute reporting isn't possible) from
REPORT_CONFIG
toZCL_INIT_ATTRS
.(EDIT: Wrote this before your comment above 😄)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that we only poll attributes that for which we set up reporting, because we only poll the ones in
REPORT_CONFIG
, so if we move them toZCL_INIT_ATTRS
they will not be polled, right?Also, why do we poll the ones that have reporting set up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. I confused the startup polling doing both and this one only doing the reporting attributes.
We poll the attribute because many smart plugs (mostly Tuya) do not respect/allow setting up attribute reporting on most/all EM cluster attributes.
I do wonder a bit how some of this would work at all then, or at least how it works at the moment (with
power_factor
and so on), but maybe we've just not had any devices that support it? I wanna check how Z2M deals with some of these devices later. It has a polling configuration per plug/device at least.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either no one complained about
power_factor
not getting updated (since there maybe not be many devices with it), or the devices report the value even without reporting config 🤷I reverted the attributes back to REPORT_CONFIG for now.
Unchanged files with check annotations Beta
Check warning on line 587 in zha/zigbee/cluster_handlers/__init__.py
GitHub Actions / shared-ci / Run tests Python 3.12
Check warning on line 587 in zha/zigbee/cluster_handlers/__init__.py
GitHub Actions / shared-ci / Run tests Python 3.12
Check warning on line 587 in zha/zigbee/cluster_handlers/__init__.py
GitHub Actions / shared-ci / Run tests Python 3.13
Check warning on line 587 in zha/zigbee/cluster_handlers/__init__.py
GitHub Actions / shared-ci / Run tests Python 3.13