Repository: XKNX/xknx
Pull request #906 (2022-04-03 21:34:37)

Use XKNX `state_updater` argument to set default method for StateUpdater
<!--
  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.
-->
Use XKNX `state_updater` argument to set default method for StateUpdater. StateUpdater is always started - Device / RemoteValue can always opt in to use it, even if default is `False`.

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)
- [x] 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
- [x] The documentation has been adjusted accordingly
- [ ] 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
- [ ] The Homeassistant plugin has been adjusted in case of new config options


codecov[bot]: # [Codecov](https://codecov.io/gh/XKNX/xknx/pull/906?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) Report
> Merging [#906](https://codecov.io/gh/XKNX/xknx/pull/906?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (329f026) into [main](https://codecov.io/gh/XKNX/xknx/commit/78b12f8c322396f83c738920b38479a4cd296099?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (78b12f8) will **decrease** coverage by `0.01%`.
> The diff coverage is `96.15%`.

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

```diff
@@            Coverage Diff             @@
##             main     #906      +/-   ##
==========================================
- Coverage   95.62%   95.60%   -0.02%     
==========================================
  Files         128      128              
  Lines        7672     7687      +15     
==========================================
+ Hits         7336     7349      +13     
- Misses        336      338       +2     
```


| [Impacted Files](https://codecov.io/gh/XKNX/xknx/pull/906?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) | Coverage Δ | |
|---|---|---|
| [xknx/core/state\_updater.py](https://codecov.io/gh/XKNX/xknx/pull/906/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9jb3JlL3N0YXRlX3VwZGF0ZXIucHk=) | `83.33% <95.74%> (+0.31%)` | :arrow_up: |
| [xknx/remote\_value/remote\_value.py](https://codecov.io/gh/XKNX/xknx/pull/906/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%> (ø)` | |
| [xknx/xknx.py](https://codecov.io/gh/XKNX/xknx/pull/906/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC94a254LnB5) | `88.65% <100.00%> (-0.12%)` | :arrow_down: |

farmio: well, coverage is *kind of green* 😬

Patch:
diff --git a/changelog.md b/changelog.md
index 38110266..aef5dcc2 100644
--- a/changelog.md
+++ b/changelog.md
@@ -5,6 +5,7 @@
 ### Features
 
 - Add support for SearchRequestExtended to find interfaces that allow IP Secure
+- Use XKNX `state_updater` argument to set default method for StateUpdater. StateUpdater is always started - Device / RemoteValue can always opt in to use it, even if default is `False`.
 
 ### Bug fixes
 
diff --git a/docs/xknx.md b/docs/xknx.md
index dc16d06e..65bde554 100644
--- a/docs/xknx.md
+++ b/docs/xknx.md
@@ -49,7 +49,7 @@ The constructor of the XKNX object takes several parameters:
 - `multicast_group` is the multicast IP address - can be used to override the default multicast address (`224.0.23.12`)
 - `multicast_port` is the multicast port - can be used to override the default multicast port (`3671`)
 - `log_directory` is the path to the log directory - when set to a valid directory we log to a dedicated file in this directory called `xknx.log`. The log files are rotated each night and will exist for 7 days. After that the oldest one will be deleted.
-- if `state_updater` is set, XKNX will start (once `start() is called) an asynchronous process for syncing the states of all connected devices every hour
+- `state_updater` is used to set the default state-updating mechanism used by devices. `False` to  disable state-updating by default, `True` to use default 60 minutes expire-interval, a number between 2 to 1440 to configure expire-time or a string "expire 50", "every 90" for strict periodically update or "init" for update when a connection is established. Default: `False`.
 - if `daemon_mode` is set, start will only stop if Control-X is pressed. This function is useful for using XKNX as a daemon, e.g. for using the callback functions or using the internal action logic.
 - `connection_config` replaces a ConnectionConfig() that was read from a yaml config file.
 
diff --git a/xknx/core/state_updater.py b/xknx/core/state_updater.py
index 616cc5d3..d8d33182 100644
--- a/xknx/core/state_updater.py
+++ b/xknx/core/state_updater.py
@@ -4,7 +4,7 @@ from __future__ import annotations
 import asyncio
 from enum import Enum
 import logging
-from typing import TYPE_CHECKING, Any, Awaitable, Callable
+from typing import TYPE_CHECKING, Any, Awaitable, Callable, NamedTuple, Union
 
 from xknx.core import XknxConnectionState
 from xknx.remote_value import RemoteValue
@@ -19,67 +19,116 @@ DEFAULT_UPDATE_INTERVAL = 60
 MAX_UPDATE_INTERVAL = 1440
 
 
+class TrackerOptions(NamedTuple):
+    """Options for the state tracker."""
+
+    tracker_type: StateTrackerType
+    update_interval_min: int | float
+
+
+TrackerOptionType = Union[bool, int, float, str, TrackerOptions]
+
+
 class StateUpdater:
     """Class for keeping the states of RemoteValues up to date."""
 
-    def __init__(self, xknx: XKNX, parallel_reads: int = 2):
+    def __init__(
+        self,
+        xknx: XKNX,
+        default_tracker_option: TrackerOptionType,
+        parallel_reads: int = 2,
+    ):
         """Initialize StateUpdater class."""
         self.xknx = xknx
         self.started = False
         self._workers: dict[int, _StateTracker] = {}
         self._semaphore = asyncio.Semaphore(value=parallel_reads)
 
-    def register_remote_value(
-        self,
-        remote_value: RemoteValue[Any, Any],
-        tracker_options: bool | int | float | str = True,
-    ) -> None:
-        """Register a RemoteValue to initialize its state and/or track for expiration."""
+        # used to determine if a RemoteValue shall register a tracker by default
+        self.default_use_updater = bool(default_tracker_option)
+        # set default before using it in `parse_tracker_options()`
+        self._default_tracker_option = TrackerOptions(
+            tracker_type=StateTrackerType.EXPIRE,
+            update_interval_min=DEFAULT_UPDATE_INTERVAL,
+        )
+        #  Default options will be used if instantiated with `True` or `False`.
+        self._default_tracker_option = self.parse_tracker_options(
+            default_tracker_option, "default configuration"
+        )
 
-        def parse_tracker_options(
-            tracker_options: bool | int | float | str,
-        ) -> tuple[StateTrackerType, int | float]:
-            """Parse tracker type and expiration time."""
-            tracker_type = StateTrackerType.EXPIRE
-            update_interval: int | float = DEFAULT_UPDATE_INTERVAL
-
-            if isinstance(tracker_options, bool):
-                # `True` would be overwritten by the check for `int`
-                return (tracker_type, update_interval)
-            if isinstance(tracker_options, (int, float)):
-                update_interval = tracker_options
-            elif isinstance(tracker_options, str):
-                _options = tracker_options.split()
-                if _options[0].upper() == "INIT":
-                    tracker_type = StateTrackerType.INIT
-                elif _options[0].upper() == "EXPIRE":
-                    tracker_type = StateTrackerType.EXPIRE
-                elif _options[0].upper() == "EVERY":
-                    tracker_type = StateTrackerType.PERIODICALLY
-                else:
-                    logger.warning(
-                        'Could not parse StateUpdater tracker_options "%s" for %s. Using default %s %s minutes.',
-                        tracker_options,
-                        remote_value,
-                        tracker_type,
-                        update_interval,
-                    )
-                    return (tracker_type, update_interval)
-                try:
-                    if _options[1].isdigit():
-                        update_interval = int(_options[1])
-                except IndexError:
-                    pass  # No time given (no _options[1])
+    def parse_tracker_options(
+        self,
+        tracker_options: TrackerOptionType,
+        tracker_name: str,
+    ) -> TrackerOptions:
+        """Parse tracker type and expiration time."""
 
+        def check_update_interval(update_interval: int | float) -> int | float:
+            """Return valid update interval."""
             if update_interval > MAX_UPDATE_INTERVAL:
                 logger.warning(
                     "StateUpdater interval of %s to long for %s. Using maximum of %s minutes (1 day)",
                     tracker_options,
-                    remote_value,
+                    tracker_name,
                     MAX_UPDATE_INTERVAL,
                 )
-                update_interval = MAX_UPDATE_INTERVAL
-            return (tracker_type, update_interval)
+                return MAX_UPDATE_INTERVAL
+            if update_interval < 1:
+                logger.warning(
+                    "StateUpdater interval of %s to short for %s. Using minimum of 1 minute",
+                    tracker_options,
+                    tracker_name,
+                )
+                return 1
+            return update_interval
+
+        if isinstance(tracker_options, TrackerOptions):
+            return TrackerOptions(
+                tracker_type=tracker_options.tracker_type,
+                update_interval_min=check_update_interval(
+                    tracker_options.update_interval_min
+                ),
+            )
+        if isinstance(tracker_options, bool):
+            # `True` would be overwritten by the check for `int`
+            return self._default_tracker_option
+
+        tracker_type = self._default_tracker_option.tracker_type
+        update_interval: int | float = self._default_tracker_option.update_interval_min
+
+        if isinstance(tracker_options, (int, float)):
+            update_interval = check_update_interval(tracker_options)
+        elif isinstance(tracker_options, str):
+            _options = tracker_options.split()
+            if _options[0].upper() == "INIT":
+                tracker_type = StateTrackerType.INIT
+            elif _options[0].upper() == "EXPIRE":
+                tracker_type = StateTrackerType.EXPIRE
+            elif _options[0].upper() == "EVERY":
+                tracker_type = StateTrackerType.PERIODICALLY
+            else:
+                logger.warning(
+                    'Could not parse StateUpdater tracker_options "%s" for %s. Using default %s %s minutes.',
+                    tracker_options,
+                    tracker_name,
+                    tracker_type,
+                    update_interval,
+                )
+                return TrackerOptions(tracker_type, update_interval)
+            try:
+                if _options[1].isdigit():
+                    update_interval = check_update_interval(int(_options[1]))
+            except IndexError:
+                pass  # No time given (no _options[1])
+
+        return TrackerOptions(tracker_type, update_interval)
+
+    def register_remote_value(
+        self,
+        remote_value: RemoteValue[Any, Any],
+        tracker_options: TrackerOptionType = True,
+    ) -> None:
+        """Register a RemoteValue to initialize its state and/or track for expiration."""
 
         async def read_state_mutex() -> None:
             """Schedule to read the state from the KNX bus - one at a time."""
@@ -96,18 +145,17 @@ class StateUpdater:
                 # ValueReader leaving the telegram_received_cb until next telegram
                 await asyncio.shield(remote_value.read_state(wait_for_result=True))
 
-        tracker_type, update_interval = parse_tracker_options(tracker_options)
+        tracker_options = self.parse_tracker_options(tracker_options, str(remote_value))
         tracker = _StateTracker(
             read_state_awaitable=read_state_mutex,
-            tracker_type=tracker_type,
-            interval_min=update_interval,
+            tracker_options=tracker_options,
         )
         self._workers[id(remote_value)] = tracker
 
         logger.debug(
             "StateUpdater registered %s %s for %s",
-            tracker_type,
-            update_interval,
+            tracker_options.tracker_type,
+            tracker_options.update_interval_min,
             remote_value,
         )
         if self.started:
@@ -157,13 +205,12 @@ class StateUpdater:
         self, state: XknxConnectionState
     ) -> None:
         """Start and stop StateUpdater via connection state update."""
-        if state == XknxConnectionState.CONNECTED and not self.started:
-            self._start()
-        elif (
-            state in (XknxConnectionState.DISCONNECTED, XknxConnectionState.CONNECTING)
-            and self.started
-        ):
-            self._stop()
+        if state == XknxConnectionState.CONNECTED:
+            if not self.started:
+                self._start()
+        else:
+            if self.started:
+                self._stop()
 
 
 class StateTrackerType(Enum):
@@ -180,12 +227,11 @@ class _StateTracker:
     def __init__(
         self,
         read_state_awaitable: Callable[[], Awaitable[None]],
-        tracker_type: StateTrackerType = StateTrackerType.EXPIRE,
-        interval_min: float = 60,
+        tracker_options: TrackerOptions,
     ):
         """Initialize StateTracker class."""
-        self.tracker_type = tracker_type
-        self.update_interval = interval_min * 60
+        self.tracker_type = tracker_options.tracker_type
+        self.update_interval = tracker_options.update_interval_min * 60
         self._read_state = read_state_awaitable
         self._task: asyncio.Task[None] | None = None
 
diff --git a/xknx/remote_value/remote_value.py b/xknx/remote_value/remote_value.py
index 373d1da9..7cda8829 100644
--- a/xknx/remote_value/remote_value.py
+++ b/xknx/remote_value/remote_value.py
@@ -42,7 +42,7 @@ class RemoteValue(ABC, Generic[DPTPayloadT, ValueT]):
         xknx: XKNX,
         group_address: GroupAddressesType | None = None,
         group_address_state: GroupAddressesType | None = None,
-        sync_state: bool | int | float | str = True,
+        sync_state: None | bool | int | float | str = None,
         device_name: str | None = None,
         feature_name: str | None = None,
         after_update_cb: AsyncCallbackType | None = None,
@@ -72,6 +72,8 @@ class RemoteValue(ABC, Generic[DPTPayloadT, ValueT]):
         self.telegram: Telegram | None = None
         self.after_update_cb: AsyncCallbackType | None = after_update_cb
 
+        if sync_state is None:
+            sync_state = xknx.state_updater.default_use_updater
         if sync_state and self.group_address_state:
             self.xknx.state_updater.register_remote_value(
                 self, tracker_options=sync_state
diff --git a/xknx/xknx.py b/xknx/xknx.py
index 75f9ad51..960815ed 100644
--- a/xknx/xknx.py
+++ b/xknx/xknx.py
@@ -12,11 +12,11 @@ from typing import Awaitable, Callable
 
 from xknx.core import (
     ConnectionManager,
-    StateUpdater,
     TaskRegistry,
     TelegramQueue,
     XknxConnectionState,
 )
+from xknx.core.state_updater import StateUpdater, TrackerOptionType
 from xknx.devices import Device, Devices
 from xknx.io import (
     DEFAULT_MCAST_GRP,
@@ -50,7 +50,7 @@ class XKNX:
         multicast_group: str = DEFAULT_MCAST_GRP,
         multicast_port: int = DEFAULT_MCAST_PORT,
         log_directory: str | None = None,
-        state_updater: bool = False,
+        state_updater: TrackerOptionType = False,
         daemon_mode: bool = False,
         connection_config: ConnectionConfig = ConnectionConfig(),
     ) -> None:
@@ -59,10 +59,9 @@ class XKNX:
         self.telegrams: asyncio.Queue[Telegram | None] = asyncio.Queue()
         self.sigint_received = asyncio.Event()
         self.telegram_queue = TelegramQueue(self)
-        self.state_updater = StateUpdater(self)
+        self.state_updater = StateUpdater(self, default_tracker_option=state_updater)
         self.connection_manager = ConnectionManager()
         self.task_registry = TaskRegistry(self)
-        self.start_state_updater = state_updater
         self.knxip_interface: KNXIPInterface | None = None
         self.started = asyncio.Event()
         self.address_format = address_format
@@ -126,8 +125,7 @@ class XKNX:
         )
         await self.knxip_interface.start()
         await self.telegram_queue.start()
-        if self.start_state_updater:
-            self.state_updater.start()
+        self.state_updater.start()
         self.started.set()
         if self.daemon_mode:
             await self.loop_until_sigint()


Test patch:
diff --git a/test/core_tests/state_updater_test.py b/test/core_tests/state_updater_test.py
index 6ac0d767..6d532afa 100644
--- a/test/core_tests/state_updater_test.py
+++ b/test/core_tests/state_updater_test.py
@@ -1,6 +1,8 @@
 """Unit test for StateUpdater."""
 from unittest.mock import Mock, patch
 
+import pytest
+
 from xknx import XKNX
 from xknx.core import XknxConnectionState
 from xknx.core.state_updater import StateTrackerType, _StateTracker
@@ -115,7 +117,7 @@ class TestStateUpdater:
         logging_warning_mock.assert_called_once_with(
             'Could not parse StateUpdater tracker_options "%s" for %s. Using default %s %s minutes.',
             "invalid",
-            remote_value_invalid,
+            str(remote_value_invalid),
             StateTrackerType.EXPIRE,
             60,
         )
@@ -130,7 +132,7 @@ class TestStateUpdater:
         logging_warning_mock.assert_called_once_with(
             "StateUpdater interval of %s to long for %s. Using maximum of %s minutes (1 day)",
             1441,
-            remote_value_long,
+            str(remote_value_long),
             1440,
         )
         remote_value_long.__del__()
@@ -195,3 +197,32 @@ class TestStateUpdater:
         )
 
         assert xknx.state_updater.started
+
+    @pytest.mark.parametrize(
+        "default,sync_state_value,expected_interval,expected_tracker_type",
+        [
+            (90, True, 90, StateTrackerType.EXPIRE),
+            (False, True, 60, StateTrackerType.EXPIRE),
+            (True, None, 60, StateTrackerType.EXPIRE),
+            (40, None, 40, StateTrackerType.EXPIRE),
+            ("every 70", None, 70, StateTrackerType.PERIODICALLY),
+            ("init", True, 60, StateTrackerType.INIT),
+            ("every 80", "expire 20", 20, StateTrackerType.EXPIRE),
+        ],
+    )
+    async def test_stat_updater_default(
+        self, default, sync_state_value, expected_interval, expected_tracker_type
+    ):
+        """Test setting a default for StateUpdater."""
+        xknx = XKNX(state_updater=default)
+        remote_value = RemoteValue(
+            xknx, sync_state=sync_state_value, group_address_state=GroupAddress("1/1/1")
+        )
+        assert (
+            xknx.state_updater._workers[id(remote_value)].update_interval
+            == expected_interval * 60
+        )
+        assert (
+            xknx.state_updater._workers[id(remote_value)].tracker_type
+            == expected_tracker_type
+        )