Repository navigation
Fix analog output number losing its range, unit and name on the first read - #917
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #917 +/- ##
=======================================
Coverage 97.22% 97.22%
=======================================
Files 61 61
Lines 10657 10666 +9
=======================================
+ Hits 10361 10370 +9
Misses 296 296 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@zigpy-review-bot Review this PR. Also, check where else in ZHA we use |
zigpy-review-bot
left a comment
There was a problem hiding this comment.
The fix is correct and verified, and I found no defect in the diff. This is comment-only because the open point is the design question from the thread (where recompute_capabilities() should run), which is a maintainer decision. There is a small central alternative that would let this PR drop its new __init__, described in answer 2.
Answers to the three questions (evidence in the inline comments):
functools.cached_property: 54 uses inzha/ondev, 52 with this PR. Of the two this PR converts,BaseNumber.native_unit_of_measurementwas the only entity property in ZHA caching a value thatrecompute_capabilities()rewrites.BaseNumber.modeis not rewritten by any override, so that conversion is harmless but not strictly needed. The others return per-class constants, values fixed at construction, or are invalidated explicitly (Device._ZIGPY_CACHED_PROPERTIES,Group.clear_caches()). Two caveats:BaseFan.supported_featurescaches_attr_supported_features, which is only safe while that stays a class constant, and the group caches hold snapshots of member entity state (details inline).recompute_capabilities()in__init__: the PR's "as the other platforms do" is accurate. Binary sensor, climate, cover, light, sensor, fan and button all call it in__init__. The only central call during initialization runs inDevice._add_pending_entities, after the initial attribute reads, while entities subscribe to attribute events inon_add()before those reads. So every entity has to be state-ready before the first read, and each platform currently guarantees that on its own. Ondev,AnalogOutputNumberwas the one class with neither an__init__call nor class-level defaults. Option: oneentity.recompute_capabilities()inDevice._discover_new_entitiesbeforeentity.on_add()gives that guarantee centrally. With that line and without this PR's__init__, the full suite still passes (1397)._capability_attribute_ids: nothing equivalent exists under another name. The only precedent isCover, which hardcodes one attribute (window_covering_type) in its handler. Every other entity that derives capabilities from attributes never refreshes them after_add_pending_entities. I confirmed that forIASZone(zone_type) andAnalogInputSensor(engineering_units,description). So the concept is new and would be worth generalizing rather than staying number-only, but that is a follow-up, not something this PR needs.
Verified (8 checks)
- Full suite at 6365c77: 1397 passed.
ruff check,ruff format --checkand mypy inside the venv are clean, with no mypy change against the base. - The PR's tests against the base source (fc5b094): 7 failed, 8 passed. Every new or changed case fails without the fix, as the description says.
- Reproduced the failure on the base:
AttributeError: 'AnalogOutputNumber' object has no attribute '_attr_native_max_value'raised fromhandle_attribute_updated→state, inside zigpy's_process_read_attributes_response. zigpy'semit()does not guard listener exceptions, so the rest of that read response is not processed, andinitialize_cluster_configslogs it at debug level only (zha/zigbee/cluster_config.py:283). - Keeping the two
cached_propertydecorators with the rest of the PR applied:test_number[False]andtest_number_capabilities_updatedfail with unitNone, so the unit conversion is required. - The
digi-xbee3.jsonchange (0→0.0) follows frommin_val or 0becoming anis Nonecheck: a cached0.0is now kept as a float. - The branch merges cleanly into current
dev, which gained #918 in the same method. - Central-call probes (scratch worktree, not pushed anywhere): results in the inline comment on
__init__. - Copilot second opinion on the diff: no findings.
| return self._attr_native_step | ||
|
|
||
| @functools.cached_property | ||
| @property |
There was a problem hiding this comment.
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,stateandall_member_entity_unique_idsare cleared byGroup.clear_caches()(group.py:252). TheGroupMemberones (endpoint,member_info,associated_entities) are not cleared individually and live as long as theGroupMemberobject, which is rebuilt whenmembersis cleared. Note thatGroup.state(group.py:199) andGroupMember.member_info(group.py:99) embedentity.statesnapshots, so they keep whatever state the entities had when first accessed until the nextclear_caches(). That predates this PR and is the closest relative of the bug fixed here.zha/zigbee/endpoint.py(4),zha/async_.py(1),identifiersinzha/application/platforms/__init__.py(3) andDeviceCounterSensor.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 __init__(self, endpoint: Endpoint, device: Device, **kwargs: Any) -> None: | ||
| """Init this number.""" | ||
| super().__init__(endpoint=endpoint, device=device, **kwargs) | ||
| self.recompute_capabilities() |
There was a problem hiding this comment.
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):
_discover_new_entities()constructs each entity and callsentity.on_add()(device.py:1104), which subscribes to the cluster's attribute events.initialize_cluster_configs()reads attributes. Each record emits an event synchronously, and the entity builds its state in the handler._add_pending_entities()callsentity.recompute_capabilities()(device.py:1170) and then checksis_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 (keepingLightGroup): 6test_devices_from_filessnapshots change. Four Frient snapshots gain anIASZoneentity, and twolumi.curtain.acn002snapshots getnullforis_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, andCover.__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.
| ), | ||
| } | ||
|
|
||
| _capability_attribute_ids = frozenset( |
There was a problem hiding this comment.
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 callsrecompute_capabilities()and emits whenwindow_covering_typechanges. 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),BinaryInputWithDescriptionandIASZone(binary_sensor/__init__.py:321,:438),BinaryOutputSwitch(switch.py:277),Thermostat(climate/__init__.py:531, whereTARGET_TEMPERATURE_RANGEdepends onctrl_sequence_of_oper),Light(light/__init__.py:1042) and the number overrides atnumber/__init__.py:598,:1059,:1093,:1121. - Probed two of them: an
IASZonejoined withoutzone_typekeepsdevice_class=Noneafterzone_typeis reported, and anAnalogInputSensorkeeps its old unit and name afterengineering_units/descriptionare reported. A manualrecompute_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.
Reading Analog Output attributes from the device failed with
'AnalogOutputNumber' object has no attribute '_attr_native_max_value':handle_attribute_updatedbuilds the state on everyattribute_readevent, and duringinitialize_cluster_configs()that happens beforerecompute_capabilities()has run. The exception abortsread_attributes()after the first record and is only logged at debug level. With an empty cache, as on every join and re-interview, onlymin_present_valueandpresent_valueget cached, so the entity ends up with1023as maximum and no step, unit or name.Changes:
AnalogOutputNumber.__init__callsrecompute_capabilities(), as the other platforms do.native_unit_of_measurementandmodeare plain properties instead ofcached_propertys, since the state is now built before the finalrecompute_capabilities().handle_attribute_updatedrecomputes the capabilities when a configuration attribute changes. This also repairs devices joined with the old code once the missing values are read, e.g. by the startup poll of mains-powered devices; Home Assistant shows the name only after the next restart. Battery-powered devices still need a Reconfigure.0is kept instead of falling back to1023.0,inforNaNis not used as step, as inAnalogInputSensor.Tests:
test_numbernow runs with and without a filled cache and checks that no read failed. New tests cover late attribute updates, a maximum of0and invalid resolutions. All of them fail without the change; the full suite passes (1397).Hardware: ESP32-C6 end device with a 0-100 power limit slider, Home Assistant 2026.9.4 with
zha2.3.0 from PyPI, then this branch, both on zigpy 2.3.0. Reconfigure gave0-1023, step 1 and no unit or name before the change, and0-100, step 10,%and the name fromdescriptionwith it. A device left broken by the old code was repaired by the startup poll after a restart.