Speedup diagnostics (#518)

matter_server/client/client.py

@@ -10,12 +10,11 @@
 from aiohttp import ClientSession
 from chip.clusters import Objects as Clusters
 
-from matter_server.common.errors import ERROR_MAP, MatterError, NodeNotExists
+from matter_server.common.errors import ERROR_MAP, NodeNotExists
 
 from ..common.helpers.util import (
-    convert_hex_string,
+    convert_ip_address,
     convert_mac_address,
-    create_attribute_path_from_attribute,
     dataclass_from_dict,
     dataclass_to_dict,
 )
@@ -51,7 +50,7 @@
 
 SUB_WILDCARD: Final = "*"
 
-# pylint: disable=too-many-public-methods
+# pylint: disable=too-many-public-methods,too-many-locals,too-many-branches
 
 
 class MatterClient:
@@ -216,17 +215,6 @@ async def get_matter_fabrics(self, node_id: int) -> list[MatterFabricData]:
         """
 
         node = self.get_node(node_id)
-        if node.available:
-            # try to refresh the OperationalCredentials.Fabric attribute
-            # so we have the most accurate information
-            attr_path = create_attribute_path_from_attribute(
-                0, Clusters.OperationalCredentials.Attributes.Fabrics
-            )
-            try:
-                await self.refresh_attribute(node_id, attr_path)
-            except MatterError as err:
-                self.logger.exception(err)
-
         fabrics: list[
             Clusters.OperationalCredentials.Structs.FabricDescriptorStruct
         ] = node.get_attribute_value(
@@ -270,13 +258,12 @@ async def ping_node(self, node_id: int) -> NodePingResult:
     async def node_diagnostics(self, node_id: int) -> NodeDiagnostics:
         """Gather diagnostics for the given node."""
         node = self.get_node(node_id)
-        # ping the node (will also refresh NetworkInterfaces data)
-        ping_result = await self.ping_node(node_id)
         # grab some details from the first (operational) network interface
         network_type = NetworkType.UNKNOWN
         mac_address = None
         attribute = Clusters.GeneralDiagnostics.Attributes.NetworkInterfaces
         network_interface: Clusters.GeneralDiagnostics.Structs.NetworkInterface
+        ip_addresses: list[str] = []
         for network_interface in node.get_attribute_value(
             0, cluster=None, attribute=attribute
         ):
@@ -302,38 +289,44 @@ async def node_diagnostics(self, node_id: int) -> NodeDiagnostics:
                 # unknown interface: ignore
                 continue
             mac_address = convert_mac_address(network_interface.hardwareAddress)
+            # enumerate ipv4 and ipv6 addresses
+            for ipv4_address_hex in network_interface.IPv4Addresses:
+                ipv4_address = convert_ip_address(ipv4_address_hex)
+                ip_addresses.append(ipv4_address)
+            for ipv6_address_hex in network_interface.IPv6Addresses:
+                ipv6_address = convert_ip_address(ipv6_address_hex, True)
+                ip_addresses.append(ipv6_address)
             break
         # get thread/wifi specific info
         node_type = NodeType.UNKNOWN
         network_name = None
         if network_type == NetworkType.THREAD:
-            cluster: Clusters.ThreadNetworkDiagnostics = node.get_cluster(
+            thread_cluster: Clusters.ThreadNetworkDiagnostics = node.get_cluster(
                 0, Clusters.ThreadNetworkDiagnostics
             )
-            network_name = convert_hex_string(cluster.networkName)
+            network_name = thread_cluster.networkName
             # parse routing role to (diagnostics) node type
             if (
-                cluster.routingRole
+                thread_cluster.routingRole
                 == Clusters.ThreadNetworkDiagnostics.Enums.RoutingRoleEnum.kSleepyEndDevice
             ):
                 node_type = NodeType.SLEEPY_END_DEVICE
-            if cluster.routingRole in (
+            if thread_cluster.routingRole in (
                 Clusters.ThreadNetworkDiagnostics.Enums.RoutingRoleEnum.kLeader,
                 Clusters.ThreadNetworkDiagnostics.Enums.RoutingRoleEnum.kRouter,
             ):
                 node_type = NodeType.ROUTING_END_DEVICE
             elif (
-                cluster.routingRole
+                thread_cluster.routingRole
                 == Clusters.ThreadNetworkDiagnostics.Enums.RoutingRoleEnum.kEndDevice
             ):
                 node_type = NodeType.END_DEVICE
         elif network_type == NetworkType.WIFI:
-            attr_value: bytes = node.get_attribute_value(
-                0,
-                cluster=None,
-                attribute=Clusters.WiFiNetworkDiagnostics.Attributes.Bssid,
+            wifi_cluster: Clusters.WiFiNetworkDiagnostics = node.get_cluster(
+                0, Clusters.WiFiNetworkDiagnostics
             )
-            network_name = convert_hex_string(attr_value)
+            if wifi_cluster and wifi_cluster.bssid:
+                network_name = wifi_cluster.bssid
             node_type = NodeType.END_DEVICE
         # override node type if node is a bridge
         if node.node_data.is_bridge:
@@ -345,9 +338,9 @@ async def node_diagnostics(self, node_id: int) -> NodeDiagnostics:
             network_type=network_type,
             node_type=node_type,
             network_name=network_name,
-            ip_adresses=list(ping_result),
+            ip_adresses=ip_addresses,
             mac_address=mac_address,
-            reachable=any(ping_result.values()),
+            available=node.available,
             active_fabrics=active_fabrics,
         )
 

matter_server/client/models/node.py

@@ -400,5 +400,5 @@ class NodeDiagnostics:
     network_name: str | None  # WiFi SSID or Thread network name
     ip_adresses: list[str]
     mac_address: str | None
-    reachable: bool
+    available: bool
     active_fabrics: list[MatterFabricData]

matter_server/server/device_controller.py

@@ -65,7 +65,7 @@
     0, Clusters.ThreadNetworkDiagnostics.Attributes.RoutingRole
 )
 
-BASE_SUBSCRIBE_ATTRIBUTES: tuple[Attribute.AttributePath, Attribute.AttributePath] = (
+BASE_SUBSCRIBE_ATTRIBUTES: tuple[Attribute.AttributePath, ...] = (
     # all endpoints, BasicInformation cluster
     Attribute.AttributePath(
         EndpointId=None, ClusterId=Clusters.BasicInformation.id, Attribute=None
@@ -76,6 +76,15 @@
         ClusterId=Clusters.BridgedDeviceBasicInformation.id,
         Attribute=None,
     ),
+    # networkinterfaces attribute on general diagnostics cluster,
+    # so we have the most accurate IP addresses for ping/diagnostics
+    Attribute.AttributePath(
+        EndpointId=0, Attribute=Clusters.GeneralDiagnostics.Attributes.NetworkInterfaces
+    ),
+    # active fabrics attribute - to speedup node diagnostics
+    Attribute.AttributePath(
+        EndpointId=0, Attribute=Clusters.OperationalCredentials.Attributes.Fabrics
+    ),
 )
 
 # pylint: disable=too-many-lines,too-many-locals,too-many-statements,too-many-branches
@@ -699,13 +708,6 @@ async def ping_node(self, node_id: int) -> NodePingResult:
             raise NodeNotExists(
                 f"Node {node_id} does not exist or is not yet interviewed"
             )
-        if node.available:
-            # try to refresh the GeneralDiagnostics.NetworkInterface attribute
-            # so we have the most accurate information before pinging
-            try:
-                await self.read_attribute(node_id, attr_path)
-            except (NodeNotResolving, ChipStackError) as err:
-                LOGGER.exception(err)
 
         battery_powered = (
             node.attributes.get(ROUTING_ROLE_ATTRIBUTE_PATH, 0)

matter_server/server/helpers/utils.py

@@ -3,6 +3,8 @@
 import asyncio
 import platform
 
+import async_timeout
+
 PLATFORM_MAC = platform.system() == "Darwin"
 
 
@@ -16,7 +18,13 @@ async def ping_ip(ip_address: str, timeout: int = 2) -> bool:
         cmd = f"ping6 -c 1 -W {timeout} {ip_address}"
     else:
         cmd = f"ping -c 1 -W {timeout} {ip_address}"
-    return (await check_output(cmd))[0] == 0
+    try:
+        # we add an additional timeout here as safeguard and to account for the fact
+        # that macos does not seem to have timeout on ping6
+        async with async_timeout.timeout(timeout + 2):
+            return (await check_output(cmd))[0] == 0
+    except asyncio.TimeoutError:
+        return False
 
 
 async def check_output(shell_cmd: str) -> tuple[int | None, bytes]:
@@ -26,5 +34,10 @@ async def check_output(shell_cmd: str) -> tuple[int | None, bytes]:
         stderr=asyncio.subprocess.STDOUT,
         stdout=asyncio.subprocess.PIPE,
     )
-    stdout, _ = await proc.communicate()
+    try:
+        stdout, _ = await proc.communicate()
+    except asyncio.CancelledError:
+        proc.terminate()
+        await proc.communicate()
+        raise
     return (proc.returncode, stdout)