Fix Matter parameter mapping

majordom_matter/controller.py

@@ -14,7 +14,7 @@
 from majordom_integration_sdk.schemas.command import DeviceCommand
 from majordom_integration_sdk.schemas.device import CredentialsType, Discovery, ProvidedCredentials
 from majordom_integration_sdk.schemas.event import DeviceParameterChange
-from majordom_integration_sdk.schemas.parameter import ParameterDataType, ParameterRole
+from majordom_integration_sdk.schemas.parameter import ParameterDataType, ParameterRole, ParameterVisibility
 from matter_server.client import MatterClient
 from matter_server.client.models.node import MatterNode
 from matter_server.common.errors import UnknownError
@@ -272,14 +272,15 @@ async def pair_device(self, discovery: Discovery, credentials: ProvidedCredentia
                                     f"(endpoint {endpoint_id}, cluster {cluster_id})"
                                 )
                             value = node.get_attribute_value(endpoint_id, cluster_id, attribute_id)
-                            value = self._mapper.apply_attribute_scale(cluster_id, attribute_id, value)
                             device.parameters.append(
-                                MatterParameterState(**parameter.__dict__, value=self._mapper.normalize_value(value))
+                                MatterParameterState(
+                                    **parameter.__dict__,
+                                    value=self._mapper.attribute_value(cluster_id, attribute_id, value),
+                                )
                             )
 
-            main_parameter_id, default_value = self._get_main_parameter(device.id, node)
-            main_parameter = next((p for p in device.parameters if p.id == main_parameter_id), None)
-            device.main_parameter = main_parameter_id if main_parameter is not None else None
+            main_parameter, default_value = self._get_main_parameter(device.id, node, device.parameters)
+            device.main_parameter = main_parameter.id if main_parameter is not None else None
             if main_parameter is not None and default_value is not None:
                 if main_parameter.integration_data.type is MatterParameterTypeEnum.attribute:
                     main_parameter.default_value = self._mapper.normalize_value(default_value)
@@ -288,7 +289,7 @@ async def pair_device(self, discovery: Discovery, credentials: ProvidedCredentia
                     main_parameter.integration_data.default_arguments = default_value
                 else:
                     raise MatterUnexpectedError(
-                        f"Command main parameter {main_parameter_id} expected dict default arguments, "
+                        f"Command main parameter {main_parameter.id} expected dict default arguments, "
                         f"got {type(default_value).__name__}: {default_value!r}"
                     )
             if main_parameter is not None and main_parameter.data_type is ParameterDataType.struct:
@@ -328,15 +329,14 @@ async def fetch(self, device: MatterDevice):
                         continue
                     attribute_id = getattr(attribute, "attribute_id", -1)
                     value = node.get_attribute_value(endpoint_id, cluster_id, attribute_id)
-                    value = self._mapper.apply_attribute_scale(cluster_id, attribute_id, value)
                     parameter_id = self._mapper.attribute_parameter_uuid(
                         device.id, endpoint_id, cluster_id, attribute_id
                     )
                     events.append(
                         DeviceParameterChange(
                             device_id=device.id,
                             parameter_id=parameter_id,
-                            value=value if isinstance(value, str | int | float | bool) else None,
+                            value=self._mapper.attribute_value(cluster_id, attribute_id, value),
                         )
                     )
 
@@ -537,8 +537,8 @@ def callback(event_type, new_value):
             event = DeviceParameterChange(
                 device_id=device_id,
                 parameter_id=parameter_id,
-                value=self._mapper.normalize_value(
-                    self._mapper.apply_attribute_scale(cluster_id, attribute_id, new_value)
+                value=self._mapper.attribute_value(
+                    cluster_id, attribute_id, self._mapper.parse_raw_attribute(cluster_id, attribute_id, new_value)
                 ),
             )
             asyncio.create_task(self.dependencies.output.controller_did_receive_events(self, [event]))
@@ -578,7 +578,6 @@ async def _execute_cluster_command(
         self, node, endpoint_id: int, cluster, parameter: MatterParameter, command: DeviceCommand
     ):
         command_id = parameter.integration_data.command_id
-        time_requested_timeout = None
         if command_id is None or command_id < 0:
             raise MatterUnexpectedError(f"Invalid command_id: {command_id}")
 
@@ -599,8 +598,7 @@ async def _execute_cluster_command(
             if not getattr(cmd_class, "is_client", True):
                 continue
 
-            if cluster.id == 0x00000101:  # DoorLock
-                time_requested_timeout = 1000
+            time_requested_timeout = 1000 if cmd_class.must_use_timed_invoke else None
             if isinstance(arguments, dict):
                 arguments = self._mapper.arguments_by_name(
                     command.device_id, endpoint_id, cluster.id, command_id, cmd_class, arguments
@@ -640,7 +638,12 @@ async def _write_cluster_attribute(
         value = self._mapper.parse_data_for_attribute(attribute_cls, command.value) if attribute_cls else command.value
         await self._matter_client.write_attribute(node.node_id, attribute_path, value)
 
-    def _get_main_parameter(self, device_id: UUID, node: MatterNode) -> tuple[UUID | None, DefaultParams]:
+    def _get_main_parameter(
+        self, device_id: UUID, node: MatterNode, parameters: list[MatterParameterState]
+    ) -> tuple[MatterParameterState | None, DefaultParams]:
+        # Only `user` parameters can be main (see can_be_main_parameter, which also needs the
+        # default_value set from these defaults) — skip hidden ones so they don't shadow a later match.
+        candidates = {p.id: p for p in parameters if p.visibility is ParameterVisibility.user}
         for endpoint_id, endpoint in node.endpoints.items():
             for cluster_id, spec in MAIN_PARAMETER_BY_CLUSTER.items():
                 if cluster_id not in endpoint.clusters:
@@ -650,21 +653,17 @@ def _get_main_parameter(self, device_id: UUID, node: MatterNode) -> tuple[UUID |
                     supported_attribute_ids: list[int] = node.get_attribute_value(endpoint_id, cluster_id, 0xFFFB) or []
                     if supported_attribute_ids and spec.command_or_attribute_id not in supported_attribute_ids:
                         continue
-                    return (
-                        self._mapper.attribute_parameter_uuid(
-                            device_id, endpoint_id, cluster_id, spec.command_or_attribute_id
-                        ),
-                        spec.default_params,
+                    parameter_id = self._mapper.attribute_parameter_uuid(
+                        device_id, endpoint_id, cluster_id, spec.command_or_attribute_id
                     )
-
-                accepted_command_ids: list[int] = node.get_attribute_value(endpoint_id, cluster_id, 0xFFF9) or []
-                if accepted_command_ids and spec.command_or_attribute_id not in accepted_command_ids:
-                    continue
-
-                return (
-                    self._mapper.command_parameter_uuid(
+                else:
+                    accepted_command_ids: list[int] = node.get_attribute_value(endpoint_id, cluster_id, 0xFFF9) or []
+                    if accepted_command_ids and spec.command_or_attribute_id not in accepted_command_ids:
+                        continue
+                    parameter_id = self._mapper.command_parameter_uuid(
                         device_id, endpoint_id, cluster_id, spec.command_or_attribute_id
-                    ),
-                    spec.default_params,
-                )
+                    )
+
+                if parameter_id in candidates:
+                    return candidates[parameter_id], spec.default_params
         return None, None

majordom_matter/mapper.py

@@ -4,11 +4,12 @@
 import logging
 from collections.abc import Callable
 from dataclasses import fields, is_dataclass
+from math import floor, log10
 from typing import Any, Union, get_args, get_origin, get_type_hints
 from uuid import UUID
 
 from chip.clusters.CHIPClusters import ChipClusters
-from chip.clusters.ClusterObjects import ClusterAttributeDescriptor, ClusterCommand
+from chip.clusters.ClusterObjects import ALL_ATTRIBUTES, ALL_CLUSTERS, ClusterAttributeDescriptor, ClusterCommand
 from chip.clusters.Types import Nullable, NullValue
 from chip.tlv import TLVReader
 from majordom_integration_sdk.schemas.device import CredentialsType
@@ -19,18 +20,22 @@
     ParameterUnit,
     ParameterVisibility,
 )
-from matter_server.client.models.node import MatterNode
+from matter_server.client.models.node import MatterNode, get_object_params
+from matter_server.common.helpers.util import parse_value
 
 from .matter_spec import (
     ATTRIBUTE_MIN_STEPS,
     ATTRIBUTE_SCALE,
     ATTRIBUTE_UNITS,
+    COMMAND_FIELD_SCALE,
     EVERYDAY_COMMANDS,
     FIELD_TYPE_TO_DATA_TYPE,
     METADATA_SOURCES,
     MIN_MAX_VALUE,
     SYSTEM_ATTRIBUTES,
     SYSTEM_CLUSTERS,
+    SYSTEM_COMMANDS,
+    TEXT_BYTES_FIELDS,
     AttributeKey,
     classify_attribute,
 )
@@ -45,6 +50,20 @@ def _unwrap_optional(field_type: Any) -> Any:
     return args[0] if args else field_type
 
 
+def _parse_bytes(raw: Any, text: bool) -> bytes:
+    """A `string` field is UTF-8 text; a `data` one is base64, as normalize_value emits it."""
+    if isinstance(raw, str):
+        return raw.encode("utf-8") if text else base64.b64decode(raw, validate=True)
+    return bytes(raw)
+
+
+def _to_wire(raw: Any, scale: float | None) -> Any:
+    """The inverse of apply_attribute_scale: a display-unit number back to its wire integer."""
+    if scale is None or not isinstance(raw, int | float) or isinstance(raw, bool):
+        return raw
+    return round(raw / scale)
+
+
 class MatterMapper:
     def __init__(
         self,
@@ -186,9 +205,23 @@ def apply_attribute_scale(self, cluster_id: int, attribute_id: int, value: Any)
         if is_dataclass(value) and hasattr(value, "energy"):
             value = value.energy
         if isinstance(value, (int, float)) and not isinstance(value, bool):
-            return value * scale
+            # Round off float noise (2183 * 0.01 = 21.830000000000002) to the scale's decimals.
+            return round(value * scale, max(0, -floor(log10(scale))))
         return value
 
+    def parse_raw_attribute(self, cluster_id: int, attribute_id: int, raw: Any) -> Any:
+        """A subscription update is matter-server's raw JSON (a struct keyed by field id); parse it
+        into the chip type the node holds, as MatterEndpoint.set_attribute_value does."""
+        attribute_class = ALL_ATTRIBUTES.get(cluster_id, {}).get(attribute_id)
+        if attribute_class is None:
+            return raw
+        name, attribute_type = get_object_params(ALL_CLUSTERS[cluster_id].descriptor, attribute_id)
+        return parse_value(name, raw, attribute_type, attribute_class().value)
+
+    def attribute_value(self, cluster_id: int, attribute_id: int, value: Any) -> Any:
+        """A raw attribute value as the Hub stores it — one path for pairing, fetch and the subscription."""
+        return self.normalize_value(self.apply_attribute_scale(cluster_id, attribute_id, value))
+
     def resolve_runtime_bounds(
         self, key, cluster_id: int, endpoint, name: str, default_min, default_max
     ) -> tuple[Any, Any]:
@@ -218,6 +251,15 @@ def resolve_runtime_bounds(
                 )
         return min_v, max_v
 
+    def resolve_runtime_step(self, key: AttributeKey, endpoint, default: int | float | None) -> int | float | None:
+        """A parameter's step from the device's own step attribute (in the parameter's wire unit),
+        else the spec's ATTRIBUTE_MIN_STEPS default."""
+        source = METADATA_SOURCES.get(key)
+        if source is None or source.step_attr is None:
+            return default
+        raw = self.normalize_value(endpoint.get_attribute_value(key.cluster_id, source.step_attr))
+        return default if raw is None else self.apply_attribute_scale(key.cluster_id, key.attribute_id, raw)
+
     def get_min_max_value(self, attribute, value) -> tuple[int | None, int | None]:
         """
         Encodes the attribute value to TLV to discover its wire type,
@@ -245,20 +287,17 @@ def parse_data_for_command(self, cmd_class: type, data: dict) -> dict:
                 continue
             raw = data[field.name]
             field_type = _unwrap_optional(hints.get(field.name, field.type))
+            key = (getattr(cmd_class, "cluster_id", None), getattr(cmd_class, "command_id", None), field.name)
 
             if raw is None:
                 result[field.name] = NullValue
             elif isinstance(field_type, type) and issubclass(field_type, enum.Enum):
                 result[field.name] = field_type(int(raw))
             elif field_type in (bytes, bytearray):
-                if isinstance(raw, str):
-                    result[field.name] = raw.encode("utf-8")
-                elif isinstance(raw, (list, tuple)):
-                    result[field.name] = bytes(raw)
-                else:
-                    result[field.name] = bytes(raw) if not isinstance(raw, (bytes, bytearray)) else raw
+                result[field.name] = _parse_bytes(raw, key in TEXT_BYTES_FIELDS)
             else:
-                result[field.name] = raw
+                scaled = COMMAND_FIELD_SCALE.get(key)
+                result[field.name] = _to_wire(raw, scaled.scale if scaled else None)
 
         return result
 
@@ -282,22 +321,16 @@ def parse_data_for_attribute(self, attribute_cls: type, raw: Any) -> Any:
         attr_type = getattr(attribute_cls, "attribute_type", None)
         field_type = getattr(attr_type, "Type", None) if attr_type else None
 
-        # Unwrap Optional[X] / Union[X, None] — same as in parse_data_for_command
-        if get_origin(field_type):
-            args = [a for a in get_args(field_type) if a is not type(None)]
-            field_type = args[0] if args else field_type
+        field_type = _unwrap_optional(field_type)
 
         if isinstance(field_type, type) and issubclass(field_type, enum.Enum):
             return field_type(int(raw))
 
         if field_type in (bytes, bytearray):
-            if isinstance(raw, str):
-                return raw.encode("utf-8")
-            if isinstance(raw, (list, tuple)):
-                return bytes(raw)
-            return bytes(raw) if not isinstance(raw, (bytes, bytearray)) else raw
+            return _parse_bytes(raw, text=False)
 
-        return raw
+        key = AttributeKey(getattr(attribute_cls, "cluster_id", -1), getattr(attribute_cls, "attribute_id", -1))
+        return _to_wire(raw, ATTRIBUTE_SCALE.get(key))
 
     # -------------------------------------------------------------------------
     # Node parsing: matter SDK cluster data -> MatterParameter
@@ -328,7 +361,7 @@ def parse_commands(
 
             # Command visibility: system-cluster commands hidden; everyday one-tap actions ->
             # user; every other command (schedule/credential/log management) -> setting.
-            if on_system_cluster:
+            if on_system_cluster or (cluster_id, command_id) in SYSTEM_COMMANDS:
                 visibility = ParameterVisibility.system
             elif (cluster_id, command_id) in EVERYDAY_COMMANDS:
                 visibility = ParameterVisibility.user
@@ -367,12 +400,19 @@ def parse_commands(
                             if matched is not None:
                                 data_type = matched
 
+                    if (cluster_id, command_id, field.name) in TEXT_BYTES_FIELDS:
+                        data_type = ParameterDataType.string
+                    scaled = COMMAND_FIELD_SCALE.get((cluster_id, command_id, field.name))
+                    if scaled is not None:
+                        data_type = ParameterDataType.decimal
+
                     args.append(
                         Parameter(
                             id=self.command_field_uuid(device_id, endpoint_id, cluster_id, command_id, field.name),
                             name=field.name,
                             data_type=data_type,
-                            unit=ParameterUnit.plain,
+                            min_step=scaled.scale if scaled else None,
+                            unit=scaled.unit if scaled else ParameterUnit.plain,
                             role=ParameterRole.control,
                             valid_values=valid_values,
                             visibility=ParameterVisibility.setting,
@@ -472,14 +512,16 @@ def parse_attributes(
             min_value, max_value = self.resolve_runtime_bounds(  # priority 1: device's own limit attrs
                 key, cluster_id, endpoint, name, min_value, max_value
             )
-            scale = ATTRIBUTE_SCALE.get(AttributeKey(cluster_id, attribute_id))
-            if scale is not None:
-                min_value = min_value * scale if min_value is not None else None
-                max_value = max_value * scale if max_value is not None else None
+            min_value = self.apply_attribute_scale(cluster_id, attribute_id, min_value)
+            max_value = self.apply_attribute_scale(cluster_id, attribute_id, max_value)
             value = self.apply_attribute_scale(cluster_id, attribute_id, raw_value)
 
             try:
-                data_type = self.get_parameter_data_type_from_value(self.normalize_value(value))
+                data_type = (
+                    ParameterDataType.data
+                    if _unwrap_optional(getattr(attr_type, "Type", None)) is bytes
+                    else self.get_parameter_data_type_from_value(self.normalize_value(value))
+                )
             except ValueError:
                 # Value type isn't one we know how to represent yet. Skip just this attribute
                 # rather than aborting the whole pairing — the device stays usable, only this
@@ -499,7 +541,7 @@ def parse_attributes(
                     min_value=min_value,
                     max_value=max_value,
                     valid_values=valid_values,
-                    min_step=ATTRIBUTE_MIN_STEPS.get(AttributeKey(cluster_id, attribute_id)),
+                    min_step=self.resolve_runtime_step(key, endpoint, ATTRIBUTE_MIN_STEPS.get(key)),
                     # Our spec table wins; the harvested HA unit fills gaps it leaves plain.
                     unit=ATTRIBUTE_UNITS.get(key, spec.unit or ParameterUnit.plain),
                     role=role,