Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tests/data/devices/digi-xbee3.json
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@
"native_value": null,
"mode": "auto",
"native_max_value": 1023.0,
"native_min_value": 0,
"native_min_value": 0.0,
"native_step": 1.0,
"native_unit_of_measurement": null
},
Expand All @@ -531,7 +531,7 @@
"native_value": null,
"mode": "auto",
"native_max_value": 1023.0,
"native_min_value": 0,
"native_min_value": 0.0,
"native_step": 1.0,
"native_unit_of_measurement": null
}
Expand Down
110 changes: 108 additions & 2 deletions tests/test_number.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Test zha number platform."""

import logging
from unittest.mock import call

import pytest
Expand Down Expand Up @@ -79,10 +80,17 @@ async def light_mock(zha_gateway: Gateway) -> ZigbeeDevice:
return zigpy_device


@pytest.mark.parametrize("cached", [True, False])
async def test_number(
zha_gateway: Gateway,
cached: bool,
caplog: pytest.LogCaptureFixture,
) -> None:
"""Test zha number platform."""
"""Test zha number platform.

Without a cached value, everything has to be read from the device, as on the
(re-)interview of a device.
"""
zigpy_analog_output_device = create_mock_zigpy_device(
zha_gateway, ZIGPY_ANALOG_OUTPUT_DEVICE
)
Expand All @@ -98,10 +106,13 @@ async def test_number(
"engineering_units": 98,
"application_type": 4 * 0x10000,
}
update_attribute_cache(cluster)
if cached:
update_attribute_cache(cluster)
cluster.PLUGGED_ATTR_READS["present_value"] = 15.0

caplog.set_level(logging.DEBUG, logger="zha.zigbee.cluster_config")
zha_device = await join_zigpy_device(zha_gateway, zigpy_analog_output_device)
assert "Failed to read attributes" not in caplog.text
# one for present_value and one for the rest configuration attributes
assert cluster.read_attributes.call_count == 2
attr_reads = set()
Expand Down Expand Up @@ -179,6 +190,101 @@ async def test_number(
assert entity.state.native_value == 30.0


async def test_number_capabilities_updated(
zha_gateway: Gateway,
) -> None:
"""Test the range, step and unit follow the device after the entity was added.

A device paired with an incomplete cache only gets the missing values later, e.g.
from the startup poll or a report.
"""
zigpy_analog_output_device = create_mock_zigpy_device(
zha_gateway, ZIGPY_ANALOG_OUTPUT_DEVICE
)
cluster: general.AnalogOutput = zigpy_analog_output_device.endpoints.get(
1
).analog_output
cluster.PLUGGED_ATTR_READS = {"min_present_value": 5.0, "present_value": 50.0}
update_attribute_cache(cluster)

zha_device = await join_zigpy_device(zha_gateway, zigpy_analog_output_device)
entity: PlatformEntity = get_entity(zha_device, platform=Platform.NUMBER)

assert entity.state.native_min_value == 5.0
assert entity.state.native_max_value == 1023
assert entity.state.native_step is None
assert entity.state.native_unit_of_measurement is None
assert entity.fallback_name is None

await send_attributes_report(
zha_gateway,
cluster,
{
"max_present_value": 100.0,
"resolution": 10.0,
"engineering_units": 98,
"description": "Power Limit",
},
)
await zha_gateway.async_block_till_done()

assert entity.state.native_min_value == 5.0
assert entity.state.native_max_value == 100.0
assert entity.state.native_step == 10.0
assert entity.state.native_unit_of_measurement == "%"
assert entity.fallback_name == "Power Limit"


async def test_number_zero_max_value(
zha_gateway: Gateway,
) -> None:
"""Test a maximum of 0 is not replaced by the default."""
zigpy_analog_output_device = create_mock_zigpy_device(
zha_gateway, ZIGPY_ANALOG_OUTPUT_DEVICE
)
cluster: general.AnalogOutput = zigpy_analog_output_device.endpoints.get(
1
).analog_output
cluster.PLUGGED_ATTR_READS = {
"min_present_value": -100.0,
"max_present_value": 0.0,
"present_value": -50.0,
}
update_attribute_cache(cluster)

zha_device = await join_zigpy_device(zha_gateway, zigpy_analog_output_device)
entity: PlatformEntity = get_entity(zha_device, platform=Platform.NUMBER)

assert entity.state.native_min_value == -100.0
assert entity.state.native_max_value == 0.0


@pytest.mark.parametrize("resolution", [0.0, float("inf"), float("nan")])
async def test_number_invalid_resolution(
zha_gateway: Gateway,
resolution: float,
) -> None:
"""Test an invalid resolution is not used as step."""
zigpy_analog_output_device = create_mock_zigpy_device(
zha_gateway, ZIGPY_ANALOG_OUTPUT_DEVICE
)
cluster: general.AnalogOutput = zigpy_analog_output_device.endpoints.get(
1
).analog_output
cluster.PLUGGED_ATTR_READS = {
"min_present_value": 0.0,
"max_present_value": 1.0,
"resolution": resolution,
"present_value": 0.5,
}
update_attribute_cache(cluster)

zha_device = await join_zigpy_device(zha_gateway, zigpy_analog_output_device)
entity: PlatformEntity = get_entity(zha_device, platform=Platform.NUMBER)

assert entity.state.native_step is None


async def test_number_missing_description_attr(
zha_gateway: Gateway,
) -> None:
Expand Down
39 changes: 31 additions & 8 deletions zha/application/platforms/number/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@

from abc import ABC, abstractmethod
from dataclasses import dataclass
import functools
import logging
import math
from typing import TYPE_CHECKING, Any

from zigpy.zcl import (
Expand Down Expand Up @@ -110,12 +110,12 @@ def native_step(self) -> float | None:
"""Return the value step."""
return self._attr_native_step

@functools.cached_property
@property

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Inventory for the cached_property question (line numbers at this PR's head). 52 uses remain:

  • zha/zigbee/device.py (14): all listed in _ZIGPY_CACHED_PROPERTIES (device.py:344) and deleted when the zigpy device is swapped (device.py:411-413). The tuple and the decorated properties match one to one.
  • zha/zigbee/group.py (6): members, state and all_member_entity_unique_ids are cleared by Group.clear_caches() (group.py:252). The GroupMember ones (endpoint, member_info, associated_entities) are not cleared individually and live as long as the GroupMember object, which is rebuilt when members is cleared. Note that Group.state (group.py:199) and GroupMember.member_info (group.py:99) embed entity.state snapshots, so they keep whatever state the entities had when first accessed until the next clear_caches(). That predates this PR and is the closest relative of the bug fixed here.
  • zha/zigbee/endpoint.py (4), zha/async_.py (1), identifiers in zha/application/platforms/__init__.py (3) and DeviceCounterSensor.identifiers / .device (sensor/__init__.py:459, :495): values fixed at construction.
  • fan/__init__.py (13), climate/__init__.py (4), cover/__init__.py:919, :924, button/__init__.py:110, :115, device_tracker.py:186: per-class constants or values set in __init__ (_fan_cluster, _args, _kwargs).

Apart from the group snapshots, none of these depends on a value that a recompute_capabilities() override rewrites. Of the two converted here, only native_unit_of_measurement did (_attr_native_unit_of_measurement, line 203). Nothing writes _attr_mode after NumberConfigurationEntity.__init__, so mode would have been fine either way. The one to keep an eye on is BaseFan.supported_features (fan/__init__.py:130-133): it caches self._attr_supported_features, which today is only ever a class attribute (fan/__init__.py:82, :393, :568). If a fan ever computes features from attributes it would go stale the same way.

There is no generic invalidation for entity-level cached properties. The two mechanisms above are specific to Device and Group. Plain properties, as done here, match what the rest of BaseNumber and the other platforms' capability properties already do.

Related, on the Home Assistant side: ZHAEntity.name is a cached_property (homeassistant/components/zha/entity.py:83 in home-assistant/core) and the number platform's _update_capability_attrs (number.py:56) refreshes only mode, min, max, step and unit. That is why a description that arrives after the entity exists shows up in HA only after a restart, as the PR description notes.

def native_unit_of_measurement(self) -> str | None:
"""Return the unit the value is expressed in."""
return self._attr_native_unit_of_measurement

@functools.cached_property
@property
def mode(self) -> NumberMode:
"""Return the mode of the entity."""
return self._attr_mode
Expand Down Expand Up @@ -168,17 +168,38 @@ class AnalogOutputNumber(BaseNumber):
),
}

_capability_attribute_ids = frozenset(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

On whether this concept exists elsewhere: not as a shared mechanism. PlatformEntity has no common attribute handler. Each platform subscribes in its own on_add() and filters in its own handle_attribute_updated.

  • The only precedent is Cover.handle_attribute_updated (cover/__init__.py:608-610), which calls recompute_capabilities() and emits when window_covering_type changes. Same idea, one hardcoded attribute.
  • Entities that compute capabilities from attributes but never refresh them on a later read or report: AnalogInputSensor (sensor/__init__.py:691), SmartEnergyMetering (sensor/__init__.py:2370), BinaryInputWithDescription and IASZone (binary_sensor/__init__.py:321, :438), BinaryOutputSwitch (switch.py:277), Thermostat (climate/__init__.py:531, where TARGET_TEMPERATURE_RANGE depends on ctrl_sequence_of_oper), Light (light/__init__.py:1042) and the number overrides at number/__init__.py:598, :1059, :1093, :1121.
  • Probed two of them: an IASZone joined without zone_type keeps device_class=None after zone_type is reported, and an AnalogInputSensor keeps its old unit and name after engineering_units / description are reported. A manual recompute_capabilities() fixes both.

So there is nothing to reuse, and the set here is fine for this PR. It does repeat six of the eight attributes declared in _server_cluster_config just above. If this is generalized later, a natural shape is a set (or an AttrConfig flag) on the base entity plus one shared handler that recomputes and emits. Two things such a generalization has to handle: several of those entities also gate _is_supported() on the same attributes (AnalogInputSensor, BinaryOutputSwitch), which needs entity re-discovery rather than only a capability recompute, and the handlers are per-platform today.

{
AnalogOutput.AttributeDefs.min_present_value.id,
AnalogOutput.AttributeDefs.max_present_value.id,
AnalogOutput.AttributeDefs.resolution.id,
AnalogOutput.AttributeDefs.description.id,
AnalogOutput.AttributeDefs.engineering_units.id,
AnalogOutput.AttributeDefs.application_type.id,
}
)

def __init__(self, endpoint: Endpoint, device: Device, **kwargs: Any) -> None:
"""Init this number."""
super().__init__(endpoint=endpoint, device=device, **kwargs)
self.recompute_capabilities()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

On the __init__ question: the claim in the description holds. Classes whose __init__ calls self.recompute_capabilities(): BinarySensor (binary_sensor/__init__.py:115), Thermostat (climate/__init__.py:395), Cover (cover/__init__.py:200), Shade (cover/__init__.py:854), Light (light/__init__.py:1005), LightGroup (light/__init__.py:1376), Sensor (sensor/__init__.py:283), Fan (fan/__init__.py:279) and WriteAttributeButton (button/__init__.py:171). The last two have no override in their class tree, so the call is a no-op today.

Overrides that rely only on the central call: StartUpColorTemperatureConfigurationEntity (number/__init__.py:598), ZCLHeatSetpointLimitEntity, MaxHeatSetpointLimit and MinHeatSetpointLimit (number/__init__.py:1059, :1093, :1121) and BinaryOutputSwitch (switch.py:277). They do not crash because the attributes they set have class-level defaults (NumberConfigurationEntity at number/__init__.py:274-275, BaseEntity._attr_fallback_name). AnalogOutputNumber had neither, since BaseNumber only annotates _attr_native_max_value / _attr_native_min_value (lines 74-75).

Lifecycle in Device.async_initialize (zha/zigbee/device.py:1230):

  1. _discover_new_entities() constructs each entity and calls entity.on_add() (device.py:1104), which subscribes to the cluster's attribute events.
  2. initialize_cluster_configs() reads attributes. Each record emits an event synchronously, and the entity builds its state in the handler.
  3. _add_pending_entities() calls entity.recompute_capabilities() (device.py:1170) and then checks is_supported().

So the one central call comes after the point where state is first needed, and "state-ready before the first read" is left to each entity. (Device.recompute_entities() at device.py:1211 also recomputes, but nothing in zha/ calls it outside tests.)

Design option for the maintainers, not a request to change this PR: call entity.recompute_capabilities() in _discover_new_entities before entity.on_add(). What I measured in a scratch worktree:

  • This PR without this __init__, plus that one line: full suite passes (1397), including all tests added here.
  • Additionally removing the eight per-entity __init__ calls for device entities (keeping LightGroup): 6 test_devices_from_files snapshots change. Four Frient snapshots gain an IASZone entity, and two lumi.curtain.acn002 snapshots get null for is_opening / is_closing / is_closed.
  • With the central call placed directly after construction instead (before _is_entity_removed_by_quirk): only the two curtain snapshots change. So quirk entity filters need the capabilities before they run, and Cover.__init__ uses them inside its own __init__ (cover/__init__.py:200-217).

That means the central call is safe to add as a guarantee, placed right after construction, but the existing __init__ calls cannot simply be deleted: Cover needs its own, and group entities are built in discovery.py:149, not through _discover_new_entities. If the central call is added (here or in a follow-up), this __init__ becomes unnecessary.


def recompute_capabilities(self) -> None:
"""Recompute capabilities."""
super().recompute_capabilities()

min_val = self._cluster.get(AnalogOutput.AttributeDefs.min_present_value.name)
max_val = self._cluster.get(AnalogOutput.AttributeDefs.max_present_value.name)
self._attr_native_min_value = min_val or 0
self._attr_native_max_value = max_val or 1023
self._attr_native_step = self._cluster.get(
AnalogOutput.AttributeDefs.resolution.name
)
self._attr_native_min_value = 0 if min_val is None else min_val
self._attr_native_max_value = 1023 if max_val is None else max_val

# Guard against 0 resolution reported by some devices
resolution = self._cluster.get(AnalogOutput.AttributeDefs.resolution.name)
if resolution is not None and math.isfinite(resolution) and resolution > 0:
self._attr_native_step = resolution
else:
self._attr_native_step = None

self._attr_native_unit_of_measurement = BACNET_UNITS_TO_HA_UNITS.get(
self._cluster.get(AnalogOutput.AttributeDefs.engineering_units.name)
)
Expand Down Expand Up @@ -241,6 +262,8 @@ def handle_attribute_updated(
| AttributeWrittenEvent,
) -> None:
"""Handle value update from cluster."""
if event.attribute_id in self._capability_attribute_ids:
self.recompute_capabilities()
self.maybe_emit_state_changed_event()


Expand Down
Loading