Add AnalogInput, MultistateInput, use description attribute for fallback_name #197

prairiesnpr · 2024-09-07T19:06:05Z

This ensures that the description attribute (0x001C), if present, is used for the fallback_name and sets the translation_key to None. The entity id and name will use the fallback_name and align with the devices value. This is mostly useful for devices using general clusters, if not set, the current functionality is maintained.

Due to testing requirements, this now also updates the cluster handlers for AnalogInput, MultistateInput, and BinaryInput.

Closes: #160

Multistate still requires zigpy/zigpy#1456 to be fully implemented, since the LVLists aren't serialized, we create a default value for now.

AnalogOutput is putting the description in the name twice.

This is due to this logic in core, should we add similar logic to core for the rest of the Entities and remove it from ZHA, keep the logic in ZHA, or have it mixed?

https://github.com/home-assistant/core/blob/f1e4229b23ef7837bb4e46877f55d6533f8d4607/homeassistant/components/zha/number.py#L52C9-L52C58

codecov · 2024-09-07T19:17:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.46%. Comparing base (cda1b95) to head (bd34bfd).

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #197      +/-   ##
==========================================
+ Coverage   96.40%   96.46%   +0.05%     
==========================================
  Files          61       61              
  Lines        9359     9469     +110     
==========================================
+ Hits         9023     9134     +111     
+ Misses        336      335       -1

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

puddly · 2024-09-07T19:29:58Z

I wonder: would it be feasible to instead set the translation key to the ZCL description? This could allow the description to be localized.

prairiesnpr · 2024-09-07T19:50:53Z

I wonder: would it be feasible to instead set the translation key to the ZCL description? This could allow the description to be localized.

That was the approach I started with, unfortunately, when the translation key wasn't found, it would return None for the name and id. I understood it should then fallback, but that's not the result I observed in testing.

prairiesnpr · 2024-09-09T20:49:45Z

zha/application/platforms/sensor/const.py

@@ -390,3 +390,36 @@ class SensorDeviceClass(enum.StrEnum):
 SensorDeviceClass.ENUM,
 SensorDeviceClass.TIMESTAMP,
 }
+
+
+class AnalogInputStateClass(enum.Enum):


The ZCL specifies units for each value of ApplicationType, currently we are using units as specified in EngineeringUnits. Both attributes are optional, do we leave this as is, where sitting a ApplicationType only sets the device class, should it also set the units, if so, what should happen if both attributes are set?

If the ApplicationType attribute is implemented, and is set to a value with a defined physical unit, the physical unit defined in ApplicationType takes priority over EngineeringUnits.

I think we should keep things as simple as possible. The spec is expansive and we should only implement the bare minimum to support real devices.

prairiesnpr · 2024-09-09T21:22:00Z

zha/application/platforms/sensor/__init__.py

+ super().__init__(unique_id, cluster_handlers, endpoint, device, **kwargs)
+ engineering_units = self._cluster_handler.engineering_units
+ self._attr_native_unit_of_measurement = UNITS.get(engineering_units)
+ self._attr_state_class = SensorStateClass.MEASUREMENT


Should this always be measurement? I can see a situation where total or total increasing makes sense, but I don't see a good way to choose based on the attributes we have available.

TheJulianJES

There are some restrictions on what we can provide as a translation_key to Home Assistant. AFAIK, we are not allowed to provide translation keys blindly, so not directly from the description attribute, for example.

We'd need to map all known description attribute values to translation keys (and fall-back to a default or untranslated name otherwise).

So, I'm wondering if it would make sense to split this PR into two: one with the translation key changes and one with the added AnalogInput and MultistateInput sensors.

prairiesnpr · 2024-09-09T22:41:13Z

There are some restrictions on what we can provide as a translation_key to Home Assistant. AFAIK, we are not allowed to provide translation keys blindly, so not directly from the description attribute, for example.

We'd need to map all known description attribute values to translation keys (and fall-back to a default or untranslated name otherwise).

So, I'm wondering if it would make sense to split this PR into two: one with the translation key changes and one with the added AnalogInput and MultistateInput sensors.

We aren't modifying the translation_key here, if a description attribute is provided, we set the key to None and then provide a fallback_name, that way we can use arbitrary values without having a matching translation_key. This follows the same logic currently used for custom quirks https://github.com/home-assistant/core/blob/f1e4229b23ef7837bb4e46877f55d6533f8d4607/homeassistant/components/zha/entity.py#L46

As far as splitting the PR, it was originally planned to have multiple PRs, one for the fallback_name functionality and then additional PRs for each cluster. I ran into issues with writing tests there, since we really needed them to both be present to get the required coverage. That said, I'm not opposed to splitting it, if we have a plan on how best to proceed.

puddly · 2024-09-16T18:41:53Z

zha/application/platforms/sensor/const.py

@@ -390,3 +390,36 @@ class SensorDeviceClass(enum.StrEnum):
 SensorDeviceClass.ENUM,
 SensorDeviceClass.TIMESTAMP,
 }
+
+
+class AnalogInputStateClass(enum.Enum):


If the ApplicationType attribute is implemented, and is set to a value with a defined physical unit, the physical unit defined in ApplicationType takes priority over EngineeringUnits.

I think we should keep things as simple as possible. The spec is expansive and we should only implement the bare minimum to support real devices.

puddly · 2024-09-16T18:52:33Z

zha/application/platforms/sensor/__init__.py

+ super().__init__(unique_id, cluster_handlers, endpoint, device, **kwargs)
+ if self._cluster_handler.cluster.get("state_text"):
+ self._enum = enum.Enum( # type: ignore [misc]
+ "state_text", self._cluster_handler.cluster.get("state_text")


Suggested change

"state_text", self._cluster_handler.cluster.get("state_text")

"state_text", self._cluster_handler.cluster["state_text"]

puddly · 2024-09-16T18:53:06Z

zha/application/platforms/sensor/__init__.py

+ [
+ (f"state_{i+1}", i + 1)
+ for i in range(
+ self._cluster_handler.cluster.get("number_of_states")


Suggested change

self._cluster_handler.cluster.get("number_of_states")

self._cluster_handler.cluster["number_of_states"]

puddly · 2024-09-16T19:06:28Z

zha/application/platforms/sensor/const.py

+class AnalogInputStateClass(enum.Enum):
+ """State class for AnalogInput Types."""
+
+ TEMPERATURE = (0x00, SensorDeviceClass.TEMPERATURE)


Let's move this enum to zigpy. We generally don't want to define ZCL enums and types in the ZHA library itself. From the spec:

I think we can define this easily in zigpy:

class AnalogInputType(t.enum8): Temp_In_Degrees_C = 0x00 ... ANALOG_INPUT_TYPES = { AnalogInputType.Temp_In_Degrees_C: TempInDegreesC, } class TempInDegreesC(t.enum16): Two_Pipe_Entering_Water_Temperature = 0x0000 ... class ApplicationType(t.uint32_t, t.Struct): group: t.uint8_t type: AnalogInputType index: t.uint16_t

And then ZHA will only deal with creating a mapping between the ZCL spec units and the HA device classes.

prairiesnpr · 2024-09-25T01:03:30Z

This will need a zigpy release before tests will pass.

prairiesnpr marked this pull request as draft September 7, 2024 20:08

prairiesnpr marked this pull request as ready for review September 7, 2024 23:44

prairiesnpr mentioned this pull request Sep 9, 2024

Add sensor entities for the MultistateInput cluster #167

Closed

prairiesnpr changed the title ~~Use description attribute for fallback_name~~ Add AnalogInput, MultistateInput, use description attribute for fallback_name Sep 9, 2024

prairiesnpr commented Sep 9, 2024

View reviewed changes

TheJulianJES reviewed Sep 9, 2024

View reviewed changes

dmulcahey mentioned this pull request Sep 10, 2024

Add sensor entities for the AnalogInput cluster #147

Closed

puddly reviewed Sep 16, 2024

View reviewed changes

prairiesnpr mentioned this pull request Sep 17, 2024

Add Analog Input ApplicationType Constants zigpy/zigpy#1472

Merged

prairiesnpr force-pushed the use_description_attr branch from a76bb19 to 7f12961 Compare September 25, 2024 15:44

prairiesnpr added 13 commits October 4, 2024 12:00

Use description attribute for fallback_name

fb7e071

Update AnalogIn, MultistateIn, and BinIn CL Handlers

abdd5c6

Register AnalogInput as Sensor

86003fe

Add MultistateInput and AnalogInput Sensor

2c3a9d9

Add Binary Sensor Test

7c803b8

Add Sensor - General test

fa4e91c

Remove xbee AI, Update Dev Sig

75bb500

The empty enum is required

efb6c06

Clean up Multistate Input, better tests

aaf6ad0

cp errors

831442b

Remove constants, update tests and Analog Input

1d0f706

Remove changed device signatures

ab1e9d9

Update tests due to upsteam changes

bd34bfd

prairiesnpr force-pushed the use_description_attr branch from 8fd863b to bd34bfd Compare October 4, 2024 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AnalogInput, MultistateInput, use description attribute for fallback_name #197

Add AnalogInput, MultistateInput, use description attribute for fallback_name #197

prairiesnpr commented Sep 7, 2024 •

edited

Loading

codecov bot commented Sep 7, 2024 •

edited

Loading

puddly commented Sep 7, 2024

prairiesnpr commented Sep 7, 2024 •

edited

Loading

prairiesnpr Sep 9, 2024

puddly Sep 16, 2024

prairiesnpr Sep 9, 2024

TheJulianJES left a comment •

edited

Loading

prairiesnpr commented Sep 9, 2024

puddly Sep 16, 2024

puddly Sep 16, 2024

puddly Sep 16, 2024

puddly Sep 16, 2024

prairiesnpr commented Sep 25, 2024

	"state_text", self._cluster_handler.cluster.get("state_text")
	"state_text", self._cluster_handler.cluster["state_text"]

	self._cluster_handler.cluster.get("number_of_states")
	self._cluster_handler.cluster["number_of_states"]

Add AnalogInput, MultistateInput, use description attribute for fallback_name #197

Are you sure you want to change the base?

Add AnalogInput, MultistateInput, use description attribute for fallback_name #197

Conversation

prairiesnpr commented Sep 7, 2024 • edited Loading

codecov bot commented Sep 7, 2024 • edited Loading

Codecov Report

puddly commented Sep 7, 2024

prairiesnpr commented Sep 7, 2024 • edited Loading

prairiesnpr Sep 9, 2024

Choose a reason for hiding this comment

puddly Sep 16, 2024

Choose a reason for hiding this comment

prairiesnpr Sep 9, 2024

Choose a reason for hiding this comment

TheJulianJES left a comment • edited Loading

Choose a reason for hiding this comment

prairiesnpr commented Sep 9, 2024

puddly Sep 16, 2024

Choose a reason for hiding this comment

puddly Sep 16, 2024

Choose a reason for hiding this comment

puddly Sep 16, 2024

Choose a reason for hiding this comment

puddly Sep 16, 2024

Choose a reason for hiding this comment

prairiesnpr commented Sep 25, 2024

prairiesnpr commented Sep 7, 2024 •

edited

Loading

codecov bot commented Sep 7, 2024 •

edited

Loading

prairiesnpr commented Sep 7, 2024 •

edited

Loading

TheJulianJES left a comment •

edited

Loading