Add attribute_converter/value_converter to number entities - #851
Add attribute_converter/value_converter to number entities#851TheJulianJES wants to merge 1 commit into
attribute_converter/value_converter to number entities#851Conversation
Number entities are writable, so unlike sensors and binary sensors they need converters in both directions: attribute_converter maps the raw attribute value to the value shown in HA, value_converter maps the value from HA back to the raw attribute value. As for sensors, a converter takes precedence over the multiplier.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #851 +/- ##
=======================================
Coverage 97.27% 97.27%
=======================================
Files 55 55
Lines 10941 10953 +12
=======================================
+ Hits 10643 10655 +12
Misses 298 298 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Comment — draft PR, no blockers. The implementation mirrors the existing Sensor/BinarySensor converter precedent correctly as far as I could verify. Answers to Q1/Q2 below, plus three asks.
Re: Q1 — can/should quirks-initialized entities move to zha-quirks itself? (asked in the PR body)
Can: yes. The runtime dependency already points that way — zha-quirks declares dependencies = ["zigpy>=2.0.0", "zha>=2.0.0"], while zha carries zha-quirks only in its testing dependency-group — and zhaquirks/builder/discovery.py already owns metadata → entity construction. What could move is not NumberConfigurationEntity itself (zha's own config numbers subclass it) but quirk-only subclasses of it.
Should: I'd say no, at least not for this feature.
- It widens the coupling instead of narrowing it. Today the seam is a small kwargs contract (
_platform_kwargs→__init__). A quirks-side subclass would instead inherit zha internals —_cluster,_multiplier,write_attributes_safe,maybe_emit_state_changed_event,_is_supported, theon_addevent-subscription lifecycle. Refactors that are free today would become breaking changes for quirks. - It doesn't remove the lockstep it would be meant to remove. Overriding
native_value/async_set_native_valuequirks-side is a single-repo change only for behavior expressible on the existing zha base; anything needing a new hook still needs both repos. Meanwhile number-entity behavior ends up split across two repos, and the next reader has to know which half implements what. - Entity state shape and
unique_idare what HA consumes, and thetests/data/devices/snapshots that cover quirk entities live here. One repo, one source of truth. - This feature isn't quirk-specific anyway — a converter is a generic entity capability a native config number could equally want. That argues for exactly where you put it.
If the real motivation is the two-repo release dance, I'd attack that directly (forward-compatible kwarg handling on the zha side) rather than by relocating entity classes.
Re: Q2 — better naming? (asked in the PR body)
I'd keep attribute_converter / value_converter.
attribute_converteris already shipped public API on.sensor()and.binary_sensor(), so the read direction is effectively fixed — renaming it breaks existing quirks.- The pair is internally consistent under "each converter is named after what it consumes":
attribute_convertertakes the raw attribute,value_convertertakes the HA value. And "value" is already ZHA's word for the HA side (native_value,async_set_native_value), so it's less vague here than it would be in isolation. - Alternatives I weighed:
read_converter/write_converteris the clearest pair but requires renaming the shipped name;attribute_write_converteris unambiguous but asymmetric and verbose. Neither buys enough to justify the churn.
What the names genuinely don't carry is direction at the call site — but that's a docstring problem, not a naming problem. See the inline comment on the __init__ docstring.
Asks
- Mirror quirks#5215's both-or-neither validation on the zha side — the invariant belongs where the behavior is (inline).
- Document the converters, their direction, and their precedence over
multiplierin the entity docstring (inline). - The new tests bypass the quirks plumbing. Not fixable in this PR given the release ordering, but please don't lose the follow-up (inline).
Coupling / release ordering, for quirks#5215
zha#851 has to be released before quirks#5215 merges, and #5215 should bump its zha>= floor — it's still zha>=2.0.0 in that diff.
That's not cosmetic. BaseEntity.__init__(self, unique_id: str) takes no **kwargs, so attribute_converter= forwarded through NumberConfigurationEntity → PlatformEntity → BaseEntity raises TypeError against an older zha. And because Device wraps discover_entities() in a broad except Exception: … continue around a generator (zha/zigbee/device.py:1080-1088), that raise terminates the generator — so the device loses the offending entity and every remaining quirk v2 entity, with nothing but a log line. Pre-existing and out of scope here; noting it only because it makes the floor bump load-bearing.
Verified (8 checks)
tests/test_number.py— 12 passed in an isolated worktree venv (py3.14)../.venv/bin/python -m mypy zha/—Success: no issues found in 57 source files. No regression.- Converter precedence matches
Sensor.native_value(zha/application/platforms/sensor/__init__.py:352-354) andBinarySensor(binary_sensor/__init__.py:150-151): the converter short-circuits before divisor/multiplier/formatter. Consistent, as the PR body claims. value_converterappears nowhere else inzhaquirks/zigpy— no name collision.- Read quirks#5215 in full:
NumberMetadatafields, the__attrs_post_init__both-or-neitherValueError, the_platform_kwargsplumbing, the builder kwargs, and the copilot-instructions docs. t.uint8_t(3.5)→3against the installed zigpy 2.1.0 — a float write is silently truncated, not rejected. Basis for the inline note onasync_set_native_value.- Dependency direction confirmed from both
pyproject.tomls (quoted in the Q1 answer). - Second opinion from Copilot (GPT-5.5,
--effort high), given both this diff and quirks#5215's diff: no findings.
| if attribute_converter is not None: | ||
| self._attribute_converter = attribute_converter | ||
| if value_converter is not None: | ||
| self._value_converter = value_converter |
There was a problem hiding this comment.
Please mirror quirks#5215's both-or-neither validation here.
NumberMetadata.__attrs_post_init__ raises ValueError when only one converter is given, but that guards only the quirks v2 entry point. This class is constructible directly, and a subclass can set _attribute_converter at class level without _value_converter. In either case native_value returns a converted value while async_set_native_value writes an unconverted one, so the round trip silently disagrees with itself.
The asymmetry is a property of this class's behavior, so the invariant reads better here — raise in __init__ when exactly one of the two ends up set, class-level defaults included — than only in the metadata layer.
| @@ -278,6 +283,10 @@ def __init__( | |||
| """Init this number configuration entity.""" | |||
There was a problem hiding this comment.
Please document the converters here: which direction each one goes, that they take precedence over multiplier, and that both are required together.
QuirkBuilder.number()'s docstring in quirks#5215 says all of this, but this is the class a reader lands on when they follow _platform_kwargs, and the parameter names alone don't carry direction.
| self._cluster, {self._attribute_name: int(value / self._multiplier)} | ||
| ) | ||
| if self._value_converter: | ||
| attr_value = self._value_converter(value) |
There was a problem hiding this comment.
Worth documenting here: the default branch below forces int(value / self._multiplier), but the converter branch writes whatever the converter returns — and HA always hands async_set_native_value a float.
So a converter written the obvious way, lambda x: 5 - x rather than lambda x: 5 - int(x), produces a float, and zigpy truncates it silently rather than rejecting it (t.uint8_t(3.5) → 3, checked against zigpy 2.1.0). quirks#5215's prose does tell authors to cast, but nothing on either side enforces or warns.
A blanket int() here would be wrong — Single/Double attributes are legitimate — so the docstring is probably the right place. Just don't leave the warning only in the quirks-side docs.
| *, | ||
| attribute_name: str | None = None, | ||
| attribute_converter: Callable[[Any], Any] | None = None, | ||
| value_converter: Callable[[Any], Any] | None = None, |
There was a problem hiding this comment.
Nit: value_converter only ever receives the HA-side float from async_set_native_value, so Callable[[float], Any] would state the contract more precisely than Callable[[Any], Any]. (attribute_converter genuinely takes Any, so the asymmetry would be real rather than sloppy.) Low value — raising it only because this signature is what quirk authors read.
| update_attribute_cache(cluster) | ||
| zha_device = await join_zigpy_device(zha_gateway, light) | ||
|
|
||
| entity = NumberConfigurationEntity( |
There was a problem hiding this comment.
These tests construct the entity directly, which skips the plumbing the feature actually ships through: zhaquirks/builder/discovery.py::_platform_kwargs, i.e. exactly the lines quirks#5215 adds. Right now nothing in either repo covers that mapping — #5215's tests stop at NumberMetadata.
The precedent in this repo goes end to end: test_quirks_sensor_attr_converter (tests/test_sensor.py:1911) and test_quirks_binary_sensor_attr_converter (tests/test_binary_sensor.py:259) both build through QuirkBuilder(...).sensor(attribute_converter=...) → registry.resolve → join_zigpy_device → get_entity.
I don't think that's achievable in this PR — QuirkBuilder.number() won't accept the kwargs until #5215 ships, and #5215 needs this released first — so keeping the direct-construction tests here is reasonable. Please add the QuirkBuilder-based test once the ordering allows (a zha follow-up, or in #5215 after the zha release), so the discovery mapping doesn't stay permanently untested.
2 participants
DRAFT.
Q1: Can/should we even move quirks-initialized entities to zha-quirks itself?
Q2: Better naming?
Proposed change
Number entities are writable, so unlike sensors and binary sensors they need converters in both directions: attribute_converter maps the raw attribute value to the value shown in HA, value_converter maps the value from HA back to the raw attribute value.
As for sensors, a converter takes precedence over the multiplier.
Additional information
Linked quirks PR:
attribute_converter/value_convertertoQuirkBuilder.number()zha-device-handlers#5215We need this for: