Repository: XKNX/xknx
Pull request #838 (2021-12-30 16:10:05)

Handle ConversionError in RemoteValue
<!--
  You are awesome! Thanks for contributing to our project!
  Please, DO NOT DELETE ANY TEXT from this template!.
-->
## Description
<!--
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.
-->
Handle ConversionError in RemoteValue. Log a warning when `DPT*.from_knx()` raises `ConversionError` for each RemoteValue processing the Telegram instead of handling the exception in TelegramQueue.
As a result wrongly configured GAs can be identified more easily. 

Eg. when using `operation_mode_state_address` instead of `controller_mode_state_address` in one Climate entity the name and feature name of the climate entity will be logged instead of a generic Conversion error from the TelegramQueue.
Other devices using the same GA with a different DPT* will not be prevented from trying to parse it.

Fixes # (issue)

## Type of change
<!--
Please tick the applicable options.
NOTE: Ticking multiple options most likely indicates
that your change is to big and it is suggested to split it into several smaller PRs.
-->

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update

## Checklist:

- [ ] I have performed a self-review of my own code
- [ ] The documentation has been adjusted accordingly
- [ ] The changes generate no new warnings
- [ ] Tests have been added that prove the fix is effective or that the feature works
- [x] The changes are documented in the changelog
- [ ] The Homeassistant plugin has been adjusted in case of new config options


codecov[bot]: # [Codecov](https://codecov.io/gh/XKNX/xknx/pull/838?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) Report
> Merging [#838](https://codecov.io/gh/XKNX/xknx/pull/838?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (f7d621b) into [main](https://codecov.io/gh/XKNX/xknx/commit/9ecff813cf77a36a100f5659d23b7bd061bcf044?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (9ecff81) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

> :exclamation: Current head f7d621b differs from pull request most recent head 47217a6. Consider uploading reports for the commit 47217a6 to get more accurate results
[![Impacted file tree graph](https://codecov.io/gh/XKNX/xknx/pull/838/graphs/tree.svg?width=650&height=150&src=pr&token=irWbIygS84&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)](https://codecov.io/gh/XKNX/xknx/pull/838?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)

```diff
@@           Coverage Diff           @@
##             main     #838   +/-   ##
=======================================
  Coverage   95.28%   95.28%           
=======================================
  Files         109      109           
  Lines        6764     6768    +4     
=======================================
+ Hits         6445     6449    +4     
  Misses        319      319           
```


| [Impacted Files](https://codecov.io/gh/XKNX/xknx/pull/838?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) | Coverage Δ | |
|---|---|---|
| [xknx/dpt/dpt\_hvac\_mode.py](https://codecov.io/gh/XKNX/xknx/pull/838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9kcHQvZHB0X2h2YWNfbW9kZS5weQ==) | `100.00% <100.00%> (ø)` | |
| [xknx/remote\_value/remote\_value.py](https://codecov.io/gh/XKNX/xknx/pull/838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9yZW1vdGVfdmFsdWUvcmVtb3RlX3ZhbHVlLnB5) | `100.00% <100.00%> (ø)` | |


Patch:
diff --git a/changelog.md b/changelog.md
index 35e3c87c..08af4cee 100644
--- a/changelog.md
+++ b/changelog.md
@@ -2,6 +2,10 @@
 
 ## Unreleased changes
 
+### Devices
+
+- Handle ConversionError in RemoteValue, log a readable warning
+
 ### Connection
 
 - Handle separate Tunneling control and data endpoints
diff --git a/docs/xknx.md b/docs/xknx.md
index c619f559..106302dd 100644
--- a/docs/xknx.md
+++ b/docs/xknx.md
@@ -146,7 +146,7 @@ async def main():
 asyncio.run(main())
 ```
 
-An awaitable `connection_state_change_cb` will be called every time the connection state to the gateway changes. Example:
+An awaitable `connection_state_changed_cb` will be called every time the connection state to the gateway changes. Example:
 
 ```python
 import asyncio
@@ -154,12 +154,12 @@ from xknx import XKNX
 from xknx.core import XknxConnectionState
 
 
-async def connection_state_change_cb(state: XknxConnectionState):
+async def connection_state_changed_cb(state: XknxConnectionState):
     print("Callback received with state {0}".format(state.name))
 
 
 async def main():
-    xknx = XKNX(connection_state_change_cb=connection_state_change_cb, daemon_mode=True)
+    xknx = XKNX(connection_state_changed_cb=connection_state_changed_cb, daemon_mode=True)
     await xknx.start()
     await xknx.stop()
 
diff --git a/xknx/dpt/dpt_hvac_mode.py b/xknx/dpt/dpt_hvac_mode.py
index c0618954..72ff086d 100644
--- a/xknx/dpt/dpt_hvac_mode.py
+++ b/xknx/dpt/dpt_hvac_mode.py
@@ -4,7 +4,7 @@ from __future__ import annotations
 from enum import Enum
 from typing import Generic, TypeVar
 
-from xknx.exceptions import ConversionError, CouldNotParseKNXIP
+from xknx.exceptions import ConversionError
 
 from .dpt import DPTBase
 
@@ -51,9 +51,10 @@ class _DPTClimateMode(DPTBase, Generic[HVACModeType]):
     def from_knx(cls, raw: tuple[int, ...]) -> HVACModeType:
         """Parse/deserialize from KNX/IP raw data."""
         cls.test_bytesarray(raw)
-        if raw[0] in cls.SUPPORTED_MODES:
+        try:
             return cls.SUPPORTED_MODES[raw[0]]
-        raise CouldNotParseKNXIP("Could not parse HVACOperationMode")
+        except KeyError:
+            raise ConversionError(f"Payload not supported for {cls.__name__}", raw=raw)
 
     @classmethod
     def to_knx(cls, value: HVACModeType) -> tuple[int]:
@@ -61,7 +62,7 @@ class _DPTClimateMode(DPTBase, Generic[HVACModeType]):
         for knx_value, mode in cls.SUPPORTED_MODES.items():
             if mode == value:
                 return (knx_value,)
-        raise ConversionError(f"Could not parse {cls.__name__}", value=value)
+        raise ConversionError(f"Value not supported for {cls.__name__}", value=value)
 
 
 class DPTHVACContrMode(_DPTClimateMode[HVACControllerMode]):
@@ -135,4 +136,4 @@ class DPTControllerStatus(_DPTClimateMode[HVACOperationMode]):
             return HVACOperationMode.STANDBY
         if raw[0] & 1 > 0:
             return HVACOperationMode.COMFORT
-        raise CouldNotParseKNXIP("Could not parse HVACOperationMode")
+        raise ConversionError(f"Payload not supported for {cls.__name__}", raw=raw)
diff --git a/xknx/remote_value/remote_value.py b/xknx/remote_value/remote_value.py
index 404a5c16..e4ed2a0a 100644
--- a/xknx/remote_value/remote_value.py
+++ b/xknx/remote_value/remote_value.py
@@ -22,7 +22,7 @@ from typing import (
 )
 
 from xknx.dpt.dpt import DPTArray, DPTBinary
-from xknx.exceptions import CouldNotParseTelegram
+from xknx.exceptions import ConversionError, CouldNotParseTelegram
 from xknx.telegram import GroupAddress, Telegram
 from xknx.telegram.address import (
     DeviceGroupAddress,
@@ -192,7 +192,17 @@ class RemoteValue(ABC, Generic[DPTPayloadType, ValueType]):
                 device_name=self.device_name,
                 feature_name=self.feature_name,
             )
-        decoded_payload = self.from_knx(_new_payload)
+        try:
+            decoded_payload = self.from_knx(_new_payload)
+        except ConversionError as err:
+            logger.warning(
+                "Can not process %s for %s - %s: %s",
+                telegram,
+                self.device_name,
+                self.feature_name,
+                err,
+            )
+            return False
         self.xknx.state_updater.update_received(self)
         if self._value is None or always_callback or self._value != decoded_payload:
             self._value = decoded_payload


Test patch:
diff --git a/requirements/testing.txt b/requirements/testing.txt
index 547c845d..3daa7155 100644
--- a/requirements/testing.txt
+++ b/requirements/testing.txt
@@ -8,6 +8,6 @@ pylint==2.12.2
 pytest==6.2.5
 pytest-asyncio==0.16.0
 pytest-cov==3.0.0
-setuptools==60.2.0
+setuptools==60.3.1
 tox==3.24.5
 mypy==0.930
diff --git a/test/dpt_tests/dpt_hvac_mode_test.py b/test/dpt_tests/dpt_hvac_mode_test.py
index 7221c356..86d5b931 100644
--- a/test/dpt_tests/dpt_hvac_mode_test.py
+++ b/test/dpt_tests/dpt_hvac_mode_test.py
@@ -2,7 +2,7 @@
 import pytest
 from xknx.dpt import DPTControllerStatus, DPTHVACMode
 from xknx.dpt.dpt_hvac_mode import HVACOperationMode
-from xknx.exceptions import ConversionError, CouldNotParseKNXIP
+from xknx.exceptions import ConversionError
 
 
 class TestDPTControllerStatus:
@@ -68,7 +68,7 @@ class TestDPTControllerStatus:
 
     def test_mode_from_knx_wrong_code(self):
         """Test parsing of DPTHVACMode with wrong code."""
-        with pytest.raises(CouldNotParseKNXIP):
+        with pytest.raises(ConversionError):
             DPTHVACMode.from_knx((0x05,))
 
     def test_controller_status_from_knx_wrong_value(self):
@@ -78,5 +78,5 @@ class TestDPTControllerStatus:
 
     def test_controller_status_from_knx_wrong_code(self):
         """Test parsing of DPTControllerStatus with wrong code."""
-        with pytest.raises(CouldNotParseKNXIP):
+        with pytest.raises(ConversionError):
             DPTControllerStatus.from_knx((0x00,))
diff --git a/test/remote_value_tests/remote_value_control_test.py b/test/remote_value_tests/remote_value_control_test.py
index ab2ac9fd..0ea4f120 100644
--- a/test/remote_value_tests/remote_value_control_test.py
+++ b/test/remote_value_tests/remote_value_control_test.py
@@ -58,11 +58,11 @@ class TestRemoteValueControl:
                 payload=GroupValueWrite(DPTArray(0x01)),
             )
             await remote_value.process(telegram)
-        with pytest.raises(ConversionError):
-            telegram = Telegram(
-                destination_address=GroupAddress("1/2/3"),
-                payload=GroupValueWrite(DPTBinary(0x10)),
-            )
-            await remote_value.process(telegram)
-            # pylint: disable=pointless-statement
-            remote_value.value
+
+        telegram = Telegram(
+            destination_address=GroupAddress("1/2/3"),
+            payload=GroupValueWrite(DPTBinary(0x10)),
+        )
+        assert await remote_value.process(telegram) is False
+        # pylint: disable=pointless-statement
+        remote_value.value
diff --git a/test/remote_value_tests/remote_value_test.py b/test/remote_value_tests/remote_value_test.py
index ba7dc5c1..7a620655 100644
--- a/test/remote_value_tests/remote_value_test.py
+++ b/test/remote_value_tests/remote_value_test.py
@@ -71,9 +71,9 @@ class TestRemoteValue:
         """Test for warning if RemoteValue is read only."""
         xknx = XKNX()
         remote_value = RemoteValue(xknx, group_address_state=GroupAddress("1/2/3"))
-        with patch("logging.Logger.warning") as mock_info:
+        with patch("logging.Logger.warning") as mock_warning:
             await remote_value.set(23)
-            mock_info.assert_called_with(
+            mock_warning.assert_called_with(
                 "Attempted to set value for non-writable device: %s - %s (value: %s)",
                 "Unknown",
                 "Unknown",
@@ -86,8 +86,8 @@ class TestRemoteValue:
         remote_value = RemoteValue(xknx)
         assert remote_value.unit_of_measurement is None
 
-    async def test_process_unsupported_payload(self):
-        """Test if exception is raised when processing telegram with unsupported payload."""
+    async def test_process_unsupported_payload_type(self):
+        """Test if exception is raised when processing telegram with unsupported payload type."""
         xknx = XKNX()
         remote_value = RemoteValue(xknx)
         with patch("xknx.remote_value.RemoteValue.payload_valid") as patch_valid, patch(
@@ -122,6 +122,34 @@ class TestRemoteValue:
             with pytest.raises(CouldNotParseTelegram, match=r".*payload invalid.*"):
                 await remote_value.process(telegram)
 
+    async def test_process_unsupported_payload(self):
+        """Test warning is logged when processing telegram with unsupported payload."""
+        xknx = XKNX()
+        remote_value = RemoteValue(xknx)
+        telegram = Telegram(
+            destination_address=GroupAddress("1/2/1"),
+            payload=GroupValueWrite(DPTArray((0x01, 0x02))),
+        )
+        with patch("xknx.remote_value.RemoteValue.payload_valid") as patch_valid, patch(
+            "xknx.remote_value.RemoteValue.has_group_address"
+        ) as patch_has_group_address, patch(
+            "xknx.remote_value.RemoteValue.from_knx"
+        ) as patch_from, patch(
+            "logging.Logger.warning"
+        ) as mock_warning:
+            patch_valid.return_value = telegram.payload.value
+            patch_has_group_address.return_value = True
+            patch_from.side_effect = ConversionError("TestError")
+
+            assert await remote_value.process(telegram) is False
+            mock_warning.assert_called_once_with(
+                "Can not process %s for %s - %s: %s",
+                telegram,
+                "Unknown",
+                "Unknown",
+                ConversionError("TestError"),
+            )
+
     async def test_read_state(self):
         """Test read state while waiting for the result."""
         xknx = XKNX()
@@ -149,12 +177,12 @@ class TestRemoteValue:
 
         with patch("xknx.remote_value.RemoteValue.payload_valid") as patch_valid, patch(
             "xknx.core.ValueReader.read", new_callable=AsyncMock
-        ) as patch_read, patch("logging.Logger.warning") as mock_info:
+        ) as patch_read, patch("logging.Logger.warning") as mock_warning:
             patch_valid.return_value = False
             patch_read.return_value = None
 
             await remote_value.read_state(wait_for_result=True)
-            mock_info.assert_called_with(
+            mock_warning.assert_called_once_with(
                 "Could not sync group address '%s' (%s - %s)",
                 GroupAddress("1/2/3"),
                 "Unknown",