Repository: XKNX/xknx
Pull request #1205 (2023-02-26 19:48:57)

Move CEMI Frame parsing to CEMIHandler
<!--
  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.
-->


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)
- [x] Code quality improvements to existing code or addition of tests

## Checklist

- [ ] The documentation has been adjusted accordingly
- [x] 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/1205?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) Report
> Merging [#1205](https://codecov.io/gh/XKNX/xknx/pull/1205?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (061b42e) into [main](https://codecov.io/gh/XKNX/xknx/commit/d5496b9803616a051904ee2fe98108aa377d8180?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (d5496b9) will **decrease** coverage by `0.01%`.
> The diff coverage is `88.88%`.

> :exclamation: Current head 061b42e differs from pull request most recent head f6b8bf5. Consider uploading reports for the commit f6b8bf5 to get more accurate results

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

```diff
@@            Coverage Diff             @@
##             main    #1205      +/-   ##
==========================================
- Coverage   96.43%   96.43%   -0.01%     
==========================================
  Files         143      143              
  Lines        8995     8993       -2     
==========================================
- Hits         8674     8672       -2     
  Misses        321      321              
```


| [Impacted Files](https://codecov.io/gh/XKNX/xknx/pull/1205?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) | Coverage Δ | |
|---|---|---|
| [xknx/io/knxip\_interface.py](https://codecov.io/gh/XKNX/xknx/pull/1205?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9pby9rbnhpcF9pbnRlcmZhY2UucHk=) | `88.23% <75.00%> (ø)` | |
| [xknx/cemi/cemi\_handler.py](https://codecov.io/gh/XKNX/xknx/pull/1205?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9jZW1pL2NlbWlfaGFuZGxlci5weQ==) | `98.46% <87.50%> (-1.54%)` | :arrow_down: |
| [xknx/io/interface.py](https://codecov.io/gh/XKNX/xknx/pull/1205?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9pby9pbnRlcmZhY2UucHk=) | `100.00% <100.00%> (ø)` | |
| [xknx/io/routing.py](https://codecov.io/gh/XKNX/xknx/pull/1205?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9pby9yb3V0aW5nLnB5) | `92.06% <100.00%> (+0.39%)` | :arrow_up: |
| [xknx/io/tunnel.py](https://codecov.io/gh/XKNX/xknx/pull/1205?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9pby90dW5uZWwucHk=) | `85.65% <100.00%> (-0.23%)` | :arrow_down: |


Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index 5ba70d87..dd966293 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -15,6 +15,7 @@ nav_order: 2
 ### Internals
 
 - Lower log levels for unsupported Telegrams and add more information.
+- Move CEMIFrame parsing from Interface to CEMIHandler.
 
 # 2.5.0 Request IA 2023-02-14
 
diff --git a/xknx/cemi/cemi_handler.py b/xknx/cemi/cemi_handler.py
index b3e37969..f2cfbb4a 100644
--- a/xknx/cemi/cemi_handler.py
+++ b/xknx/cemi/cemi_handler.py
@@ -16,6 +16,7 @@ from xknx.exceptions import (
     ConfirmationError,
     ConversionError,
     DataSecureError,
+    UnsupportedCEMIMessage,
 )
 from xknx.secure.data_secure import DataSecure
 from xknx.secure.keyring import Keyring
@@ -80,20 +81,32 @@ class CEMIHandler:
                 f"L_DATA_CON Data Link Layer confirmation timed out for {cemi}"
             )
 
-    def handle_cemi_frame(self, cemi: CEMIFrame) -> None:
-        """Parse and handle incoming CEMI Frames."""
-        logger.debug("Incoming CEMI: %s", cemi)
+    def handle_raw_cemi(self, raw_cemi: bytes) -> None:
+        """Parse and handle incoming raw CEMI Frames."""
+        try:
+            cemi = CEMIFrame.from_knx(raw_cemi)
+        except UnsupportedCEMIMessage as unsupported_cemi_err:
+            logger.info("CEMI not supported: %s", unsupported_cemi_err)
+            return
+        self.handle_cemi_frame(cemi)
 
+    def handle_cemi_frame(self, cemi: CEMIFrame) -> None:
+        """Handle incoming CEMI Frames."""
         if cemi.code is CEMIMessageCode.L_DATA_CON:
             # L_DATA_CON confirmation frame signals ready to send next telegram
             self._l_data_confirmation_event.set()
+            logger.debug("Incoming CEMI confirmation: %s", cemi)
             return
         if cemi.code is CEMIMessageCode.L_DATA_REQ:
             # L_DATA_REQ frames should only be outgoing.
             logger.warning("Received unexpected L_DATA_REQ frame: %s", cemi)
             return
+        if cemi.src_addr == self.xknx.current_address:
+            # L_DATA_IND frames from our own address should be ignored (may occur form routing)
+            logger.debug("Ignoring own CEMI: %s", cemi)
+            return
+        logger.debug("Incoming CEMI: %s", cemi)
 
-        # TODO: do we have to decrypt Data Secure L_DATA_CON?
         if self.data_secure is not None:
             try:
                 cemi = self.data_secure.received_cemi(cemi=cemi)
diff --git a/xknx/io/interface.py b/xknx/io/interface.py
index 4a4bb790..aa736489 100644
--- a/xknx/io/interface.py
+++ b/xknx/io/interface.py
@@ -12,7 +12,7 @@ from typing import Callable
 
 from xknx.cemi import CEMIFrame
 
-CEMICallbackType = Callable[[CEMIFrame], None]
+CEMICallbackType = Callable[[bytes], None]
 
 
 class Interface(ABC):
diff --git a/xknx/io/knxip_interface.py b/xknx/io/knxip_interface.py
index b81aa3ab..6bf85fa9 100644
--- a/xknx/io/knxip_interface.py
+++ b/xknx/io/knxip_interface.py
@@ -460,12 +460,14 @@ class KNXIPInterface:
             await self._interface.disconnect()
             self._interface = None
 
-    def cemi_received(self, cemi: CEMIFrame) -> None:
-        """Pass CEMIFrame to CEMIHandler. Callback for having received CEMIFrames."""
-        self.xknx.cemi_handler.handle_cemi_frame(cemi)
+    def cemi_received(self, raw_cemi: bytes) -> None:
+        """Pass raw CEMIFrame data to CEMIHandler. Callback for having received CEMIFrames."""
+        self.xknx.cemi_handler.handle_raw_cemi(raw_cemi)
 
     async def send_cemi(self, cemi: CEMIFrame) -> None:
         """Send CEMIFrame to connected device (either Tunneling or Routing)."""
+        # to ease converting L_Data.req CEMI frames to L_Data.ind and local confirmation
+        # in routing we pass `CEMIFrame` from the CEMIHandler here, instead of raw bytes
         if self._interface is None:
             raise CommunicationError("KNX/IP interface not connected")
         return await self._interface.send_cemi(cemi)
@@ -539,9 +541,9 @@ class KNXIPInterfaceThreaded(KNXIPInterface):
         self._thread_loop.call_soon_threadsafe(self._thread_loop.stop)
         self.connection_thread.join()
 
-    def cemi_received(self, cemi: CEMIFrame) -> None:
+    def cemi_received(self, raw_cemi: bytes) -> None:
         """Pass CEMIFrame to CEMIHandler. Callback for having received CEMIFrames."""
-        self._main_loop.call_soon_threadsafe(super().cemi_received, cemi)
+        self._main_loop.call_soon_threadsafe(super().cemi_received, raw_cemi)
 
     async def send_cemi(self, cemi: CEMIFrame) -> None:
         """Send CEMIFrame to connected device (either Tunneling or Routing)."""
diff --git a/xknx/io/routing.py b/xknx/io/routing.py
index bf8dd7a1..8347db43 100644
--- a/xknx/io/routing.py
+++ b/xknx/io/routing.py
@@ -14,7 +14,7 @@ from typing import TYPE_CHECKING, Final
 
 from xknx.cemi import CEMIFrame, CEMIMessageCode
 from xknx.core import XknxConnectionState
-from xknx.exceptions import CommunicationError, UnsupportedCEMIMessage
+from xknx.exceptions import CommunicationError
 from xknx.knxip import (
     HPAI,
     KNXIPFrame,
@@ -226,7 +226,7 @@ class Routing(Interface):
             self._send_knxipframe(KNXIPFrame.init_from_body(routing_indication))
 
         cemi.code = CEMIMessageCode.L_DATA_CON
-        self.cemi_received_callback(cemi)
+        self.cemi_received_callback(cemi.to_knx())
 
     def _send_knxipframe(self, knxipframe: KNXIPFrame) -> None:
         """Send KNXIPFrame to connected routing device."""
@@ -257,15 +257,7 @@ class Routing(Interface):
 
     def _handle_routing_indication(self, routing_indication: RoutingIndication) -> None:
         """Handle incoming RoutingIndication."""
-        try:
-            cemi = CEMIFrame.from_knx(routing_indication.raw_cemi)
-        except UnsupportedCEMIMessage as unsupported_cemi_err:
-            logger.info("CEMI not supported: %s", unsupported_cemi_err)
-            return
-        if cemi.src_addr == self.individual_address:
-            logger.debug("Ignoring own packet %s", cemi)
-            return
-        self.cemi_received_callback(cemi)
+        self.cemi_received_callback(routing_indication.raw_cemi)
 
 
 class SecureRouting(Routing):
diff --git a/xknx/io/tunnel.py b/xknx/io/tunnel.py
index 1739da36..7ae8ae98 100644
--- a/xknx/io/tunnel.py
+++ b/xknx/io/tunnel.py
@@ -12,11 +12,7 @@ from typing import TYPE_CHECKING
 
 from xknx.cemi import CEMIFrame
 from xknx.core import XknxConnectionState
-from xknx.exceptions import (
-    CommunicationError,
-    TunnellingAckError,
-    UnsupportedCEMIMessage,
-)
+from xknx.exceptions import CommunicationError, TunnellingAckError
 from xknx.knxip import (
     HPAI,
     ConnectRequestInformation,
@@ -313,12 +309,7 @@ class _Tunnel(Interface):
         self, tunneling_request: TunnellingRequest
     ) -> None:
         """Handle incoming tunnel request."""
-        try:
-            cemi = CEMIFrame.from_knx(tunneling_request.raw_cemi)
-        except UnsupportedCEMIMessage as unsupported_cemi_err:
-            logger.info("CEMI not supported: %s", unsupported_cemi_err)
-            return
-        self.cemi_received_callback(cemi)
+        self.cemi_received_callback(tunneling_request.raw_cemi)
 
     def _disconnect_request_received(
         self, disconnect_request: DisconnectRequest


Test patch:
diff --git a/test/cemi_tests/cemi_handler_test.py b/test/cemi_tests/cemi_handler_test.py
index 5b5aaa80..c6426f7e 100644
--- a/test/cemi_tests/cemi_handler_test.py
+++ b/test/cemi_tests/cemi_handler_test.py
@@ -111,3 +111,41 @@ def test_incoming_management_telegram(telegram):
         xknx.cemi_handler.handle_cemi_frame(test_cemi)
         mock_management_process.assert_called_once()
         assert xknx.telegrams.qsize() == 0
+
+
+@pytest.mark.parametrize(
+    "raw",
+    [
+        # LDataInd Unsupported Extended APCI from 0.0.1 to 0/0/0 broadcast
+        # <UnsupportedCEMIMessage description="APCI not supported: 0b1111111000 in CEMI: 2900b0d0000100000103f8" />
+        bytes.fromhex("2900b0d0000100000103f8"),
+        # <UnsupportedCEMIMessage description="CEMI too small. Length: 9; CEMI: 2900b06010fa10ff00" />
+        # communication_channel_id: 0x02   sequence_counter: 0x81
+        bytes.fromhex("2900b06010fa10ff00"),
+    ],
+)
+def test_invalid_cemi(raw):
+    """Test incoming invalid CEMI Frames."""
+    xknx = XKNX()
+
+    with patch("logging.Logger.info") as mock_info, patch.object(
+        xknx.cemi_handler, "handle_cemi_frame"
+    ) as mock_handle_cemi_frame:
+        xknx.cemi_handler.handle_raw_cemi(raw)
+        mock_info.assert_called_once()
+        mock_handle_cemi_frame.assert_not_called()
+
+
+def test_incoming_from_self():
+    """Test incoming CEMI from own IA."""
+    xknx = XKNX()
+    xknx.current_address = IndividualAddress("1.1.22")
+    # L_Data.ind GroupValueWrite from 1.1.22 to to 5/1/22 with DPT9 payload 0C 3F
+    raw = bytes.fromhex("2900bcd011162916030080 0c 3f")
+
+    with patch("logging.Logger.debug") as mock_debug, patch.object(
+        xknx.cemi_handler, "telegram_received"
+    ) as mock_telegram_received:
+        xknx.cemi_handler.handle_raw_cemi(raw)
+        mock_debug.assert_called_once()
+        mock_telegram_received.assert_not_called()
diff --git a/test/io_tests/routing_test.py b/test/io_tests/routing_test.py
index eadb84ea..e81ff743 100644
--- a/test/io_tests/routing_test.py
+++ b/test/io_tests/routing_test.py
@@ -64,23 +64,6 @@ class TestRouting:
         ]
         await asyncio.sleep(0)  # await local L_Data.con
 
-    @patch("logging.Logger.info")
-    async def test_routing_indication_received_apci_unsupported(self, logging_mock):
-        """Test Tunnel sending ACK for unsupported frames."""
-        routing = Routing(
-            self.xknx,
-            individual_address=None,
-            cemi_received_callback=self.cemi_received_mock,
-            local_ip="192.168.1.1",
-        )
-        # LDataInd Unsupported Extended APCI from 0.0.1 to 0/0/0 broadcast
-        # <UnsupportedCEMIMessage description="APCI not supported: 0b1111111000 in CEMI: 2900b0d0000100000103f8" />
-        raw = bytes.fromhex("0610 0530 0010 2900b0d0000100000103f8")
-
-        routing.transport.data_received_callback(raw, ("192.168.1.2", 3671))
-        self.cemi_received_mock.assert_not_called()
-        logging_mock.assert_called_once()
-
     @patch("logging.Logger.warning")
     async def test_routing_lost_message(self, logging_mock):
         """Test class for received RoutingLostMessage frames."""
diff --git a/test/io_tests/tunnel_test.py b/test/io_tests/tunnel_test.py
index 54fe6180..df5da6fe 100644
--- a/test/io_tests/tunnel_test.py
+++ b/test/io_tests/tunnel_test.py
@@ -68,17 +68,20 @@ class TestUDPTunnel:
             # L_Data.ind T_Connect from 1.0.250 to 1.0.255 (xknx tunnel endpoint)
             # communication_channel_id: 0x02   sequence_counter: 0x21
             bytes.fromhex("0610 0420 0014 04 02 21 00 2900b06010fa10ff0080"),
+            # <UnsupportedCEMIMessage description="CEMI too small. Length: 9; CEMI: 2900b06010fa10ff00" />
+            # communication_channel_id: 0x02   sequence_counter: 0x21
+            bytes.fromhex("0610 0420 0013 04 02 21 00 2900b06010fa10ff00"),
         ],
     )
     @patch("xknx.io.UDPTunnel._send_tunnelling_ack")
     async def test_tunnel_request_received(self, send_ack_mock, raw):
-        """Test Tunnel for calling send_ack on normal frames."""
-        test_cemi = CEMIFrame.from_knx(raw[10:])
+        """Test Tunnel for calling send_ack on frames."""
+        raw_cemi = raw[10:]
         self.tunnel.expected_sequence_number = 0x21
 
         self.tunnel.transport.data_received_callback(raw, ("192.168.1.2", 3671))
         await asyncio.sleep(0)
-        self.cemi_received_mock.assert_called_once_with(test_cemi)
+        self.cemi_received_mock.assert_called_once_with(raw_cemi)
         send_ack_mock.assert_called_once_with(raw[7], raw[8])
 
     @patch("xknx.io.UDPTunnel._send_tunnelling_ack")
@@ -167,31 +170,6 @@ class TestUDPTunnel:
         assert self.tunnel.expected_sequence_number == 11
         assert self.cemi_received_mock.call_count == 1
 
-    @patch("xknx.io.UDPTunnel._send_tunnelling_ack")
-    def test_tunnel_request_received_cemi_too_small(self, send_ack_mock):
-        """Test Tunnel sending ACK for unsupported frames."""
-        # <UnsupportedCEMIMessage description="CEMI too small. Length: 9; CEMI: 2900b06010fa10ff00" />
-        # communication_channel_id: 0x02   sequence_counter: 0x81
-        raw = bytes.fromhex("0610 0420 0013 04 02 81 00 2900b06010fa10ff00")
-        self.tunnel.expected_sequence_number = 0x81
-
-        self.tunnel.transport.data_received_callback(raw, ("192.168.1.2", 3671))
-        self.cemi_received_mock.assert_not_called()
-        send_ack_mock.assert_called_once_with(0x02, 0x81)
-
-    @patch("xknx.io.UDPTunnel._send_tunnelling_ack")
-    def test_tunnel_request_received_apci_unsupported(self, send_ack_mock):
-        """Test Tunnel sending ACK for unsupported frames."""
-        # LDataInd Unsupported Extended APCI from 0.0.1 to 0/0/0 broadcast
-        # <UnsupportedCEMIMessage description="APCI not supported: 0b1111111000 in CEMI: 2900b0d0000100000103f8" />
-        # communication_channel_id: 0x02   sequence_counter: 0x4f
-        raw = bytes.fromhex("0610 0420 0015 04 02 4f 00 2900b0d0000100000103f8")
-        self.tunnel.expected_sequence_number = 0x4F
-
-        self.tunnel.transport.data_received_callback(raw, ("192.168.1.2", 3671))
-        self.cemi_received_mock.assert_not_called()
-        send_ack_mock.assert_called_once_with(0x02, 0x4F)
-
     async def test_tunnel_send_retry(self, time_travel):
         """Test tunnel resends the telegram when no ACK was received."""
         self.tunnel.transport.send = Mock()