Handle invalid payloads per RemoteValue, log a readable warning

XKNX · Feb 5, 2022 · db6d2cb · db6d2cb
1 parent f0c8789
commit db6d2cb
Show file tree

Hide file tree

Showing 21 changed files with 126 additions and 134 deletions.
diff --git a/changelog.md b/changelog.md
@@ -6,6 +6,10 @@
 
 - Do a ConnectionStateRequest heartbeat on TCP tunnel connections too
 
+### Devices
+
+- Handle invalid payloads per RemoteValue, log a readable warning
+
 ## 0.19.1 Bugfix for route_back 2022-01-31
 
 ### Connection

diff --git a/xknx/remote_value/remote_value.py b/xknx/remote_value/remote_value.py
@@ -147,11 +147,9 @@ def remote_value_addresses() -> Iterator[DeviceGroupAddress | None]:
         return group_address in remote_value_addresses()
 
     @abstractmethod
-    # TODO: typing - remove Optional
-    def payload_valid(
-        self, payload: DPTArray | DPTBinary | None
-    ) -> DPTPayloadType | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTPayloadType:
         """Return payload if telegram payload may be parsed - to be implemented in derived class."""
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     @abstractmethod
     def from_knx(self, payload: DPTPayloadType) -> ValueType:
@@ -182,21 +180,13 @@ async def process(self, telegram: Telegram, always_callback: bool = False) -> bo
                 device_name=self.device_name,
                 feature_name=self.feature_name,
             )
-        _new_payload = self.payload_valid(telegram.payload.value)
-        if _new_payload is None:
-            raise CouldNotParseTelegram(
-                "payload invalid",
-                payload=str(telegram.payload),
-                destination_address=str(telegram.destination_address),
-                source_address=str(telegram.source_address),
-                device_name=self.device_name,
-                feature_name=self.feature_name,
-            )
+
         try:
+            _new_payload = self.payload_valid(telegram.payload.value)
             decoded_payload = self.from_knx(_new_payload)
-        except ConversionError as err:
+        except (ConversionError, CouldNotParseTelegram) as err:
             logger.warning(
-                "Can not process %s for %s - %s: %s",
+                "Can not process %s \n for %s - %s: %s",
                 telegram,
                 self.device_name,
                 self.feature_name,

diff --git a/xknx/remote_value/remote_value_1count.py b/xknx/remote_value/remote_value_1count.py
@@ -6,20 +6,19 @@
 from __future__ import annotations
 
 from xknx.dpt import DPTArray, DPTBinary, DPTValue1Count
+from xknx.exceptions import CouldNotParseTelegram
 
 from .remote_value import RemoteValue
 
 
 class RemoteValue1Count(RemoteValue[DPTArray, int]):
     """Abstraction for remote value of KNX 6.010 (DPT_Value_1_Count)."""
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray:
         """Test if telegram payload may be parsed."""
-        return (
-            payload
-            if isinstance(payload, DPTArray) and len(payload.value) == 1
-            else None
-        )
+        if isinstance(payload, DPTArray) and len(payload.value) == 1:
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: int) -> DPTArray:
         """Convert value to payload."""

diff --git a/xknx/remote_value/remote_value_climate_mode.py b/xknx/remote_value/remote_value_climate_mode.py
@@ -85,13 +85,11 @@ def supported_operation_modes(self) -> list[HVACOperationMode]:
         """Return a list of all supported operation modes."""
         return list(self._climate_mode_transcoder.SUPPORTED_MODES.values())
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray:
         """Test if telegram payload may be parsed."""
-        return (
-            payload
-            if isinstance(payload, DPTArray) and len(payload.value) == 1
-            else None
-        )
+        if isinstance(payload, DPTArray) and len(payload.value) == 1:
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: Any) -> DPTArray:
         """Convert value to payload."""
@@ -133,13 +131,11 @@ def supported_operation_modes() -> list[HVACControllerMode]:
         """Return a list of all supported operation modes."""
         return list(DPTHVACContrMode.SUPPORTED_MODES.values())
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray:
         """Test if telegram payload may be parsed."""
-        return (
-            payload
-            if isinstance(payload, DPTArray) and len(payload.value) == 1
-            else None
-        )
+        if isinstance(payload, DPTArray) and len(payload.value) == 1:
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     @staticmethod
     def to_knx(value: Any) -> DPTArray:
@@ -194,9 +190,11 @@ def __init__(
             after_update_cb=after_update_cb,
         )
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTBinary | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTBinary:
         """Test if telegram payload may be parsed."""
-        return payload if isinstance(payload, DPTBinary) else None
+        if isinstance(payload, DPTBinary):
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: Any) -> DPTBinary:
         """Convert value to payload."""
@@ -276,9 +274,11 @@ def __init__(
             after_update_cb=after_update_cb,
         )
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTBinary | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTBinary:
         """Test if telegram payload may be parsed."""
-        return payload if isinstance(payload, DPTBinary) else None
+        if isinstance(payload, DPTBinary):
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     @staticmethod
     def supported_operation_modes() -> list[HVACControllerMode]:

diff --git a/xknx/remote_value/remote_value_color_rgb.py b/xknx/remote_value/remote_value_color_rgb.py
@@ -8,7 +8,7 @@
 from typing import TYPE_CHECKING, Sequence, Tuple
 
 from xknx.dpt import DPTArray, DPTBinary
-from xknx.exceptions import ConversionError
+from xknx.exceptions import ConversionError, CouldNotParseTelegram
 
 from .remote_value import AsyncCallbackType, GroupAddressesType, RemoteValue
 
@@ -40,13 +40,11 @@ def __init__(
             after_update_cb=after_update_cb,
         )
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray:
         """Test if telegram payload may be parsed."""
-        return (
-            payload
-            if isinstance(payload, DPTArray) and len(payload.value) == 3
-            else None
-        )
+        if isinstance(payload, DPTArray) and len(payload.value) == 3:
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: Sequence[int]) -> DPTArray:
         """Convert value to payload."""

diff --git a/xknx/remote_value/remote_value_color_rgbw.py b/xknx/remote_value/remote_value_color_rgbw.py
@@ -8,7 +8,7 @@
 from typing import TYPE_CHECKING, Sequence, Tuple
 
 from xknx.dpt import DPTArray, DPTBinary
-from xknx.exceptions import ConversionError
+from xknx.exceptions import ConversionError, CouldNotParseTelegram
 
 from .remote_value import AsyncCallbackType, GroupAddressesType, RemoteValue
 
@@ -41,13 +41,11 @@ def __init__(
         )
         self.previous_value: tuple[int, int, int, int] = (0, 0, 0, 0)
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray:
         """Test if telegram payload may be parsed."""
-        return (
-            payload
-            if isinstance(payload, DPTArray) and len(payload.value) == 6
-            else None
-        )
+        if isinstance(payload, DPTArray) and len(payload.value) == 6:
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: Sequence[int]) -> DPTArray:
         """

diff --git a/xknx/remote_value/remote_value_color_xyy.py b/xknx/remote_value/remote_value_color_xyy.py
@@ -9,6 +9,7 @@
 
 from xknx.dpt import DPTArray, DPTBinary
 from xknx.dpt.dpt_color import DPTColorXYY, XYYColor
+from xknx.exceptions import CouldNotParseTelegram
 
 from .remote_value import AsyncCallbackType, GroupAddressesType, RemoteValue
 
@@ -42,14 +43,11 @@ def __init__(
             after_update_cb=after_update_cb,
         )
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray:
         """Test if telegram payload may be parsed."""
-        return (
-            payload
-            if isinstance(payload, DPTArray)
-            and len(payload.value) == self.PAYLOAD_LENGTH
-            else None
-        )
+        if isinstance(payload, DPTArray) and len(payload.value) == self.PAYLOAD_LENGTH:
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: XYYColor) -> DPTArray:
         """Convert value to payload."""

diff --git a/xknx/remote_value/remote_value_control.py b/xknx/remote_value/remote_value_control.py
@@ -9,7 +9,7 @@
 from typing import TYPE_CHECKING, Any
 
 from xknx.dpt import DPTArray, DPTBinary, DPTControlStepCode
-from xknx.exceptions import ConversionError
+from xknx.exceptions import ConversionError, CouldNotParseTelegram
 
 from .remote_value import AsyncCallbackType, GroupAddressesType, RemoteValue
 
@@ -50,9 +50,11 @@ def __init__(
             after_update_cb=after_update_cb,
         )
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTBinary | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTBinary:
         """Test if telegram payload may be parsed."""
-        return payload if isinstance(payload, DPTBinary) else None
+        if isinstance(payload, DPTBinary):
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: Any) -> DPTBinary:
         """Convert value to payload."""

diff --git a/xknx/remote_value/remote_value_datetime.py b/xknx/remote_value/remote_value_datetime.py
@@ -10,7 +10,7 @@
 from typing import TYPE_CHECKING
 
 from xknx.dpt import DPTArray, DPTBinary, DPTDate, DPTDateTime, DPTTime
-from xknx.exceptions import ConversionError
+from xknx.exceptions import ConversionError, CouldNotParseTelegram
 
 from .remote_value import AsyncCallbackType, GroupAddressesType, RemoteValue
 
@@ -62,14 +62,14 @@ def __init__(
             after_update_cb=after_update_cb,
         )
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray:
         """Test if telegram payload may be parsed."""
-        return (
-            payload
-            if isinstance(payload, DPTArray)
+        if (
+            isinstance(payload, DPTArray)
             and len(payload.value) == self.dpt_class.payload_length
-            else None
-        )
+        ):
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: time.struct_time) -> DPTArray:
         """Convert value to payload."""

diff --git a/xknx/remote_value/remote_value_dpt_2_byte_unsigned.py b/xknx/remote_value/remote_value_dpt_2_byte_unsigned.py
@@ -8,6 +8,7 @@
 from typing import TYPE_CHECKING
 
 from xknx.dpt import DPT2ByteUnsigned, DPTArray, DPTBinary
+from xknx.exceptions import CouldNotParseTelegram
 
 from .remote_value import AsyncCallbackType, GroupAddressesType, RemoteValue
 
@@ -39,13 +40,11 @@ def __init__(
             after_update_cb=after_update_cb,
         )
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray:
         """Test if telegram payload may be parsed."""
-        return (
-            payload
-            if isinstance(payload, DPTArray) and len(payload.value) == 2
-            else None
-        )
+        if isinstance(payload, DPTArray) and len(payload.value) == 2:
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: int) -> DPTArray:
         """Convert value to payload."""

diff --git a/xknx/remote_value/remote_value_dpt_value_1_ucount.py b/xknx/remote_value/remote_value_dpt_value_1_ucount.py
@@ -6,20 +6,19 @@
 from __future__ import annotations
 
 from xknx.dpt import DPTArray, DPTBinary, DPTValue1Ucount
+from xknx.exceptions import CouldNotParseTelegram
 
 from .remote_value import RemoteValue
 
 
 class RemoteValueDptValue1Ucount(RemoteValue[DPTArray, int]):
     """Abstraction for remote value of KNX DPT 5.010."""
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray:
         """Test if telegram payload may be parsed."""
-        return (
-            payload
-            if isinstance(payload, DPTArray) and len(payload.value) == 1
-            else None
-        )
+        if isinstance(payload, DPTArray) and len(payload.value) == 1:
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: int) -> DPTArray:
         """Convert value to payload."""

diff --git a/xknx/remote_value/remote_value_raw.py b/xknx/remote_value/remote_value_raw.py
@@ -9,7 +9,7 @@
 from typing import TYPE_CHECKING, Union
 
 from xknx.dpt.dpt import DPTArray, DPTBinary
-from xknx.exceptions import ConversionError
+from xknx.exceptions import ConversionError, CouldNotParseTelegram
 
 from .remote_value import AsyncCallbackType, GroupAddressesType, RemoteValue
 
@@ -45,13 +45,13 @@ def __init__(
 
     def payload_valid(
         self, payload: DPTArray | DPTBinary | None
-    ) -> DPTArray | DPTBinary | None:
+    ) -> DPTArray | DPTBinary:
         """Test if telegram payload may be parsed."""
         if isinstance(payload, DPTBinary) and self.payload_length == 0:
             return payload
         if isinstance(payload, DPTArray) and len(payload.value) == self.payload_length:
             return payload
-        return None
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: int) -> DPTArray | DPTBinary:
         """Convert value to payload."""

diff --git a/xknx/remote_value/remote_value_scaling.py b/xknx/remote_value/remote_value_scaling.py
@@ -8,6 +8,7 @@
 from typing import TYPE_CHECKING
 
 from xknx.dpt import DPTArray, DPTBinary
+from xknx.exceptions import CouldNotParseTelegram
 
 from .remote_value import AsyncCallbackType, GroupAddressesType, RemoteValue
 
@@ -43,13 +44,11 @@ def __init__(
         self.range_from = range_from
         self.range_to = range_to
 
-    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray | None:
+    def payload_valid(self, payload: DPTArray | DPTBinary | None) -> DPTArray:
         """Test if telegram payload may be parsed."""
-        return (
-            payload
-            if isinstance(payload, DPTArray) and len(payload.value) == 1
-            else None
-        )
+        if isinstance(payload, DPTArray) and len(payload.value) == 1:
+            return payload
+        raise CouldNotParseTelegram("Payload invalid", payload=str(payload))
 
     def to_knx(self, value: float) -> DPTArray:
         """Convert value to payload."""