Repository: XKNX/xknx
Pull request #503 (2020-11-21 23:13:14)

process every incoming Telegram in all Devices
<!--
  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.
-->
process every incoming Telegram in all Devices regardless if a callback for the GA is registered (eg. StateUpdater) or not.

This will process Telegrams from the Device class even when a callback for this GA is registered ge. from StateUpdater. Some RemoteValues have no callbacks set (ClimateMode, Cover.position) from their Device so the state would not get initialized properly. 
Also devices that use same GAs will be updated each, instead only one.

I'm not sure why this even was prohibited before - maybe @Julius2342 can help me out here?


This will need some testing - especially for climate modes and cover positions (which I don't have in my installation).

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:

- [x] I have performed a self-review of my own code
- [ ] The documentation has been adjusted accordingly
- [x] The changes generate no new warnings
- [x] Tests have been added that prove the fix is effective or that the feature works
- [x] The changes are documented in the changelog
- [x] The Homeassistant plugin has been adjusted in case of new config options



Patch:
diff --git a/changelog.md b/changelog.md
index b53da94b..80084bbe 100644
--- a/changelog.md
+++ b/changelog.md
@@ -6,6 +6,10 @@
 
 - Light, Switch: initialize state with `None` instead of `False` to account for unknown state.
 
+### Internals
+
+- process every incoming Telegram in all Devices, regardless if a callback for the GA is registered (eg. StateUpdater) or not.
+
 ## 0.15.3 Opposite day! 2020-10-29
 
 ### Devices
diff --git a/examples/example_telegram_monitor.py b/examples/example_telegram_monitor.py
index b4399fb9..4ab3af4e 100644
--- a/examples/example_telegram_monitor.py
+++ b/examples/example_telegram_monitor.py
@@ -10,7 +10,6 @@ from xknx.telegram import AddressFilter
 async def telegram_received_cb(telegram):
     """Do something with the received telegram."""
     print(f"Telegram received: {telegram}")
-    return True
 
 
 def show_help():
diff --git a/home-assistant-plugin/custom_components/xknx/__init__.py b/home-assistant-plugin/custom_components/xknx/__init__.py
index ff5b272d..517b4ff1 100644
--- a/home-assistant-plugin/custom_components/xknx/__init__.py
+++ b/home-assistant-plugin/custom_components/xknx/__init__.py
@@ -326,8 +326,6 @@ class KNXModule:
             "knx_event",
             {"address": str(telegram.group_address), "data": telegram.payload.value},
         )
-        # False signals XKNX to proceed with processing telegrams.
-        return False
 
     async def service_send_to_knx_bus(self, call):
         """Service for sending an arbitrary KNX message to the KNX bus."""
diff --git a/xknx/core/telegram_queue.py b/xknx/core/telegram_queue.py
index 1e012563..b4b9a1b3 100644
--- a/xknx/core/telegram_queue.py
+++ b/xknx/core/telegram_queue.py
@@ -141,12 +141,7 @@ class TelegramQueue:
     async def process_telegram_incoming(self, telegram):
         """Process incoming telegram."""
         telegram_logger.debug(telegram)
-        processed = False
         for telegram_received_cb in self.telegram_received_cbs:
             if telegram_received_cb.is_within_filter(telegram):
-                ret = await telegram_received_cb.callback(telegram)
-                if ret:
-                    processed = True
-        # TODO: why don't we process every incoming telegram by device?
-        if not processed:
-            await self.xknx.devices.process(telegram)
+                await telegram_received_cb.callback(telegram)
+        await self.xknx.devices.process(telegram)
diff --git a/xknx/core/value_reader.py b/xknx/core/value_reader.py
index 5a2307d9..01f32334 100644
--- a/xknx/core/value_reader.py
+++ b/xknx/core/value_reader.py
@@ -53,17 +53,13 @@ class ValueReader:
 
     async def telegram_received(self, telegram):
         """Test if telegram has correct group address and trigger event."""
-        if telegram.telegramtype not in (
+        if telegram.group_address == self.group_address and telegram.telegramtype in (
             TelegramType.GROUP_RESPONSE,
             TelegramType.GROUP_WRITE,
         ):
-            return False
-        if self.group_address != telegram.group_address:
-            return False
-        self.success = True
-        self.received_telegram = telegram
-        self.response_received_or_timeout.set()
-        return True
+            self.success = True
+            self.received_telegram = telegram
+            self.response_received_or_timeout.set()
 
     def timeout(self):
         """Handle timeout for not having received expected group response."""
diff --git a/xknx/remote_value/remote_value.py b/xknx/remote_value/remote_value.py
index 6951ce47..abab2bbe 100644
--- a/xknx/remote_value/remote_value.py
+++ b/xknx/remote_value/remote_value.py
@@ -109,7 +109,7 @@ class RemoteValue:
         logger.warning("'to_knx()' not implemented for %s", self.__class__.__name__)
 
     async def process(self, telegram, always_callback=False):
-        """Process incoming telegram."""
+        """Process incoming or outgoing telegram."""
         if not self.has_group_address(telegram.group_address):
             return False
         if not self.payload_valid(telegram.payload):


Test patch:
diff --git a/test/core_tests/telegram_queue_test.py b/test/core_tests/telegram_queue_test.py
index e8826de1..299aa3b1 100644
--- a/test/core_tests/telegram_queue_test.py
+++ b/test/core_tests/telegram_queue_test.py
@@ -9,6 +9,14 @@ from xknx.exceptions import CouldNotParseTelegram
 from xknx.telegram import AddressFilter, GroupAddress, Telegram, TelegramDirection
 
 
+class AsyncMock(MagicMock):
+    """Async Mock."""
+
+    # pylint: disable=invalid-overridden-method
+    async def __call__(self, *args, **kwargs):
+        return super().__call__(*args, **kwargs)
+
+
 class TestTelegramQueue(unittest.TestCase):
     """Test class for telegram queue."""
 
@@ -126,15 +134,10 @@ class TestTelegramQueue(unittest.TestCase):
         """Test telegram_received_callback after state of switch was changed."""
         # pylint: disable=no-self-use
         xknx = XKNX()
-
-        telegram_received_callback = Mock()
-
-        async def async_telegram_received_cb(device):
-            """Async callback."""
-            telegram_received_callback(device)
+        async_telegram_received_callback = AsyncMock()
 
         callback = xknx.telegram_queue.register_telegram_received_cb(
-            async_telegram_received_cb
+            async_telegram_received_callback
         )
         xknx.telegram_queue.unregister_telegram_received_cb(callback)
 
@@ -146,7 +149,7 @@ class TestTelegramQueue(unittest.TestCase):
         self.loop.run_until_complete(
             xknx.telegram_queue.process_telegram_incoming(telegram)
         )
-        telegram_received_callback.assert_not_called()
+        async_telegram_received_callback.assert_not_called()
 
     #
     # TEST PROCESS
@@ -176,20 +179,16 @@ class TestTelegramQueue(unittest.TestCase):
         devices_by_ga_mock.assert_called_once_with(GroupAddress("1/2/3"))
         test_device.process.assert_called_once_with(telegram)
 
-    @patch("xknx.devices.Devices.devices_by_group_address")
-    def test_process_to_callback(self, devices_by_ga_mock):
-        """Test process_telegram_incoming for returning after processing callback."""
+    @patch("xknx.devices.Devices.process", new_callable=AsyncMock)
+    def test_process_to_callback(self, devices_process):
+        """Test process_telegram_incoming with callback."""
         # pylint: disable=no-self-use
         xknx = XKNX()
+        async_telegram_received_callback = AsyncMock()
 
-        telegram_received_callback = Mock()
-
-        async def async_telegram_received_cb(device):
-            """Async callback. Returning 'True'."""
-            telegram_received_callback(device)
-            return True
-
-        xknx.telegram_queue.register_telegram_received_cb(async_telegram_received_cb)
+        xknx.telegram_queue.register_telegram_received_cb(
+            async_telegram_received_callback
+        )
 
         telegram = Telegram(
             direction=TelegramDirection.INCOMING,
@@ -199,8 +198,8 @@ class TestTelegramQueue(unittest.TestCase):
         self.loop.run_until_complete(
             xknx.telegram_queue.process_telegram_incoming(telegram)
         )
-        telegram_received_callback.assert_called_once_with(telegram)
-        devices_by_ga_mock.assert_not_called()
+        async_telegram_received_callback.assert_called_once_with(telegram)
+        devices_process.assert_called_once_with(telegram)
 
     @patch("xknx.io.KNXIPInterface")
     @patch("logging.Logger.warning")
diff --git a/test/core_tests/value_reader_test.py b/test/core_tests/value_reader_test.py
index afbe35d5..85efd967 100644
--- a/test/core_tests/value_reader_test.py
+++ b/test/core_tests/value_reader_test.py
@@ -135,12 +135,18 @@ class TestValueReader(unittest.TestCase):
                 value_reader.telegram_received(test_telegram)
             )
 
-        self.assertFalse(async_telegram_received(telegram_wrong_address))
-        self.assertFalse(async_telegram_received(telegram_wrong_type))
+        async_telegram_received(telegram_wrong_address)
         self.assertIsNone(value_reader.received_telegram)
+        self.assertFalse(value_reader.success)
 
-        self.assertTrue(async_telegram_received(expected_telegram_1))
+        async_telegram_received(telegram_wrong_type)
+        self.assertIsNone(value_reader.received_telegram)
+        self.assertFalse(value_reader.success)
+
+        async_telegram_received(expected_telegram_1)
         self.assertEqual(value_reader.received_telegram, expected_telegram_1)
+        self.assertTrue(value_reader.success)
 
-        self.assertTrue(async_telegram_received(expected_telegram_2))
+        async_telegram_received(expected_telegram_2)
         self.assertEqual(value_reader.received_telegram, expected_telegram_2)
+        self.assertTrue(value_reader.success)