Add AnalogInput sensor, use description attribute for fallback_name #197
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #197 +/- ##
==========================================
+ Coverage 96.82% 96.88% +0.05%
==========================================
Files 61 63 +2
Lines 9997 10404 +407
==========================================
+ Hits 9680 10080 +400
- Misses 317 324 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I wonder: would it be feasible to instead set the translation key to the ZCL |
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. |
zha/application/platforms/sensor/__init__.py
Ou
8000
tdated
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 As far as splitting the PR, it was originally planned to have multiple PRs, one for the |
|
This will need a zigpy release before tests will pass. |
a76bb19 to
7f12961
Compare
8fd863b to
bd34bfd
Compare
5a8c916 to
3b21131
Compare
|
I've pushed a whole host of changes to this branch, apologies! There were a few bugs that needed fixing in ZHA (and thus some tweaks to Core):
@P-R-O-C-H-Y Give this branch a try. For analog input:
For multistate input:
The only implementation detail I'm still unsure of is
In the ZCL, arrays are bizarrely both 0- and 1-indexed: the zero-th element is the size of the array, and the first element is the first element is the first. I think this means that |
|
@puddly I am having some issues with Analog Input. It is showing up now, but the Description and Application type is not used. |
|
@P-R-O-C-H-Y This should be fixed by zigpy/zigpy#1593. With the device diagnostics you provided, the device is correctly shown: {
"info_object": {
+ "fallback_name": "Power Consumption (Watts)",
"unique_id": "ab:cd:ef:12:39:a2:73:55-1-12-analog_input",
"migrate_unique_ids": [],
"platform": "sensor",
"class_name": "AnalogInputSensor",
"translation_key": null,
"device_class": "power",
"state_class": "measurement",
"entity_category": null,
"entity_registry_enabled_default": true,
"enabled": true,
"primary": false,
"cluster_handlers": [
{
"class_name": "AnalogInputClusterHandler",
"generic_id": "cluster_handler_0x000c",
"endpoint_id": 1,
"cluster": {
"id": 12,
"name": "AnalogInput",
"type": "server"
},
"id": "1:0x000c",
"unique_id": "ab:cd:ef:12:39:a2:73:55:1:0x000c",
"status": "INITIALIZED",
"value_attribute": "present_value"
}
],
"device_ieee": [ 85, 115, 162, 57, 18, 239, 205, 171],
"endpoint_id": 1,
"available": true,
"group_id": null,
"attribute": "present_value",
"suggested_display_precision": null,
"divisor": 1,
"multiplier": 1,
+ "unit": "W",
"options": null
},
"state": {
"class_name": "AnalogInputSensor",
"available": true,
+ "state": 280.0
}The unit is correctly identified as well. If you implement the |
|
@puddly Can you give a hint about the |
|
I've removed the |
| @@ -99,6 +95,14 @@ def on_add(self) -> None: | |||
| self.handle_cluster_handler_attribute_updated, | |||
| ) | |||
| ) | |||
| if ( | |||
| hasattr(self._analog_output_cluster_handler, "description") | |||
There was a problem hiding this comment.
The hasattr check shouldn't be needed, as the description property will always be present on the AnalogOutputClusterHandler that's guaranteed here since #442 in (now called) AnalogOutputNumber.
So, only the None check below is needed.
| if ( | ||
| hasattr(self._cluster_handler, "description") | ||
| and self._cluster_handler.description is not None | ||
| ): | ||
| self._attr_translation_key = None | ||
| self._attr_fallback_name: str = self._cluster_handler.description |
There was a problem hiding this comment.
Having this in the base Sensor class feels a bit weird. The description attribute will only be available on the AnalogInputClusterHandler used for AnalogInputSensor.
So, shouldn't we just move this part there (and remove the hasattr check as well)?
| "info_object": { | ||
| "fallback_name": null, | ||
| "fallback_name": "fading_time", | ||
| "unique_id": "ab:cd:ef:12:94:85:f7:5f-2-13", | ||
| "migrate_unique_ids": [], | ||
| "platform": "number", | ||
| "class_name": "Number", | ||
| "translation_key": "number", | ||
| "translation_key": null, | ||
| "device_class": null, | ||
| "state_class": null, | ||
| "entity_category": null, |
There was a problem hiding this comment.
More of a note: These entities shouldn't be present anymore.
We've replaced these AnalogOutput config entities for a lot of Tuya sensors with proper quirks v2 entities.
However, they'll still be in the zigpy attribute cache, until the device is re-paired.
I thought that the quirks v1 signature/replacement shouldn't persist to the database, but I remember we've had that issue at one point, so the quirked signature might be stuck in the database, or these were saved when the v1 quirks were still present and the regeneration of diagnostics just looked at the quirked signature and not the original signature?
We might wanna add a .removes(AnalogInput.cluster_id, endpoint_id=xx) to a whole lot of Tuya quirks for the various fake config entities that were added in the past...
Assuming these entities are still "provided" (but non-functional) for real users.
|
@puddly @TheJulianJES Can we take a look again into this and add MultiState Input/Output? Thanks |
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