Repository: XKNX/xknx
Pull request #1051 (2022-10-11 21:24:01)

Fix management frame handling
<!--
  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.
-->
- fix APCI service parsing for 10bit control fileds
- set reasonable default count values for APCI classes
- set xknx.current_address for routing connections so management frames received over Routing are handled
- fix wrong length of AuthorizeRequest
- raise sane error messages in Management 

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.
-->

- [x] 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:

- [ ] The documentation has been adjusted accordingly
- [ ] Tests have been added that prove the fix is effective or that the feature works
- [x] The changes are documented in the changelog (docs/changelog.md)

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

> :exclamation: Current head 870ab56 differs from pull request most recent head af688bb. Consider uploading reports for the commit af688bb to get more accurate results

[![Impacted file tree graph](https://codecov.io/gh/XKNX/xknx/pull/1051/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/1051?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)

```diff
@@           Coverage Diff           @@
##             main    #1051   +/-   ##
=======================================
  Coverage   96.36%   96.36%           
=======================================
  Files         138      138           
  Lines        8579     8581    +2     
=======================================
+ Hits         8267     8269    +2     
  Misses        312      312           
```


| [Impacted Files](https://codecov.io/gh/XKNX/xknx/pull/1051?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) | Coverage Δ | |
|---|---|---|
| [xknx/management/management.py](https://codecov.io/gh/XKNX/xknx/pull/1051/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9tYW5hZ2VtZW50L21hbmFnZW1lbnQucHk=) | `88.27% <0.00%> (ø)` | |
| [xknx/io/routing.py](https://codecov.io/gh/XKNX/xknx/pull/1051/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9pby9yb3V0aW5nLnB5) | `90.62% <100.00%> (+0.07%)` | :arrow_up: |
| [xknx/knxip/cemi\_frame.py](https://codecov.io/gh/XKNX/xknx/pull/1051/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9rbnhpcC9jZW1pX2ZyYW1lLnB5) | `100.00% <100.00%> (ø)` | |
| [xknx/telegram/apci.py](https://codecov.io/gh/XKNX/xknx/pull/1051/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC90ZWxlZ3JhbS9hcGNpLnB5) | `99.29% <100.00%> (ø)` | |


Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index 4d0402ff..644077c3 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -6,6 +6,16 @@ nav_order: 2
 
 # Changelog
 
+# Unreleased changes
+
+### Management
+
+- Fix APCI service parsing for 10bit control fileds.
+- Set reasonable default count values for APCI classes.
+- Set xknx.current_address for routing connections so management frames received over Routing are handled properly.
+- Fix wrong length of AuthorizeRequest.
+- Raise sane error messages in Management.
+
 # 1.2.0 Secure Routing 2022-10-10
 
 ### Features
diff --git a/xknx/io/routing.py b/xknx/io/routing.py
index 7c9573c2..6ae3a41f 100644
--- a/xknx/io/routing.py
+++ b/xknx/io/routing.py
@@ -172,6 +172,7 @@ class Routing(Interface):
 
     async def connect(self) -> bool:
         """Start routing."""
+        self.xknx.current_address = self.xknx.own_address
         await self.xknx.connection_manager.connection_state_changed(
             XknxConnectionState.CONNECTING
         )
diff --git a/xknx/knxip/cemi_frame.py b/xknx/knxip/cemi_frame.py
index 5a50e04d..57124f62 100644
--- a/xknx/knxip/cemi_frame.py
+++ b/xknx/knxip/cemi_frame.py
@@ -152,7 +152,9 @@ class CEMIFrame:
 
         npdu_len = cemi[8 + addil]
 
-        apdu = cemi[9 + addil :]
+        tpdu = cemi[9 + addil :]
+        apdu = bytes([tpdu[0] & 0b11]) + tpdu[1:]  # clear TPCI bits
+
         if len(apdu) != (npdu_len + 1):  # TCPI octet not included in NPDU length
             raise CouldNotParseKNXIP(
                 f"APDU LEN should be {npdu_len} but is {len(apdu) - 1} in CEMI: {cemi.hex()}"
@@ -164,12 +166,10 @@ class CEMIFrame:
         # APCI (application layer control information) -> Last  10 bit of TPCI/APCI
         try:
             self.tpci = TPCI.resolve(
-                raw_tpci=cemi[9 + addil], dst_is_group_address=dst_is_group_address
+                raw_tpci=tpdu[0], dst_is_group_address=dst_is_group_address
             )
         except ConversionError as err:
-            raise UnsupportedCEMIMessage(
-                f"TPCI not supported: {cemi[9 + addil]:#10b}"
-            ) from err
+            raise UnsupportedCEMIMessage(f"TPCI not supported: {tpdu[0]:#10b}") from err
 
         if self.tpci.control:
             if npdu_len:
diff --git a/xknx/management/management.py b/xknx/management/management.py
index c6b1813b..86ac6875 100644
--- a/xknx/management/management.py
+++ b/xknx/management/management.py
@@ -152,7 +152,7 @@ class P2PConnection:
             self._response_waiter.cancel()
             raise ManagementConnectionError(
                 f"Connection to {self.address} failed: {exc}"
-            )
+            ) from exc
         except CommunicationError as exc:
             self._response_waiter.cancel()
             raise ManagementConnectionError("Error while sending Telegram") from exc
@@ -177,7 +177,7 @@ class P2PConnection:
         except ConfirmationError as exc:
             raise ManagementConnectionError(
                 f"Disconnect from {self.address} failed: {exc}"
-            )
+            ) from exc
         except CommunicationError as exc:
             raise ManagementConnectionError("Error while sending Telegram") from exc
         finally:
@@ -253,9 +253,11 @@ class P2PConnection:
             except asyncio.TimeoutError:
                 raise ManagementConnectionTimeout(
                     "No ACK received for repeated telegram."
-                )
+                ) from None
         except ConfirmationError as exc:
-            raise ManagementConnectionError(f"Error while sending Telegram: {exc}")
+            raise ManagementConnectionError(
+                f"Error while sending Telegram: {exc}"
+            ) from exc
         except CommunicationError as exc:
             raise ManagementConnectionError("Error while sending Telegram") from exc
         finally:
@@ -277,7 +279,9 @@ class P2PConnection:
                 self._response_waiter, timeout=MANAGAMENT_CONNECTION_TIMEOUT
             )
         except asyncio.TimeoutError:
-            raise ManagementConnectionTimeout()
+            raise ManagementConnectionTimeout(
+                f"Timeout while waiting for {expected_payload}"
+            ) from None
         finally:
             # set up new Future for the next request
             self._response_waiter = asyncio.get_event_loop().create_future()
diff --git a/xknx/telegram/apci.py b/xknx/telegram/apci.py
index 36a4e142..4f769780 100644
--- a/xknx/telegram/apci.py
+++ b/xknx/telegram/apci.py
@@ -420,7 +420,7 @@ class ADCRead(APCI):
 
     CODE = APCIService.ADC_READ
 
-    def __init__(self, channel: int = 0, count: int = 0) -> None:
+    def __init__(self, channel: int = 0, count: int = 1) -> None:
         """Initialize a new instance of ADCRead."""
         self.channel = channel
         self.count = count
@@ -495,7 +495,7 @@ class MemoryRead(APCI):
 
     CODE = APCIService.MEMORY_READ
 
-    def __init__(self, address: int = 0, count: int = 0) -> None:
+    def __init__(self, address: int = 0, count: int = 1) -> None:
         """Initialize a new instance of MemoryRead."""
         self.address = address
         self.count = count
@@ -538,7 +538,7 @@ class MemoryWrite(APCI):
     CODE = APCIService.MEMORY_WRITE
 
     def __init__(
-        self, address: int = 0, count: int = 0, data: bytes | None = None
+        self, address: int = 0, count: int = 1, data: bytes | None = None
     ) -> None:
         """Initialize a new instance of MemoryWrite."""
 
@@ -590,7 +590,7 @@ class MemoryResponse(APCI):
     CODE = APCIService.MEMORY_RESPONSE
 
     def __init__(
-        self, address: int = 0, count: int = 0, data: bytes | None = None
+        self, address: int = 0, count: int = 1, data: bytes | None = None
     ) -> None:
         """Initialize a new instance of MemoryResponse."""
 
@@ -745,7 +745,7 @@ class UserMemoryRead(APCI):
 
     CODE = APCIUserService.USER_MEMORY_READ
 
-    def __init__(self, address: int = 0, count: int = 0) -> None:
+    def __init__(self, address: int = 0, count: int = 1) -> None:
         """Initialize a new instance of UserMemoryRead."""
         self.address = address
         self.count = count
@@ -790,7 +790,7 @@ class UserMemoryWrite(APCI):
     CODE = APCIUserService.USER_MEMORY_WRITE
 
     def __init__(
-        self, address: int = 0, count: int = 0, data: bytes | None = None
+        self, address: int = 0, count: int = 1, data: bytes | None = None
     ) -> None:
         """Initialize a new instance of UserMemoryWrite."""
 
@@ -844,7 +844,7 @@ class UserMemoryResponse(APCI):
     CODE = APCIUserService.USER_MEMORY_RESPONSE
 
     def __init__(
-        self, address: int = 0, count: int = 0, data: bytes | None = None
+        self, address: int = 0, count: int = 1, data: bytes | None = None
     ) -> None:
         """Initialize a new instance of UserMemoryResponse."""
 
@@ -1094,7 +1094,7 @@ class AuthorizeRequest(APCI):
 
     def calculated_length(self) -> int:
         """Get length of APCI payload."""
-        return 5
+        return 6
 
     def from_knx(self, raw: bytes) -> None:
         """Parse/deserialize from KNX/IP raw data."""
@@ -1152,7 +1152,7 @@ class PropertyValueRead(APCI):
         self,
         object_index: int = 0,
         property_id: int = 0,
-        count: int = 0,
+        count: int = 1,
         start_index: int = 1,
     ) -> None:
         """Initialize a new instance of PropertyValueRead."""
@@ -1216,7 +1216,7 @@ class PropertyValueWrite(APCI):
         self,
         object_index: int = 0,
         property_id: int = 0,
-        count: int = 0,
+        count: int = 1,
         start_index: int = 0,
         data: bytes | None = None,
     ) -> None:
@@ -1295,7 +1295,7 @@ class PropertyValueResponse(APCI):
         self,
         object_index: int = 0,
         property_id: int = 0,
-        count: int = 0,
+        count: int = 1,
         start_index: int = 0,
         data: bytes | None = None,
     ) -> None:
@@ -1407,7 +1407,7 @@ class PropertyDescriptionResponse(APCI):
         property_id: int = 0,
         property_index: int = 0,
         type_: int = 0,
-        max_count: int = 0,
+        max_count: int = 1,
         access: int = 0,
     ) -> None:
         """Initialize a new instance of PropertyDescriptionRead."""


Test patch:
diff --git a/test/telegram_tests/apci_test.py b/test/telegram_tests/apci_test.py
index eb7706af..eecf0b93 100644
--- a/test/telegram_tests/apci_test.py
+++ b/test/telegram_tests/apci_test.py
@@ -1014,7 +1014,7 @@ class TestAuthorizeRequest:
         """Test the test_calculated_length method."""
         payload = AuthorizeRequest(key=12345678)
 
-        assert payload.calculated_length() == 5
+        assert payload.calculated_length() == 6
 
     def test_from_knx(self):
         """Test the from_knx method."""