Skip to content
Draft
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
96 changes: 96 additions & 0 deletions tests/test_number.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from zha.application import Platform
from zha.application.gateway import Gateway
from zha.application.platforms import EntityCategory, PlatformEntity
from zha.application.platforms.number import NumberConfigurationEntity
from zha.application.platforms.number.const import NumberMode

ZIGPY_ANALOG_OUTPUT_DEVICE = {
Expand Down Expand Up @@ -430,3 +431,98 @@ async def test_color_number(
)
await zha_gateway.async_block_till_done()
assert entity.state["state"] == initial_value


@pytest.mark.parametrize(
(
"attribute_converter",
"value_converter",
"initial_value",
"expected_native",
"set_native",
"expected_written",
),
(
# invert a 0..5 scale where 0 is the maximum (Schneider outlet brightness)
(lambda x: 5 - x, lambda x: 5 - int(x), 1, 4, 1, 4),
# non-symmetric converters are allowed: each direction is independent
(lambda x: x + 100, lambda x: int(x) - 100, 5, 105, 110, 10),
),
)
async def test_number_attribute_converter(
zha_gateway: Gateway,
attribute_converter,
value_converter,
initial_value: int,
expected_native: int,
set_native: int,
expected_written: int,
) -> None:
"""Test number entity attribute_converter/value_converter round trip."""

light = await light_mock(zha_gateway)
cluster = light.endpoints[1].on_off
cluster.PLUGGED_ATTR_READS = {"on_time": initial_value}
update_attribute_cache(cluster)
zha_device = await join_zigpy_device(zha_gateway, light)

entity = NumberConfigurationEntity(

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.

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.resolvejoin_zigpy_deviceget_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.

endpoint=zha_device.endpoints[1],
device=zha_device,
cluster=cluster,
from_quirk=True,
fallback_name="Test number",
translation_key="test_number",
attribute_name="on_time",
attribute_converter=attribute_converter,
value_converter=value_converter,
min_value=0,
max_value=255,
)

# the raw attribute value is converted for display
assert entity.native_value == expected_native
assert entity.state["state"] == expected_native

# the value from HA is converted back before it is written
cluster.write_attributes.reset_mock()
await entity.async_set_native_value(set_native)
assert cluster.write_attributes.mock_calls == [
call({"on_time": expected_written}, manufacturer=UNDEFINED),
]


async def test_number_attribute_converter_ignores_multiplier(
zha_gateway: Gateway,
) -> None:
"""Test that converters take precedence over the multiplier, as for sensors."""

light = await light_mock(zha_gateway)
cluster = light.endpoints[1].on_off
cluster.PLUGGED_ATTR_READS = {"on_time": 10}
update_attribute_cache(cluster)
zha_device = await join_zigpy_device(zha_gateway, light)

entity = NumberConfigurationEntity(
endpoint=zha_device.endpoints[1],
device=zha_device,
cluster=cluster,
from_quirk=True,
fallback_name="Test number",
translation_key="test_number",
attribute_name="on_time",
attribute_converter=lambda x: 5 - x,
value_converter=lambda x: 5 - int(x),
multiplier=0.1,
min_value=0,
max_value=255,
)

# 10 * 0.1 would be 1.0 if the multiplier applied, but the converter wins
assert entity.native_value == -5

cluster.write_attributes.reset_mock()
await entity.async_set_native_value(1)
assert cluster.write_attributes.mock_calls == [
call({"on_time": 4}, manufacturer=UNDEFINED),
]
19 changes: 16 additions & 3 deletions zha/application/platforms/number/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from __future__ import annotations

from abc import ABC, abstractmethod
from collections.abc import Callable
from dataclasses import dataclass
import functools
import logging
Expand Down Expand Up @@ -259,13 +260,17 @@ class NumberConfigurationEntity(BaseNumber):
_attr_native_step: float = 1.0
_multiplier: float = 1
_attribute_name: str
_attribute_converter: Callable[[Any], Any] | None = None
_value_converter: Callable[[Any], Any] | None = None

def __init__(
self,
endpoint: Endpoint,
device: Device,
*,
attribute_name: str | None = None,
attribute_converter: Callable[[Any], Any] | None = None,
value_converter: Callable[[Any], Any] | None = None,

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.

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.

min_value: float | None = None,
max_value: float | None = None,
step: float | None = None,
Expand All @@ -278,6 +283,10 @@ def __init__(
"""Init this number configuration entity."""

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.

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.

if attribute_name is not None:
self._attribute_name = attribute_name
if attribute_converter is not None:
self._attribute_converter = attribute_converter
if value_converter is not None:
self._value_converter = value_converter

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.

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.

if min_value is not None:
self._attr_native_min_value = min_value
if max_value is not None:
Expand Down Expand Up @@ -344,13 +353,17 @@ def native_value(self) -> float | None:
value = self._cluster.get(self._attribute_name)
if value is None:
return None
if self._attribute_converter:
return self._attribute_converter(value)
return value * self._multiplier

async def async_set_native_value(self, value: float) -> None:
"""Update the current value from HA."""
await write_attributes_safe(
self._cluster, {self._attribute_name: int(value / self._multiplier)}
)
if self._value_converter:
attr_value = self._value_converter(value)

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.

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.

else:
attr_value = int(value / self._multiplier)
await write_attributes_safe(self._cluster, {self._attribute_name: attr_value})
self.maybe_emit_state_changed_event()

async def async_update(self) -> None:
Expand Down
Loading