Add support for ZHA entities exposed by Zigpy quirks #111176

dmulcahey · 2024-02-23T01:31:32Z

Proposed change

This PR adds support for entities exposed by the next iteration of Zigpy quirks. ~~Needs to be rebased after #111175 and #111159 are merged.~~

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2024-02-23T01:31:40Z

Hey there @Adminiuga, @puddly, @TheJulianJES, mind taking a look at this pull request as it has been labeled with an integration (zha) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of zha can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign zha Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

tests/components/zha/test_discover.py

homeassistant/components/zha/core/discovery.py

homeassistant/components/zha/switch.py

homeassistant/components/zha/select.py

tests/components/zha/test_sensor.py

homeassistant/components/zha/core/discovery.py

homeassistant/components/zha/entity.py

homeassistant/components/zha/core/discovery.py

tests/components/zha/test_switch.py

tests/components/zha/test_discover.py

TheJulianJES

LGTM, but would appreciate if puddly can also have a look 😄

* Add counter entities to the ZHA coordinator device * rework to prepare for non coordinator device counters * Initial scaffolding to support quirks v2 entities * update for zigpy changes * add assertion error message * clean up test * update group entity discovery kwargs * constants and clearer names * apply custom device configuration * quirks switches * quirks select entities * quirks sensor entities * update discovery * move call to super * add complex quirks v2 discovery test * remove duplicate replaces * add quirks v2 button entity support * add quirks v2 binary sensor entity support * fix exception in counter entitiy discovery * oops * update formatting * support custom on and off values * logging * don't filter out entities quirks says should be created * fix type alias warnings * sync up with zigpy changes and additions * add a binary sensor test * button coverage * switch coverage * initial select coverage * number coverage * sensor coverage * update discovery after rebase * coverage * single line * line lengths * fix double underscore * review comments * set category from quirks in base entity * line lengths * move comment * imports * simplify * simplify

MartinHjelmare

I think the design of this feature is questionable. We want to review the data that is used for the Home Assistant Entity API where the Entity API doesn't allow arbitrary values. That data is now hidden in many cases in a 3rd party library.

MartinHjelmare · 2024-03-04T11:55:03Z

homeassistant/components/zha/entity.py

+            self._attr_entity_registry_enabled_default = False
+
+        if entity_metadata.translation_key:
+            self._attr_translation_key = entity_metadata.translation_key


The translation key must be set with a string from the integration since it must map to a string in the strings.json file. We don't allow setting it directly with data from a 3rd party library.

MartinHjelmare · 2024-03-04T11:57:02Z

homeassistant/components/zha/number.py

+        if number_metadata.step is not None:
+            self._attr_native_step = number_metadata.step
+        if number_metadata.unit is not None:
+            self._attr_native_unit_of_measurement = number_metadata.unit


Units are preferred to use the Home Assistant constants as far as possible.

We are using clones of the HA units in the lib

We still prefer using the Home Assistant constants when possible.

MartinHjelmare · 2024-03-04T12:01:25Z

homeassistant/components/zha/sensor.py

+        ZhaEntity._init_from_quirks_metadata(self, entity_metadata)  # pylint: disable=protected-access
+        sensor_metadata: ZCLEnumMetadata = entity_metadata.entity_metadata
+        self._attribute_name = sensor_metadata.attribute_name
+        self._enum = sensor_metadata.enum


Side note: Where is the options property set for the enum sensors with device class enum?

https://developers.home-assistant.io/docs/core/entity/sensor#available-device-classes

Options are computed from the enum values and that has not been changed by this PR

Where is the options set?

MartinHjelmare · 2024-03-04T12:06:23Z

homeassistant/components/zha/sensor.py

+        """Init this entity from the quirks metadata."""
+        ZhaEntity._init_from_quirks_metadata(self, entity_metadata)  # pylint: disable=protected-access
+        sensor_metadata: ZCLEnumMetadata = entity_metadata.entity_metadata
+        self._attribute_name = sensor_metadata.attribute_name


This will create extra state attributes that we don't see the name of. We want to review all extra attributes before they are added. There must at minimum be a map in the integration for these.

This is the ZCL attribute for the sensor and has nothing to do with HA state extra attributes.

The attribute name is at least used here for an extra state attribute:

core/homeassistant/components/zha/sensor.py

Line 460 in c2a9916

max_attr_name = f"{self._attribute_name}_max"

That’s in a completely different concrete class… and the attribute name is hard coded in it:

core/homeassistant/components/zha/sensor.py

Line 447 in c2a9916

_attribute_name = "active_power"

The class constructor, the __init__ method, will overwrite the set class attributes if an instance attribute with the same name is set. It looks like we could potentially overwrite all entity classes that inherit a constructor with the quirks interface. I don't see any guard for that.

There won’t be any metadata in these cases. I can add a check to them that if it is present they also throw a hard error… Or I can move the metadata handling to a new extension of the base and point all quirks stuff at that so none of the other classes have to worry about this… couple ways to solve it.

dmulcahey · 2024-03-04T12:40:45Z

I think the design of this feature is questionable. We want to review the data that is used for the Home Assistant Entity API where the Entity API doesn't allow arbitrary values. That data is now hidden in many cases in a 3rd party library.

How do you review this for mqtt? It seems to work similarly to what is done here? These changes were made to simplify user contributions and to remove boilerplate code. For instance:

core/homeassistant/components/zha/sensor.py

Line 1287 in c2a9916

@MULTI_MATCH(cluster_handler_names="ikea_airpurifier")

what is the benefit in having someone define every class below this point when it can be handled the way we have in this PR.

I think some further work can be done to validate that we aren’t using arbitrary values for unit and device classes / state classes and throwing a hard error if they aren’t part of the HA enums. The only thing we can’t solve like that is translation key. For that I think we can add a unit test in ZHA that ensures every supplied key from quirks is in strings.json. Wouldn’t that solve the other issue?

I’d rather drop the feature in its entirety If we are saying we are going to be forced to maintain boilerplate maps in every platform in ZHA to essentially copy this info to HA. It defeats the entire purpose of what we were trying to accomplish. We are trying to make it easier to contribute for users on the Zigpy side (similar to how easy it is to contribute to z2m they also don’t have these requirements…)

MartinHjelmare · 2024-03-04T12:54:26Z

I understand the reason behind the feature, but I don't think this solution is in line with how we want to review integrations in Home Assistant. We want to make sure that every integration follows our design and does the right thing. That's not possible if all the data is hidden in a 3rd party library.

Although we validate many things today, we can't validate everything automatically. Some things still require a reviewer to be able to read the code and check that the guidelines are followed. Examples include using entity category and extra state attributes.

dmulcahey · 2024-03-04T12:58:19Z

I understand the reason behind the feature, but I don't think this solution is in line with how we want to review integrations in Home Assistant. We want to make sure that every integration follows our design and does the right thing. That's not possible if all the data is hidden in a 3rd party library.

Although we validate many things today, we can't validate everything automatically. Some things still require a reviewer to be able to read the code and check that the guidelines are followed. Examples include using entity category and extra state attributes.

How is this done today for z2m (or any mqtt integration)? We can’t have it both ways… ZHA is immediately at a disadvantage with contributions if we have to jump through hoops that they don’t for user contributions. I’m willing to do the work to lock this down with validation etc… but if we are saying this isn’t possible (I fail to see how it isn’t) 🤷🏻‍♂️

MartinHjelmare · 2024-03-04T13:07:43Z

The MQTT integration is an exception since it's configured by the user and not written by the developer.

dmulcahey · 2024-03-04T13:20:41Z

I think I can shield all of this in the classes… I can validate all units, classes, categories, etc against the enums in HA, I can add a unit test to ensure all supported metadata translation keys exist, I can split the class hierarchies so that entities derived from metadata can’t “change” things in extensions. What is still a problem at that point?

MartinHjelmare · 2024-03-04T14:03:55Z

The problem is that we can't review what entities are created and how the entity API is used. Ie the final combination of all the features of the entity is hidden from the reviewer. Home Assistant is no longer in control of how the entity is created.

dmulcahey · 2024-03-04T14:23:05Z

The problem is that we can't review what entities are created and how the entity API is used. Ie the final combination of all the features of the entity is hidden from the reviewer.

Sure the final combination of unit, translation key, enabled / disabled by default may not always be visible… but why would it need to be? None of that would violate / abuse HA’s architecture in any way. The entity classes in ZHA still control everything and they are reviewed…

Home Assistant is no longer in control of how the entity is created.

This is patently false. The entity classes are still in HA and nothing can be done w/o changing them. There is nothing magical in the metadata coming from the library and I have offered to validate everything on the HA side.

This entire discussion boils down to just not liking how this is done. I have yet to read anything concrete in the form of solutions other than duplicate all possible combinations of the metadata into HA which defeats the purpose. Let’s take the remainder of this discussion offline as I don’t think we are getting anywhere useful like this.

home-assistant bot added cla-signed has-tests integration: zha new-feature labels Feb 23, 2024

home-assistant bot assigned Adminiuga, puddly and TheJulianJES Feb 23, 2024

home-assistant bot added by-code-owner Quality Scale: No score labels Feb 23, 2024

dmulcahey force-pushed the dm/quirksv2-implementation branch from 273d3bd to d2bbc66 Compare February 23, 2024 18:42

dmulcahey and others added 19 commits February 28, 2024 14:56

Add counter entities to the ZHA coordinator device

e19a26a

rework to prepare for non coordinator device counters

fbac356

Initial scaffolding to support quirks v2 entities

29edee1

update for zigpy changes

701d4ba

add assertion error message

5eb9ef8

clean up test

ede11e5

update group entity discovery kwargs

7d02aed

constants and clearer names

a52d5cd

apply custom device configuration

307dfe9

quirks switches

b8e1337

quirks select entities

f167ded

quirks sensor entities

b5f5951

update discovery

480fbe3

move call to super

edd5667

add complex quirks v2 discovery test

ebd79b4

remove duplicate replaces

2b91e33

add quirks v2 button entity support

25fa855

add quirks v2 binary sensor entity support

02f4c1d

fix exception in counter entitiy discovery

3241b4f

dmulcahey marked this pull request as ready for review February 28, 2024 22:02

dmulcahey requested review from Adminiuga, puddly and TheJulianJES as code owners February 28, 2024 22:02

TheJulianJES reviewed Feb 29, 2024

View reviewed changes

dmulcahey added 9 commits February 29, 2024 07:06

single line

bcc5b27

line lengths

64ddc80

fix double underscore

5563199

review comments

de774e6

set category from quirks in base entity

d299c50

line lengths

2345ab9

move comment

bf2dfc3

imports

d33609d

simplify

a06cddf

TheJulianJES approved these changes Feb 29, 2024

View reviewed changes

simplify

24d2221

balloob merged commit 73b6e2b into home-assistant:dev Feb 29, 2024
19 checks passed

balloob added the cherry-picked label Feb 29, 2024

github-actions bot locked and limited conversation to collaborators Mar 1, 2024

MartinHjelmare reviewed Mar 4, 2024

View reviewed changes

home-assistant unlocked this conversation Mar 4, 2024

github-actions bot locked and limited conversation to collaborators Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ZHA entities exposed by Zigpy quirks #111176

Add support for ZHA entities exposed by Zigpy quirks #111176

dmulcahey commented Feb 23, 2024 •

edited

Loading

home-assistant bot commented Feb 23, 2024

TheJulianJES left a comment

MartinHjelmare left a comment

MartinHjelmare Mar 4, 2024

MartinHjelmare Mar 4, 2024

dmulcahey Mar 4, 2024

MartinHjelmare Mar 4, 2024

MartinHjelmare Mar 4, 2024

dmulcahey Mar 4, 2024 •

edited

Loading

MartinHjelmare Mar 4, 2024

MartinHjelmare Mar 4, 2024 •

edited

Loading

dmulcahey Mar 4, 2024

MartinHjelmare Mar 4, 2024

dmulcahey Mar 4, 2024

MartinHjelmare Mar 4, 2024 •

edited

Loading

dmulcahey Mar 4, 2024 •

edited

Loading

dmulcahey commented Mar 4, 2024 •

edited

Loading

MartinHjelmare commented Mar 4, 2024

dmulcahey commented Mar 4, 2024

MartinHjelmare commented Mar 4, 2024

dmulcahey commented Mar 4, 2024

MartinHjelmare commented Mar 4, 2024

dmulcahey commented Mar 4, 2024

Add support for ZHA entities exposed by Zigpy quirks #111176

Add support for ZHA entities exposed by Zigpy quirks #111176

Conversation

dmulcahey commented Feb 23, 2024 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Feb 23, 2024

TheJulianJES left a comment

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmulcahey Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

dmulcahey Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

dmulcahey commented Mar 4, 2024 • edited Loading

MartinHjelmare commented Mar 4, 2024

dmulcahey commented Mar 4, 2024

MartinHjelmare commented Mar 4, 2024

dmulcahey commented Mar 4, 2024

MartinHjelmare commented Mar 4, 2024

dmulcahey commented Mar 4, 2024

dmulcahey commented Feb 23, 2024 •

edited

Loading

dmulcahey Mar 4, 2024 •

edited

Loading

MartinHjelmare Mar 4, 2024 •

edited

Loading

MartinHjelmare Mar 4, 2024 •

edited

Loading

dmulcahey Mar 4, 2024 •

edited

Loading

dmulcahey commented Mar 4, 2024 •

edited

Loading