Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow device cluster entities overwrite from v2 quirks #328

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from
Draft

Allow device cluster entities overwrite from v2 quirks #328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b424762
Allow device cluster entities overwrite from v2 quirks.
mrrstux Dec 15, 2024
1c63350
Fix tests.
mrrstux Dec 19, 2024
b9af6d2
Transition all entities to new unique id scheme
mrrstux Jan 28, 2025
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
68 changes: 68 additions & 0 deletions tests/test_climate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import zhaquirks.tuya.ts0601_trv
import zigpy.profiles
import zigpy.quirks
from zigpy.quirks.v2 import QuirkBuilder
import zigpy.zcl.clusters
from zigpy.zcl.clusters.hvac import Thermostat
import zigpy.zcl.foundation as zcl_f
Expand Down Expand Up @@ -44,6 +45,7 @@
Thermostat as ThermostatEntity,
)
from zha.application.platforms.climate.const import FanState
from zha.application.platforms.number import NumberConfigurationEntity
from zha.application.platforms.sensor import (
Sensor,
SinopeHVACAction,
Expand Down Expand Up @@ -179,6 +181,7 @@
"abs_max_cool_setpoint_limit": 4000,
"ctrl_sequence_of_oper": Thermostat.ControlSequenceOfOperation.Cooling_and_Heating,
"local_temperature": None,
"local_temperature_calibration": 0,
"max_cool_setpoint_limit": 3900,
"max_heat_setpoint_limit": 2900,
"min_cool_setpoint_limit": 2100,
Expand Down Expand Up @@ -1463,3 +1466,68 @@ async def test_set_zonnsmart_operation_mode(zha_gateway: Gateway) -> None:
await send_attributes_report(zha_gateway, thrm_cluster, {"operation_preset": 4})

assert entity.state[ATTR_PRESET_MODE] == "frost protect"


async def test_thermostat_default_local_temperature_calibration_config(
zha_gateway: Gateway,
) -> None:
"""Test that a default local temperature calibration config gets attached to a thermostat entity."""

zha_device = await device_climate_mock(zha_gateway, CLIMATE)

assert zha_device.model == "FakeModel"
assert zha_device.manufacturer == "unk_manufacturer"

local_temperature_calibration_entity = zha_device.platform_entities[
(Platform.NUMBER, "00:0d:6f:00:0a:90:69:e7-1-513-local_temperature_calibration")
]
assert local_temperature_calibration_entity
assert isinstance(local_temperature_calibration_entity, NumberConfigurationEntity)
assert local_temperature_calibration_entity.info_object.min_value == -2.5
assert local_temperature_calibration_entity.info_object.max_value == 2.5
assert local_temperature_calibration_entity.info_object.step == 0.1
assert local_temperature_calibration_entity.info_object.multiplier == 0.1


async def test_thermostat_quirkv2_local_temperature_calibration_config_overwrite(
zha_gateway: Gateway,
) -> None:
"""Test that a quirk v2 local temperature calibration config overwrites the default one."""

zigpy_device = create_mock_zigpy_device(
zha_gateway, CLIMATE, manufacturer="unk_manufacturer", model="FakeModel"
)
zigpy_device.node_desc.mac_capability_flags |= 0b_0000_0100
zigpy_device.endpoints[1].thermostat.PLUGGED_ATTR_READS = ZCL_ATTR_PLUG

(
QuirkBuilder("unk_manufacturer", "FakeModel", zigpy.quirks._DEVICE_REGISTRY)
# Local temperature calibration.
.number(
Thermostat.AttributeDefs.local_temperature_calibration.name,
zigpy.zcl.clusters.hvac.Thermostat.cluster_id,
min_value=-5,
max_value=5,
step=0.1,
multiplier=0.1,
translation_key="local_temperature_calibration",
fallback_name="Local temperature offset",
)
.add_to_registry()
)

zigpy_device = zigpy.quirks._DEVICE_REGISTRY.get_device(zigpy_device)
zha_device = await join_zigpy_device(zha_gateway, zigpy_device)

assert zha_device.model == "FakeModel"
assert zha_device.manufacturer == "unk_manufacturer"

local_temperature_calibration_entity = zha_device.platform_entities[
(Platform.NUMBER, "00:0d:6f:00:0a:90:69:e7-1-513-local_temperature_calibration")
]
assert local_temperature_calibration_entity
assert isinstance(local_temperature_calibration_entity, NumberConfigurationEntity)
assert local_temperature_calibration_entity.info_object.min_value == -5.0
assert local_temperature_calibration_entity.info_object.max_value == 5.0
assert local_temperature_calibration_entity.info_object.step == 0.1
assert local_temperature_calibration_entity.info_object.multiplier == 0.1
2 changes: 2 additions & 0 deletions zha/application/const.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@

ENTITY_METADATA = "entity_metadata"

ENTITY_PREVIOUS_UNIQUE_ID = "previous_unique_id"

ZCL_INIT_ATTRS = "ZCL_INIT_ATTRS"
REPORT_CONFIG = "REPORT_CONFIG"

Expand Down
19 changes: 15 additions & 4 deletions zha/application/discovery.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,9 +335,10 @@ def discover_quirks_v2_entities(self, device: Device) -> None:
endpoint.async_new_entity(
platform=platform,
entity_class=entity_class,
unique_id=endpoint.unique_id,
unique_id=f"{endpoint.unique_id}-{cluster.cluster_id}",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will break all existing v2 entities and it will require a migration of the HA entity registry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that migration a post-install operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In ZHA async_setup_entry I can get access to all HA ZHA entities. But it doesn't look like the HA associated entity data contains the info I need to migrate the ids - I would not know the cluster id of each entity in order to migrate to <unique id>-<cluster id>.

Here is how the registry entry looks like for a local temperature calibration entity:

'RegistryEntry(entity_id='number.thermostat_trv_local_temperature_offset', unique_id='28:fa:26:01:02:0b:67:2a-1-513-local_temperature_calibration', platform='zha', previous_unique_id=None, aliases=set(), area_id=None, categories={}, capabilities={'min': -5.0, 'max': 5.0, 'step': 0.1, 'mode': 'auto'}, config_entry_id='75fe69aebaacaaed3a012a97167d2d10', created_at=datetime.datetime(1970, 1, 1, 0, 0, tzinfo=datetime.timezone.utc), device_class=None, device_id='9e9268b18b27e97adb5d7543c1df8f62', disabled_by=None, entity_category=<EntityCategory.CONFIG: 'config'>, hidden_by=None, icon=None, id='617948e1ccca4e53075e64956998a17e', has_entity_name=True, labels=set(), modified_at=datetime.datetime(2024, 12, 16, 19, 51, 43, 399371, tzinfo=datetime.timezone.utc), name=None, options={'conversation': {'should_expose': False}}, original_device_class=None, original_icon=None, original_name='Local temperature offset', supported_features=0, translation_key='local_temperature_calibration', unit_of_measurement='°C', _cache={})'.

Is there any other place where the migration could be done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found a way: inside BaseEntityInfo, the previous unique id can be stored and used by ZHA to migrate existing HA entities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the migration code: home-assistant/core#136788.

cluster_handlers=[cluster_handler],
entity_metadata=entity_metadata,
previous_unique_id=endpoint.unique_id,
)

_LOGGER.debug(
Expand Down Expand Up @@ -432,8 +433,15 @@ def discover_by_device_type(self, endpoint: Endpoint) -> None:
if platform_entity_class is None:
return
endpoint.claim_cluster_handlers(claimed)

cluster_id = claimed[0].cluster.cluster_id

endpoint.async_new_entity(
platform, platform_entity_class, unique_id, claimed
platform,
platform_entity_class,
f"{endpoint.unique_id}-{cluster_id}",
claimed,
previous_unique_id=endpoint.unique_id,
)

def discover_by_cluster_id(self, endpoint: Endpoint) -> None:
Expand Down Expand Up @@ -561,17 +569,20 @@ def discover_multi_entities(
[ch.name for ch in entity_and_handler.claimed_cluster_handlers],
)

first_ch = entity_and_handler.claimed_cluster_handlers[0]

if platform == cmpt_by_dev_type:
# for well known device types,
# like thermostats we'll take only 1st class
endpoint.async_new_entity(
platform,
entity_and_handler.entity_class,
endpoint.unique_id,
f"{endpoint.unique_id}-{first_ch.cluster.cluster_id}",
entity_and_handler.claimed_cluster_handlers,
previous_unique_id=endpoint.unique_id,
)
break
first_ch = entity_and_handler.claimed_cluster_handlers[0]

endpoint.async_new_entity(
platform,
entity_and_handler.entity_class,
Expand Down
31 changes: 23 additions & 8 deletions zha/application/platforms/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from zigpy.types.named import EUI64

from zha.application import Platform
from zha.application.const import ENTITY_PREVIOUS_UNIQUE_ID
from zha.const import STATE_CHANGED
from zha.debounce import Debouncer
from zha.event import EventBase
Expand Down Expand Up @@ -50,6 +51,7 @@ class BaseEntityInfo:

fallback_name: str
unique_id: str
previous_unique_id: str | None
platform: str
class_name: str
translation_key: str | None
Expand Down Expand Up @@ -118,11 +120,12 @@ class BaseEntity(LogMixin, EventBase):
_attr_state_class: str | None
_attr_enabled: bool = True

def __init__(self, unique_id: str) -> None:
def __init__(self, unique_id: str, previous_unique_id: str | None = None) -> None:
"""Initialize the platform entity."""
super().__init__()

self._unique_id: str = unique_id
self._previous_unique_id: str | None = previous_unique_id

self.__previous_state: Any = None
self._tracked_tasks: list[asyncio.Task] = []
Expand Down Expand Up @@ -189,6 +192,12 @@ def unique_id(self) -> str:
"""Return the unique id."""
return self._unique_id

@final
@property
def previous_unique_id(self) -> str | None:
"""Return the previous unique id, if any."""
return self._previous_unique_id

@cached_property
def identifiers(self) -> BaseIdentifiers:
"""Return a dict with the information necessary to identify this entity."""
Expand All @@ -203,6 +212,7 @@ def info_object(self) -> BaseEntityInfo:

return BaseEntityInfo(
unique_id=self.unique_id,
previous_unique_id=self.previous_unique_id,
platform=self.PLATFORM,
class_name=self.__class__.__name__,
fallback_name=self.fallback_name,
Expand Down Expand Up @@ -299,6 +309,12 @@ def __init__(
if self._unique_id_suffix:
unique_id += f"-{self._unique_id_suffix}"

if ENTITY_PREVIOUS_UNIQUE_ID in kwargs:
previous_unique_id = kwargs[ENTITY_PREVIOUS_UNIQUE_ID]
if previous_unique_id is not None:
previous_unique_id += f"-{self._unique_id_suffix}"
kwargs[ENTITY_PREVIOUS_UNIQUE_ID] = previous_unique_id

# XXX: The ordering here matters: `_init_from_quirks_metadata` affects how
# the `unique_id` is computed!
super().__init__(unique_id=unique_id, **kwargs)
Expand All @@ -309,16 +325,15 @@ def __init__(
self.cluster_handlers[cluster_handler.name] = cluster_handler
self._device: Device = device
self._endpoint = endpoint
# we double create these in discovery tests because we reissue the create calls to count and prove them out
if (self.PLATFORM, self.unique_id) not in self._device.platform_entities:
self._device.platform_entities[(self.PLATFORM, self.unique_id)] = self
else:
_LOGGER.debug(
"Not registering entity %r, unique id %r already exists: %r",
self,

if (self.PLATFORM, self.unique_id) in self._device.platform_entities:
_LOGGER.warning(
"Duplicate entity detected - unique id %r already exists: %r. Replacing with %r",
(self.PLATFORM, self.unique_id),
self._device.platform_entities[(self.PLATFORM, self.unique_id)],
self,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intention was to offer a debugging message for those cases where it is not intended or expected to encounter duplicated entities. Can be removed or demoted to debug.

)
self._device.platform_entities[(self.PLATFORM, self.unique_id)] = self
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is allowing a newer entity instance to overwrite an existing one. In my specific case, an automatic zha created local temp calibration entity would be replaced by one provided by a quirk v2. At least, in my setup, the quirk v2 entity gets created last.


@classmethod
def create_platform_entity(
Expand Down
Loading