From 1dc9af09b4c54667fb261f3ef8c0eb8d75365b77 Mon Sep 17 00:00:00 2001 From: Stefan Agner Date: Tue, 18 Jun 2024 18:39:55 +0200 Subject: [PATCH 1/4] Convert to asyncio Matter SDK API Changes required to use the new co-routines based Matter SDK API. --- matter_server/server/device_controller.py | 98 +++++++++++++---------- matter_server/server/sdk.py | 35 +++----- 2 files changed, 65 insertions(+), 68 deletions(-) diff --git a/matter_server/server/device_controller.py b/matter_server/server/device_controller.py index c4cb34a8..8f8ab258 100644 --- a/matter_server/server/device_controller.py +++ b/matter_server/server/device_controller.py @@ -62,8 +62,6 @@ from collections.abc import Iterable from pathlib import Path - from chip.native import PyChipError - from .server import MatterServer DATA_KEY_NODES = "nodes" @@ -274,21 +272,28 @@ async def commission_with_code( attempts, MAX_COMMISSION_RETRIES, ) - result: ( - PyChipError | None - ) = await self._chip_device_controller.commission_with_code( - node_id, - code, - DiscoveryType.DISCOVERY_NETWORK_ONLY - if network_only - else DiscoveryType.DISCOVERY_ALL, - ) - if result and result.is_success: - break - if attempts >= MAX_COMMISSION_RETRIES: - raise NodeCommissionFailed( - f"Commission with code failed for node {node_id}." + try: + commissioned_node_id: int = ( + await self._chip_device_controller.commission_with_code( + node_id, + code, + DiscoveryType.DISCOVERY_NETWORK_ONLY + if network_only + else DiscoveryType.DISCOVERY_ALL, + ) ) + # We use SDK default behavior which always uses the commissioning Node ID in the + # generated NOC. So this should be the same really. + LOGGER.info( + "Commissioned Node ID: %s vs %s", commissioned_node_id, node_id + ) + assert commissioned_node_id == node_id + break + except ChipStackError as err: + if attempts >= MAX_COMMISSION_RETRIES: + raise NodeCommissionFailed( + f"Commission with code failed for node {node_id}." + ) from err await asyncio.sleep(5) LOGGER.info("Matter commissioning of Node ID %s successful.", node_id) @@ -346,33 +351,42 @@ async def commission_on_network( # by retrying, we increase the chances of a successful commission while attempts <= MAX_COMMISSION_RETRIES: attempts += 1 - result: PyChipError | None - if ip_addr is None: - # regular CommissionOnNetwork if no IP address provided - LOGGER.info( - "Starting Matter commissioning on network using Node ID %s (attempt %s/%s).", - node_id, - attempts, - MAX_COMMISSION_RETRIES, - ) - result = await self._chip_device_controller.commission_on_network( - node_id, setup_pin_code, filter_type, filter - ) - else: - LOGGER.info( - "Starting Matter commissioning using Node ID %s and IP %s (attempt %s/%s).", - node_id, - ip_addr, - attempts, - MAX_COMMISSION_RETRIES, - ) - result = await self._chip_device_controller.commission_ip( - node_id, setup_pin_code, ip_addr - ) - if result and result.is_success: + try: + if ip_addr is None: + # regular CommissionOnNetwork if no IP address provided + LOGGER.info( + "Starting Matter commissioning on network using Node ID %s (attempt %s/%s).", + node_id, + attempts, + MAX_COMMISSION_RETRIES, + ) + commissioned_node_id = ( + await self._chip_device_controller.commission_on_network( + node_id, setup_pin_code, filter_type, filter + ) + ) + else: + LOGGER.info( + "Starting Matter commissioning using Node ID %s and IP %s (attempt %s/%s).", + node_id, + ip_addr, + attempts, + MAX_COMMISSION_RETRIES, + ) + commissioned_node_id = ( + await self._chip_device_controller.commission_ip( + node_id, setup_pin_code, ip_addr + ) + ) + # We use SDK default behavior which always uses the commissioning Node ID in the + # generated NOC. So this should be the same really. + assert commissioned_node_id == node_id break - if attempts >= MAX_COMMISSION_RETRIES: - raise NodeCommissionFailed(f"Commissioning failed for node {node_id}.") + except ChipStackError as err: + if attempts >= MAX_COMMISSION_RETRIES: + raise NodeCommissionFailed( + f"Commissioning failed for node {node_id}." + ) from err await asyncio.sleep(5) LOGGER.info("Matter commissioning of Node ID %s successful.", node_id) diff --git a/matter_server/server/sdk.py b/matter_server/server/sdk.py index 6fcca266..2a5ed43e 100644 --- a/matter_server/server/sdk.py +++ b/matter_server/server/sdk.py @@ -8,7 +8,6 @@ from __future__ import annotations import asyncio -from concurrent.futures import ThreadPoolExecutor from functools import partial import logging import time @@ -25,6 +24,7 @@ if TYPE_CHECKING: from collections.abc import Callable + from concurrent.futures import ThreadPoolExecutor from pathlib import Path from chip.ChipDeviceCtrl import ( @@ -59,7 +59,6 @@ def __init__(self, server: MatterServer, paa_root_cert_dir: Path): self._node_lock: dict[int, asyncio.Lock] = {} self._subscriptions: dict[int, Attribute.SubscriptionTransaction] = {} - self._sdk_non_entrant_executor = ThreadPoolExecutor(max_workers=1) # Instantiate the underlying ChipDeviceController instance on the Fabric self._chip_controller = self.server.stack.fabric_admin.NewController( @@ -100,16 +99,6 @@ async def _call_sdk( ) -> _T: return await self._call_sdk_executor(None, target, *args, **kwargs) - async def _call_sdk_non_reentrant( - self, - target: Callable[..., _T], - *args: Any, - **kwargs: Any, - ) -> _T: - return await self._call_sdk_executor( - self._sdk_non_entrant_executor, target, *args, **kwargs - ) - async def get_compressed_fabric_id(self) -> int: """Get the compressed fabric id.""" return await self._call_sdk(self._chip_controller.GetCompressedFabricId) @@ -128,10 +117,9 @@ async def commission_with_code( node_id: int, setup_payload: str, discovery_type: DiscoveryType, - ) -> PyChipError: + ) -> int: """Commission a device using a QR Code or Manual Pairing Code.""" - return await self._call_sdk_non_reentrant( - self._chip_controller.CommissionWithCode, + return await self._chip_controller.CommissionWithCode( setupPayload=setup_payload, nodeid=node_id, discoveryType=discovery_type, @@ -143,10 +131,9 @@ async def commission_on_network( setup_pin_code: int, disc_filter_type: FilterType = FilterType.NONE, disc_filter: Any = None, - ) -> PyChipError: + ) -> int: """Commission a device on the network.""" - return await self._call_sdk_non_reentrant( - self._chip_controller.CommissionOnNetwork, + return await self._chip_controller.CommissionOnNetwork( nodeId=node_id, setupPinCode=setup_pin_code, filterType=disc_filter_type, @@ -155,10 +142,9 @@ async def commission_on_network( async def commission_ip( self, node_id: int, setup_pin_code: int, ip_addr: str - ) -> PyChipError: + ) -> int: """Commission a device using an IP address.""" - return await self._call_sdk_non_reentrant( - self._chip_controller.CommissionIP, + return await self._chip_controller.CommissionIP( nodeid=node_id, setupPinCode=setup_pin_code, ipaddr=ip_addr, @@ -185,9 +171,7 @@ async def unpair_device(self, node_id: int) -> PyChipError: Tries to look up the device attached to our controller with the given remote node id and ask it to remove Fabric. """ - return await self._call_sdk_non_reentrant( - self._chip_controller.UnpairDevice, nodeid=node_id - ) + return await self._chip_controller.UnpairDevice(nodeid=node_id) async def open_commissioning_window( self, @@ -199,8 +183,7 @@ async def open_commissioning_window( ) -> CommissioningParameters: """Open a commissioning window to commission a device present on this controller to another.""" async with self._get_node_lock(node_id): - return await self._call_sdk_non_reentrant( - self._chip_controller.OpenCommissioningWindow, + return await self._chip_controller.OpenCommissioningWindow( nodeid=node_id, timeout=timeout, iteration=iteration, From 1fcbb9fce2b9aa0bc581affa13c2a8910df66cbe Mon Sep 17 00:00:00 2001 From: Stefan Agner Date: Thu, 20 Jun 2024 19:09:02 +0200 Subject: [PATCH 2/4] Remove commissioning retries More often than not commissioning retries are not needed. In fact they lead to worse UX performance as it takes longer for an attempt which is bound to fail (e.g. because the code is wrong or the device is not actually commissionable) to fail. --- matter_server/server/device_controller.py | 127 +++++++++------------- 1 file changed, 50 insertions(+), 77 deletions(-) diff --git a/matter_server/server/device_controller.py b/matter_server/server/device_controller.py index 8f8ab258..bdb06fec 100644 --- a/matter_server/server/device_controller.py +++ b/matter_server/server/device_controller.py @@ -71,7 +71,6 @@ NODE_SUBSCRIPTION_CEILING_WIFI = 60 NODE_SUBSCRIPTION_CEILING_THREAD = 60 NODE_SUBSCRIPTION_CEILING_BATTERY_POWERED = 600 -MAX_COMMISSION_RETRIES = 3 NODE_RESUBSCRIBE_ATTEMPTS_UNAVAILABLE = 3 NODE_RESUBSCRIBE_TIMEOUT_OFFLINE = 30 * 60 * 1000 NODE_PING_TIMEOUT = 10 @@ -260,41 +259,29 @@ async def commission_with_code( """ node_id = self._get_next_node_id() - attempts = 0 - # we retry commissioning a few times as we've seen devices in the wild - # that are a bit unstable. - # by retrying, we increase the chances of a successful commission - while attempts <= MAX_COMMISSION_RETRIES: - attempts += 1 - LOGGER.info( - "Starting Matter commissioning with code using Node ID %s (attempt %s/%s).", - node_id, - attempts, - MAX_COMMISSION_RETRIES, - ) - try: - commissioned_node_id: int = ( - await self._chip_device_controller.commission_with_code( - node_id, - code, - DiscoveryType.DISCOVERY_NETWORK_ONLY - if network_only - else DiscoveryType.DISCOVERY_ALL, - ) - ) - # We use SDK default behavior which always uses the commissioning Node ID in the - # generated NOC. So this should be the same really. - LOGGER.info( - "Commissioned Node ID: %s vs %s", commissioned_node_id, node_id + LOGGER.info( + "Starting Matter commissioning with code using Node ID %s.", + node_id, + ) + try: + commissioned_node_id: int = ( + await self._chip_device_controller.commission_with_code( + node_id, + code, + DiscoveryType.DISCOVERY_NETWORK_ONLY + if network_only + else DiscoveryType.DISCOVERY_ALL, ) - assert commissioned_node_id == node_id - break - except ChipStackError as err: - if attempts >= MAX_COMMISSION_RETRIES: - raise NodeCommissionFailed( - f"Commission with code failed for node {node_id}." - ) from err - await asyncio.sleep(5) + ) + # We use SDK default behavior which always uses the commissioning Node ID in the + # generated NOC. So this should be the same really. + LOGGER.info("Commissioned Node ID: %s vs %s", commissioned_node_id, node_id) + if commissioned_node_id != node_id: + raise RuntimeError("Returned Node ID must match requested Node ID") + except ChipStackError as err: + raise NodeCommissionFailed( + f"Commission with code failed for node {node_id}." + ) from err LOGGER.info("Matter commissioning of Node ID %s successful.", node_id) @@ -345,49 +332,35 @@ async def commission_on_network( if ip_addr is not None: ip_addr = self.server.scope_ipv6_lla(ip_addr) - attempts = 0 - # we retry commissioning a few times as we've seen devices in the wild - # that are a bit unstable. - # by retrying, we increase the chances of a successful commission - while attempts <= MAX_COMMISSION_RETRIES: - attempts += 1 - try: - if ip_addr is None: - # regular CommissionOnNetwork if no IP address provided - LOGGER.info( - "Starting Matter commissioning on network using Node ID %s (attempt %s/%s).", - node_id, - attempts, - MAX_COMMISSION_RETRIES, - ) - commissioned_node_id = ( - await self._chip_device_controller.commission_on_network( - node_id, setup_pin_code, filter_type, filter - ) - ) - else: - LOGGER.info( - "Starting Matter commissioning using Node ID %s and IP %s (attempt %s/%s).", - node_id, - ip_addr, - attempts, - MAX_COMMISSION_RETRIES, - ) - commissioned_node_id = ( - await self._chip_device_controller.commission_ip( - node_id, setup_pin_code, ip_addr - ) + try: + if ip_addr is None: + # regular CommissionOnNetwork if no IP address provided + LOGGER.info( + "Starting Matter commissioning on network using Node ID %s.", + node_id, + ) + commissioned_node_id = ( + await self._chip_device_controller.commission_on_network( + node_id, setup_pin_code, filter_type, filter ) - # We use SDK default behavior which always uses the commissioning Node ID in the - # generated NOC. So this should be the same really. - assert commissioned_node_id == node_id - break - except ChipStackError as err: - if attempts >= MAX_COMMISSION_RETRIES: - raise NodeCommissionFailed( - f"Commissioning failed for node {node_id}." - ) from err - await asyncio.sleep(5) + ) + else: + LOGGER.info( + "Starting Matter commissioning using Node ID %s and IP %s.", + node_id, + ip_addr, + ) + commissioned_node_id = await self._chip_device_controller.commission_ip( + node_id, setup_pin_code, ip_addr + ) + # We use SDK default behavior which always uses the commissioning Node ID in the + # generated NOC. So this should be the same really. + if commissioned_node_id != node_id: + raise RuntimeError("Returned Node ID must match requested Node ID") + except ChipStackError as err: + raise NodeCommissionFailed( + f"Commissioning failed for node {node_id}." + ) from err LOGGER.info("Matter commissioning of Node ID %s successful.", node_id) From 3892260950f1a89b5f3b24b36af94919f88ac6d6 Mon Sep 17 00:00:00 2001 From: Stefan Agner Date: Thu, 20 Jun 2024 19:10:44 +0200 Subject: [PATCH 3/4] Update to Matter SDK wheels 2024.6.2 --- pyproject.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 3327ba7a..e59752e1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -20,7 +20,7 @@ dependencies = [ "async-timeout", "coloredlogs", "orjson", - "home-assistant-chip-clusters==2024.6.1", + "home-assistant-chip-clusters==2024.6.2", ] description = "Python Matter WebSocket Server" license = {text = "Apache-2.0"} @@ -39,7 +39,7 @@ server = [ "cryptography==42.0.8", "orjson==3.10.5", "zeroconf==0.132.2", - "home-assistant-chip-core==2024.6.1", + "home-assistant-chip-core==2024.6.2", ] test = [ "codespell==2.3.0", From babcc04c1536419a28e56d3f4150a0872c48f470 Mon Sep 17 00:00:00 2001 From: Stefan Agner Date: Thu, 20 Jun 2024 19:28:36 +0200 Subject: [PATCH 4/4] Explicitly cast the return values to avoid pylint errors --- matter_server/server/sdk.py | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/matter_server/server/sdk.py b/matter_server/server/sdk.py index 2a5ed43e..b55a8335 100644 --- a/matter_server/server/sdk.py +++ b/matter_server/server/sdk.py @@ -119,10 +119,13 @@ async def commission_with_code( discovery_type: DiscoveryType, ) -> int: """Commission a device using a QR Code or Manual Pairing Code.""" - return await self._chip_controller.CommissionWithCode( - setupPayload=setup_payload, - nodeid=node_id, - discoveryType=discovery_type, + return cast( + int, + await self._chip_controller.CommissionWithCode( + setupPayload=setup_payload, + nodeid=node_id, + discoveryType=discovery_type, + ), ) async def commission_on_network( @@ -133,21 +136,27 @@ async def commission_on_network( disc_filter: Any = None, ) -> int: """Commission a device on the network.""" - return await self._chip_controller.CommissionOnNetwork( - nodeId=node_id, - setupPinCode=setup_pin_code, - filterType=disc_filter_type, - filter=disc_filter, + return cast( + int, + await self._chip_controller.CommissionOnNetwork( + nodeId=node_id, + setupPinCode=setup_pin_code, + filterType=disc_filter_type, + filter=disc_filter, + ), ) async def commission_ip( self, node_id: int, setup_pin_code: int, ip_addr: str ) -> int: """Commission a device using an IP address.""" - return await self._chip_controller.CommissionIP( - nodeid=node_id, - setupPinCode=setup_pin_code, - ipaddr=ip_addr, + return cast( + int, + await self._chip_controller.CommissionIP( + nodeid=node_id, + setupPinCode=setup_pin_code, + ipaddr=ip_addr, + ), ) async def set_wifi_credentials(self, ssid: str, credentials: str) -> None: