Skip to content

Commit ad60d62

Browse files
authored
Fix not claiming cluster handler for some quirks v2 entities (#548)
* Add test to check for proper implementation * WIP: first basic implementation * Rework implementation * Change test to use existing attribute * Test command button doesn't claim cluster handler unnecessarily * Remove comment about write_attr_button causing claimage * Temporarily add comment about "write attr button" claiming cluster handler * Change comment to emphasize that `BIND = True` by default already * Adjust three device tests to represent v2 entities initializing chs * Change `command_kwargs` in new and existing test * Change `FakeManufacturerCluster` to `OppleCluster` * Revert change in unrelated test, as more of it needs to be changed * Add back removed comment in test
1 parent d0870fa commit ad60d62

File tree

6 files changed

+198
-16
lines changed

6 files changed

+198
-16
lines changed

tests/data/devices/bosch-rbsh-mms-zb-eu.json

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -841,7 +841,7 @@
841841
},
842842
"id": "1:0xfca0",
843843
"unique_id": "ab:cd:ef:12:75:e4:96:a5:1:0xfca0",
844-
"status": "CREATED",
844+
"status": "INITIALIZED",
845845
"value_attribute": null
846846
}
847847
],
@@ -888,7 +888,7 @@
888888
},
889889
"id": "1:0xfca0",
890890
"unique_id": "ab:cd:ef:12:75:e4:96:a5:1:0xfca0",
891-
"status": "CREATED",
891+
"status": "INITIALIZED",
892892
"value_attribute": null
893893
}
894894
],
@@ -935,7 +935,7 @@
935935
},
936936
"id": "1:0xfca0",
937937
"unique_id": "ab:cd:ef:12:75:e4:96:a5:1:0xfca0",
938-
"status": "CREATED",
938+
"status": "INITIALIZED",
939939
"value_attribute": null
940940
}
941941
],
@@ -982,7 +982,7 @@
982982
},
983983
"id": "1:0xfca0",
984984
"unique_id": "ab:cd:ef:12:75:e4:96:a5:1:0xfca0",
985-
"status": "CREATED",
985+
"status": "INITIALIZED",
986986
"value_attribute": null
987987
}
988988
],
@@ -1031,7 +1031,7 @@
10311031
},
10321032
"id": "1:0xfca0",
10331033
"unique_id": "ab:cd:ef:12:75:e4:96:a5:1:0xfca0",
1034-
"status": "CREATED",
1034+
"status": "INITIALIZED",
10351035
"value_attribute": null
10361036
}
10371037
],
@@ -1079,7 +1079,7 @@
10791079
},
10801080
"id": "1:0xfca0",
10811081
"unique_id": "ab:cd:ef:12:75:e4:96:a5:1:0xfca0",
1082-
"status": "CREATED",
1082+
"status": "INITIALIZED",
10831083
"value_attribute": null
10841084
}
10851085
],
@@ -1513,7 +1513,7 @@
15131513
},
15141514
"id": "1:0xfca0",
15151515
"unique_id": "ab:cd:ef:12:75:e4:96:a5:1:0xfca0",
1516-
"status": "CREATED",
1516+
"status": "INITIALIZED",
15171517
"value_attribute": null
15181518
}
15191519
],
@@ -1607,7 +1607,7 @@
16071607
},
16081608
"id": "1:0xfca0",
16091609
"unique_id": "ab:cd:ef:12:75:e4:96:a5:1:0xfca0",
1610-
"status": "CREATED",
1610+
"status": "INITIALIZED",
16111611
"value_attribute": null
16121612
}
16131613
],
@@ -1655,7 +1655,7 @@
16551655
},
16561656
"id": "2:0xfca0",
16571657
"unique_id": "ab:cd:ef:12:75:e4:96:a5:2:0xfca0",
1658-
"status": "CREATED",
1658+
"status": "INITIALIZED",
16591659
"value_attribute": null
16601660
}
16611661
],
@@ -1703,7 +1703,7 @@
17031703
},
17041704
"id": "3:0xfca0",
17051705
"unique_id": "ab:cd:ef:12:75:e4:96:a5:3:0xfca0",
1706-
"status": "CREATED",
1706+
"status": "INITIALIZED",
17071707
"value_attribute": null
17081708
}
17091709
],

tests/data/devices/ikea-of-sweden-vallhorn-wireless-motion-sensor.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,7 @@
467467
},
468468
"id": "1:0xfc81",
469469
"unique_id": "8c:6f:b9:ff:fe:26:3b:4a:1:0xfc81",
470-
"status": "CREATED",
470+
"status": "INITIALIZED",
471471
"value_attribute": null
472472
}
473473
],
@@ -702,7 +702,7 @@
702702
},
703703
"id": "1:0xfc81",
704704
"unique_id": "8c:6f:b9:ff:fe:26:3b:4a:1:0xfc81",
705-
"status": "CREATED",
705+
"status": "INITIALIZED",
706706
"value_attribute": null
707707
}
708708
],

tests/data/devices/third-reality-inc-3rsp02028bz.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -426,7 +426,7 @@
426426
},
427427
"id": "1:0xff03",
428428
"unique_id": "28:2c:02:bf:ff:eb:4f:2f:1:0xff03",
429-
"status": "CREATED",
429+
"status": "INITIALIZED",
430430
"value_attribute": null
431431
}
432432
],

tests/test_button.py

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
PROFILE_ID,
1313
)
1414
from zhaquirks.tuya.tuya_valve import ParksideTuyaValveManufCluster
15+
import zigpy
1516
from zigpy.exceptions import ZigbeeException
1617
from zigpy.profiles import zha
1718
from zigpy.quirks import CustomCluster, CustomDevice, DeviceRegistry
@@ -34,6 +35,7 @@
3435
update_attribute_cache,
3536
)
3637
from zha.application import Platform
38+
from zha.application.const import ZCL_INIT_ATTRS
3739
from zha.application.gateway import Gateway
3840
from zha.application.platforms import EntityCategory, PlatformEntity
3941
from zha.application.platforms.button import (
@@ -43,6 +45,7 @@
4345
)
4446
from zha.application.platforms.button.const import ButtonDeviceClass
4547
from zha.exceptions import ZHAException
48+
from zha.zigbee.cluster_handlers.manufacturerspecific import OppleRemoteClusterHandler
4649
from zha.zigbee.device import Device
4750

4851
ZIGPY_DEVICE = {
@@ -354,3 +357,89 @@ async def test_quirks_write_attr_buttons_uid(zha_gateway: Gateway) -> None:
354357
assert entity_btn_2.translation_key == "btn_2"
355358
assert entity_btn_2._unique_id_suffix == "btn_2"
356359
assert entity_btn_2._attribute_value == 2
360+
361+
362+
class OppleCluster(CustomCluster, ManufacturerSpecificCluster):
363+
"""Aqara manufacturer specific cluster."""
364+
365+
cluster_id = 0xFCC0
366+
ep_attribute = "opple_cluster"
367+
368+
class ServerCommandDefs(zcl_f.BaseCommandDefs):
369+
"""Server command definitions."""
370+
371+
self_test: Final = zcl_f.ZCLCommandDef(
372+
id=0x00, schema={"identify_time": t.uint16_t}
373+
)
374+
375+
376+
async def test_cluster_handler_quirks_unnecessary_claiming(
377+
zha_gateway: Gateway,
378+
) -> None:
379+
"""Test quirks button doesn't claim cluster handlers unnecessarily."""
380+
381+
registry = DeviceRegistry()
382+
(
383+
QuirkBuilder(
384+
"Fake_Manufacturer_sensor_2", "Fake_Model_sensor_2", registry=registry
385+
)
386+
.replaces(OppleCluster)
387+
.command_button(
388+
OppleCluster.ServerCommandDefs.self_test.name,
389+
OppleCluster.cluster_id,
390+
command_kwargs={"identify_time": 5},
391+
translation_key="self_test",
392+
fallback_name="Self test",
393+
)
394+
.add_to_registry()
395+
)
396+
397+
zigpy_device = create_mock_zigpy_device(
398+
zha_gateway,
399+
{
400+
1: {
401+
SIG_EP_INPUT: [
402+
general.Basic.cluster_id,
403+
OppleCluster.cluster_id,
404+
],
405+
SIG_EP_OUTPUT: [],
406+
SIG_EP_TYPE: zigpy.profiles.zha.DeviceType.OCCUPANCY_SENSOR,
407+
SIG_EP_PROFILE: zigpy.profiles.zha.PROFILE_ID,
408+
}
409+
},
410+
manufacturer="Fake_Manufacturer_sensor_2",
411+
model="Fake_Model_sensor_2",
412+
)
413+
zigpy_device = registry.get_device(zigpy_device)
414+
415+
# Suppress normal endpoint probing, as this will claim the Opple cluster handler
416+
# already due to it being in the "CLUSTER_HANDLER_ONLY_CLUSTERS" registry.
417+
# We want to test the handler also gets claimed via quirks v2 attributes init.
418+
with patch("zha.application.discovery.EndpointProbe.discover_entities"):
419+
zha_device = await join_zigpy_device(zha_gateway, zigpy_device)
420+
assert isinstance(zha_device.device, CustomDeviceV2)
421+
422+
# get cluster handler of OppleCluster
423+
opple_ch = zha_device.endpoints[1].all_cluster_handlers["1:0xfcc0"]
424+
assert isinstance(opple_ch, OppleRemoteClusterHandler)
425+
426+
# make sure the cluster handler was not claimed,
427+
# as no reporting is configured and no attributes are to be read
428+
assert opple_ch not in zha_device.endpoints[1].claimed_cluster_handlers.values()
429+
430+
# check that BIND is left at default of True, though ZHA will ignore it
431+
assert opple_ch.BIND is True
432+
433+
# check ZCL_INIT_ATTRS is empty
434+
assert opple_ch.ZCL_INIT_ATTRS == {}
435+
436+
# check that no ZCL_INIT_ATTRS instance variable was created
437+
assert opple_ch.__dict__.get(ZCL_INIT_ATTRS) is None
438+
assert opple_ch.ZCL_INIT_ATTRS is OppleRemoteClusterHandler.ZCL_INIT_ATTRS
439+
440+
# double check we didn't modify the class variable
441+
assert OppleRemoteClusterHandler.ZCL_INIT_ATTRS == {}
442+
443+
# check if REPORT_CONFIG is empty, both instance and class variable
444+
assert opple_ch.REPORT_CONFIG == ()
445+
assert OppleRemoteClusterHandler.REPORT_CONFIG == ()

tests/test_sensor.py

Lines changed: 74 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1598,7 +1598,7 @@ async def test_state_class(
15981598
assert "Quirks provided an invalid state class: energy" in caplog.text
15991599

16001600

1601-
async def test_cluster_handler_quirks_attributes(zha_gateway: Gateway) -> None:
1601+
async def test_cluster_handler_quirks_attribute_reporting(zha_gateway: Gateway) -> None:
16021602
"""Test quirks sensor setting up ZCL_INIT_ATTRS and REPORT_CONFIG correctly."""
16031603

16041604
# Suppress normal endpoint probing, as this will claim the Opple cluster handler
@@ -1615,6 +1615,9 @@ async def test_cluster_handler_quirks_attributes(zha_gateway: Gateway) -> None:
16151615
# make sure the cluster handler was claimed due to reporting config, so ZHA binds it
16161616
assert opple_ch in zha_device.endpoints[1].claimed_cluster_handlers.values()
16171617

1618+
# check that BIND is not set to False, as reporting is configured
1619+
assert opple_ch.BIND is True
1620+
16181621
# check ZCL_INIT_ATTRS contains sensor attributes that are not in REPORT_CONFIG
16191622
assert opple_ch.ZCL_INIT_ATTRS == {
16201623
"energy": True,
@@ -1645,6 +1648,76 @@ async def test_cluster_handler_quirks_attributes(zha_gateway: Gateway) -> None:
16451648
assert OppleRemoteClusterHandler.REPORT_CONFIG == ()
16461649

16471650

1651+
async def test_cluster_handler_quirks_attribute_reading(zha_gateway: Gateway) -> None:
1652+
"""Test quirks sensor setting up ZCL_INIT_ATTRS, claiming cluster handler."""
1653+
1654+
registry = DeviceRegistry()
1655+
(
1656+
QuirkBuilder(
1657+
"Fake_Manufacturer_sensor_2", "Fake_Model_sensor_2", registry=registry
1658+
)
1659+
.replaces(OppleCluster)
1660+
.sensor(
1661+
"last_feeding_size",
1662+
OppleCluster.cluster_id,
1663+
translation_key="last_feeding_size",
1664+
fallback_name="Last feeding size",
1665+
)
1666+
.add_to_registry()
1667+
)
1668+
1669+
zigpy_device = create_mock_zigpy_device(
1670+
zha_gateway,
1671+
{
1672+
1: {
1673+
SIG_EP_INPUT: [
1674+
general.Basic.cluster_id,
1675+
OppleCluster.cluster_id,
1676+
],
1677+
SIG_EP_OUTPUT: [],
1678+
SIG_EP_TYPE: zigpy.profiles.zha.DeviceType.OCCUPANCY_SENSOR,
1679+
SIG_EP_PROFILE: zigpy.profiles.zha.PROFILE_ID,
1680+
}
1681+
},
1682+
manufacturer="Fake_Manufacturer_sensor_2",
1683+
model="Fake_Model_sensor_2",
1684+
)
1685+
zigpy_device = registry.get_device(zigpy_device)
1686+
1687+
# Suppress normal endpoint probing, as this will claim the Opple cluster handler
1688+
# already due to it being in the "CLUSTER_HANDLER_ONLY_CLUSTERS" registry.
1689+
# We want to test the handler also gets claimed via quirks v2 attributes init.
1690+
with patch("zha.application.discovery.EndpointProbe.discover_entities"):
1691+
zha_device = await join_zigpy_device(zha_gateway, zigpy_device)
1692+
assert isinstance(zha_device.device, CustomDeviceV2)
1693+
1694+
# get cluster handler of OppleCluster
1695+
opple_ch = zha_device.endpoints[1].all_cluster_handlers["1:0xfcc0"]
1696+
assert isinstance(opple_ch, OppleRemoteClusterHandler)
1697+
1698+
# make sure the cluster handler was claimed due to attributes to be initialized
1699+
# otherwise, ZHA won't configure the cluster handler, so attributes are not read
1700+
assert opple_ch in zha_device.endpoints[1].claimed_cluster_handlers.values()
1701+
1702+
# check that BIND is set to False, as no reporting is configured
1703+
assert opple_ch.BIND is False
1704+
1705+
# check ZCL_INIT_ATTRS contains sensor attributes that are not in REPORT_CONFIG
1706+
assert opple_ch.ZCL_INIT_ATTRS == {
1707+
"last_feeding_size": True,
1708+
}
1709+
# check that ZCL_INIT_ATTRS is an instance variable and not a class variable now
1710+
assert opple_ch.ZCL_INIT_ATTRS is opple_ch.__dict__[ZCL_INIT_ATTRS]
1711+
assert opple_ch.ZCL_INIT_ATTRS is not OppleRemoteClusterHandler.ZCL_INIT_ATTRS
1712+
1713+
# double check we didn't modify the class variable
1714+
assert OppleRemoteClusterHandler.ZCL_INIT_ATTRS == {}
1715+
1716+
# check if REPORT_CONFIG is empty, both instance and class variable
1717+
assert opple_ch.REPORT_CONFIG == ()
1718+
assert OppleRemoteClusterHandler.REPORT_CONFIG == ()
1719+
1720+
16481721
async def test_device_counter_sensors(zha_gateway: Gateway) -> None:
16491722
"""Test coordinator counter sensor."""
16501723

zha/application/discovery.py

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,10 @@ def discover_quirks_v2_entities(self, device: Device) -> Iterator[PlatformEntity
232232

233233
assert cluster_handler
234234

235+
# flags to determine if we need to claim/bind the cluster handler
236+
attribute_initialization_found: bool = False
237+
reporting_found: bool = False
238+
235239
for entity_metadata in entity_metadata_list:
236240
platform = Platform(entity_metadata.entity_platform.value)
237241
metadata_type = type(entity_metadata)
@@ -254,6 +258,7 @@ def discover_quirks_v2_entities(self, device: Device) -> Iterator[PlatformEntity
254258

255259
# process the entity metadata for ZCL_INIT_ATTRS and REPORT_CONFIG
256260
if attr_name := getattr(entity_metadata, "attribute_name", None):
261+
# TODO: ignore "attribute write buttons"? currently, we claim ch
257262
# if the entity has a reporting config, add it to the cluster handler
258263
if rep_conf := getattr(entity_metadata, "reporting_config", None):
259264
# if attr is already in REPORT_CONFIG, remove it first
@@ -268,8 +273,8 @@ def discover_quirks_v2_entities(self, device: Device) -> Iterator[PlatformEntity
268273
cluster_handler.REPORT_CONFIG += (
269274
AttrReportConfig(attr=attr_name, config=astuple(rep_conf)),
270275
)
271-
# claim the cluster handler, so ZHA configures and binds it
272-
endpoint.claim_cluster_handlers([cluster_handler])
276+
# mark cluster handler for claiming and binding later
277+
reporting_found = True
273278

274279
# not in REPORT_CONFIG, add to ZCL_INIT_ATTRS if it not already in
275280
elif attr_name not in cluster_handler.ZCL_INIT_ATTRS:
@@ -283,6 +288,8 @@ def discover_quirks_v2_entities(self, device: Device) -> Iterator[PlatformEntity
283288
cluster_handler.ZCL_INIT_ATTRS[attr_name] = (
284289
entity_metadata.attribute_initialized_from_cache
285290
)
291+
# mark cluster handler for claiming later, but not binding
292+
attribute_initialization_found = True
286293

287294
yield entity_class(
288295
cluster_handlers=[cluster_handler],
@@ -299,6 +306,19 @@ def discover_quirks_v2_entities(self, device: Device) -> Iterator[PlatformEntity
299306
[cluster_handler.name],
300307
)
301308

309+
# if the cluster handler is unclaimed, claim it and set BIND accordingly,
310+
# so ZHA configures the cluster handler: reporting + reads attributes
311+
if (attribute_initialization_found or reporting_found) and (
312+
cluster_handler not in endpoint.claimed_cluster_handlers.values()
313+
):
314+
endpoint.claim_cluster_handlers([cluster_handler])
315+
# BIND is True by default, so only set to False if no reporting found.
316+
# We can safely do this, since quirks v2 entities are initialized last,
317+
# so if the cluster handler wasn't claimed by EndpointProbe so far,
318+
# only v2 entities need it.
319+
if not reporting_found:
320+
cluster_handler.BIND = False
321+
302322
@ignore_exceptions_during_iteration
303323
def discover_coordinator_device_entities(
304324
self, device: Device

0 commit comments

Comments
 (0)