Repository: XKNX/xknx
Pull request #950 (2022-05-13 07:11:33)

Fix Cover position updates
<!--
  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 travel time prediction when receiving updates from bus while moving

Fixes https://github.com/home-assistant/core/issues/71745

## 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
- [ ] The Homeassistant plugin has been adjusted in case of new config options


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

> :exclamation: Current head 75eea79 differs from pull request most recent head 3436bfd. Consider uploading reports for the commit 3436bfd to get more accurate results

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

```diff
@@           Coverage Diff           @@
##             main     #950   +/-   ##
=======================================
  Coverage   96.00%   96.00%           
=======================================
  Files         131      131           
  Lines        7900     7901    +1     
=======================================
+ Hits         7584     7585    +1     
  Misses        316      316           
```


| [Impacted Files](https://codecov.io/gh/XKNX/xknx/pull/950?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) | Coverage Δ | |
|---|---|---|
| [xknx/devices/travelcalculator.py](https://codecov.io/gh/XKNX/xknx/pull/950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9kZXZpY2VzL3RyYXZlbGNhbGN1bGF0b3IucHk=) | `95.45% <96.77%> (+0.05%)` | :arrow_up: |


Patch:
diff --git a/changelog.md b/changelog.md
index ee864abf..bd47ef59 100644
--- a/changelog.md
+++ b/changelog.md
@@ -2,6 +2,10 @@
 
 ## Unreleased changes
 
+### Devices
+
+- Cover: fix travel time prediction when receiving updates from bus while moving
+
 ### Protocol
 
 - Parse and encode different TPCI in a CEMIFrame or Telegram
diff --git a/xknx/devices/travelcalculator.py b/xknx/devices/travelcalculator.py
index eb9ccc0c..edcb4177 100644
--- a/xknx/devices/travelcalculator.py
+++ b/xknx/devices/travelcalculator.py
@@ -28,15 +28,14 @@ class TravelCalculator:
 
     def __init__(self, travel_time_down: float, travel_time_up: float) -> None:
         """Initialize TravelCalculator class."""
-        self.last_known_position: int | None = None
-        self.position_confirmed: bool = False
-
+        self.travel_direction = TravelStatus.STOPPED
         self.travel_time_down = travel_time_down
         self.travel_time_up = travel_time_up
 
-        self.travel_to_position: int | None = None
-        self.travel_started_time: float = 0.0
-        self.travel_direction = TravelStatus.STOPPED
+        self._last_known_position: int | None = None
+        self._last_known_postion_timestamp: float = 0.0
+        self._position_confirmed: bool = False
+        self._travel_to_position: int | None = None
 
         # 100 is closed, 0 is fully open
         self.position_closed: int = 100
@@ -44,38 +43,39 @@ class TravelCalculator:
 
     def set_position(self, position: int) -> None:
         """Set position and target of cover."""
-        self.travel_to_position = position
+        self._travel_to_position = position
         self.update_position(position)
 
     def update_position(self, position: int) -> None:
         """Update known position of cover."""
-        self.last_known_position = position
-        if position == self.travel_to_position:
-            self.position_confirmed = True
+        self._last_known_position = position
+        self._last_known_postion_timestamp = time.time()
+        if position == self._travel_to_position:
+            self._position_confirmed = True
 
     def stop(self) -> None:
         """Stop traveling."""
         stop_position = self.current_position()
         if stop_position is None:
             return
-        self.last_known_position = stop_position
-        self.travel_to_position = stop_position
-        self.position_confirmed = False
+        self._last_known_position = stop_position
+        self._travel_to_position = stop_position
+        self._position_confirmed = False
         self.travel_direction = TravelStatus.STOPPED
 
-    def start_travel(self, travel_to_position: int) -> None:
+    def start_travel(self, _travel_to_position: int) -> None:
         """Start traveling to position."""
-        if self.last_known_position is None:
-            self.set_position(travel_to_position)
+        if self._last_known_position is None:
+            self.set_position(_travel_to_position)
             return
         self.stop()
-        self.travel_started_time = time.time()
-        self.travel_to_position = travel_to_position
-        self.position_confirmed = False
+        self._last_known_postion_timestamp = time.time()
+        self._travel_to_position = _travel_to_position
+        self._position_confirmed = False
 
         self.travel_direction = (
             TravelStatus.DIRECTION_DOWN
-            if travel_to_position > self.last_known_position
+            if _travel_to_position > self._last_known_position
             else TravelStatus.DIRECTION_UP
         )
 
@@ -89,13 +89,13 @@ class TravelCalculator:
 
     def current_position(self) -> int | None:
         """Return current (calculated or known) position."""
-        if not self.position_confirmed:
+        if not self._position_confirmed:
             return self._calculate_position()
-        return self.last_known_position
+        return self._last_known_position
 
     def is_traveling(self) -> bool:
         """Return if cover is traveling."""
-        return self.current_position() != self.travel_to_position
+        return self.current_position() != self._travel_to_position
 
     def is_opening(self) -> bool:
         """Return if the cover is opening."""
@@ -111,7 +111,7 @@ class TravelCalculator:
 
     def position_reached(self) -> bool:
         """Return if cover has reached designated position."""
-        return self.current_position() == self.travel_to_position
+        return self.current_position() == self._travel_to_position
 
     def is_open(self) -> bool:
         """Return if cover is (fully) open."""
@@ -123,9 +123,9 @@ class TravelCalculator:
 
     def _calculate_position(self) -> int | None:
         """Return calculated position."""
-        if self.travel_to_position is None or self.last_known_position is None:
-            return self.last_known_position
-        relative_position = self.travel_to_position - self.last_known_position
+        if self._travel_to_position is None or self._last_known_position is None:
+            return self._last_known_position
+        relative_position = self._travel_to_position - self._last_known_position
 
         def position_reached_or_exceeded(relative_position: int) -> bool:
             """Return if designated position was reached."""
@@ -142,14 +142,14 @@ class TravelCalculator:
             return False
 
         if position_reached_or_exceeded(relative_position):
-            return self.travel_to_position
+            return self._travel_to_position
 
         travel_time = self._calculate_travel_time(relative_position)
-        if time.time() > self.travel_started_time + travel_time:
-            return self.travel_to_position
+        if time.time() > self._last_known_postion_timestamp + travel_time:
+            return self._travel_to_position
 
-        progress = (time.time() - self.travel_started_time) / travel_time
-        position = self.last_known_position + relative_position * progress
+        progress = (time.time() - self._last_known_postion_timestamp) / travel_time
+        position = self._last_known_position + relative_position * progress
         return int(position)
 
     def _calculate_travel_time(self, relative_position: int) -> float:


Test patch:
diff --git a/test/devices_tests/cover_test.py b/test/devices_tests/cover_test.py
index 70da444c..e17d0677 100644
--- a/test/devices_tests/cover_test.py
+++ b/test/devices_tests/cover_test.py
@@ -526,11 +526,11 @@ class TestCover:
             destination_address=GroupAddress("1/2/1"),
             payload=GroupValueWrite(DPTBinary(0)),
         )
-        assert cover.travelcalculator.travel_to_position == 50
+        assert cover.travelcalculator._travel_to_position == 50
         assert cover.is_opening()
         # process the outgoing telegram to make sure it doesn't overwrite the target position
         await cover.process(telegram)
-        assert cover.travelcalculator.travel_to_position == 50
+        assert cover.travelcalculator._travel_to_position == 50
         assert xknx.telegrams.qsize() == 0
 
     async def test_position_without_position_address_down(self):
@@ -551,11 +551,11 @@ class TestCover:
             destination_address=GroupAddress("1/2/1"),
             payload=GroupValueWrite(DPTBinary(1)),
         )
-        assert cover.travelcalculator.travel_to_position == 80
+        assert cover.travelcalculator._travel_to_position == 80
         assert cover.is_closing()
         # process the outgoing telegram to make sure it doesn't overwrite the target position
         await cover.process(telegram)
-        assert cover.travelcalculator.travel_to_position == 80
+        assert cover.travelcalculator._travel_to_position == 80
 
     async def test_position_without_position_address_uninitialized_up(self):
         """Test moving uninitialized cover to absolute position - with no absolute positioning supported."""
@@ -674,7 +674,7 @@ class TestCover:
         await cover.process(telegram)
         assert cover.current_position() == 16
         assert not cover.is_traveling()
-        assert cover.travelcalculator.travel_to_position == 16
+        assert cover.travelcalculator._travel_to_position == 16
         # new position - movement starts
         telegram = Telegram(
             GroupAddress("1/2/3"), payload=GroupValueWrite(DPTArray(255))
@@ -682,7 +682,7 @@ class TestCover:
         await cover.process(telegram)
         assert cover.current_position() == 16
         assert cover.is_closing()
-        assert cover.travelcalculator.travel_to_position == 100
+        assert cover.travelcalculator._travel_to_position == 100
         # new state while moving - movement goes on; travelcalculator updated
         telegram = Telegram(
             GroupAddress("1/2/4"), payload=GroupValueWrite(DPTArray(213))
@@ -690,7 +690,7 @@ class TestCover:
         await cover.process(telegram)
         assert cover.current_position() == 84
         assert cover.is_closing()
-        assert cover.travelcalculator.travel_to_position == 100
+        assert cover.travelcalculator._travel_to_position == 100
 
     async def test_process_angle(self):
         """Test process / reading telegrams from telegram queue. Test if position is processed correctly."""
diff --git a/test/devices_tests/travelcalculator_test.py b/test/devices_tests/travelcalculator_test.py
index ba63371f..f5960f7b 100644
--- a/test/devices_tests/travelcalculator_test.py
+++ b/test/devices_tests/travelcalculator_test.py
@@ -40,7 +40,7 @@ class TestTravelCalculator:
         assert travelcalculator.current_position() == 80
 
     def test_travel_down(self):
-        """Test travel up."""
+        """Test travel down."""
         travelcalculator = TravelCalculator(25, 50)
         with patch("time.time") as mock_time:
             mock_time.return_value = 1580000000.0
@@ -74,7 +74,7 @@ class TestTravelCalculator:
             assert travelcalculator.position_reached()
 
     def test_travel_up(self):
-        """Test travel down."""
+        """Test travel up."""
         travelcalculator = TravelCalculator(25, 50)
         with patch("time.time") as mock_time:
             mock_time.return_value = 1580000000.0
@@ -137,6 +137,62 @@ class TestTravelCalculator:
             assert travelcalculator.current_position() == 68
             assert travelcalculator.position_reached()
 
+    def test_travel_down_with_updates(self):
+        """Test travel down with position updates from bus."""
+        travelcalculator = TravelCalculator(25, 50)
+        with patch("time.time") as mock_time:
+            mock_time.return_value = 1580000000.0
+            travelcalculator.set_position(40)
+            travelcalculator.start_travel(100)  # 15 seconds to reach 100
+
+            # time not changed, still at beginning
+            assert travelcalculator.current_position() == 40
+            assert not travelcalculator.position_reached()
+            assert travelcalculator.travel_direction == TravelStatus.DIRECTION_DOWN
+
+            mock_time.return_value = 1580000002.0
+            assert travelcalculator.current_position() == 48
+            assert not travelcalculator.position_reached()
+            # update from bus matching calculation
+            travelcalculator.update_position(48)
+            assert travelcalculator.current_position() == 48
+            assert not travelcalculator.position_reached()
+
+            mock_time.return_value = 1580000010.0
+            assert travelcalculator.current_position() == 80
+            assert not travelcalculator.position_reached()
+            # update from bus not matching calculation takes precedence (1 second slower)
+            travelcalculator.update_position(76)
+            assert travelcalculator.current_position() == 76
+            assert not travelcalculator.position_reached()
+            # travel time extended by 1 second due to update from bus
+            mock_time.return_value = 1580000015.0
+            assert travelcalculator.current_position() == 96
+            assert not travelcalculator.position_reached()
+            mock_time.return_value = 1580000015.0 + 1
+            assert travelcalculator.current_position() == 100
+            assert travelcalculator.position_reached()
+
+    def test_travel_up_with_updates(self):
+        """Test travel up with position updates from bus."""
+        travelcalculator = TravelCalculator(25, 50)
+        with patch("time.time") as mock_time:
+            mock_time.return_value = 1580000000.0
+            travelcalculator.set_position(70)
+            travelcalculator.start_travel(50)  # 10 seconds to reach 50
+
+            mock_time.return_value = 1580000005.0
+            assert travelcalculator.current_position() == 60
+            assert not travelcalculator.position_reached()
+            # update from bus not matching calculation takes precedence (1 second faster)
+            travelcalculator.update_position(58)
+            assert travelcalculator.current_position() == 58
+            assert not travelcalculator.position_reached()
+            # position reached 1 second earlier than predicted
+            mock_time.return_value = 1580000010.0 - 1
+            assert travelcalculator.current_position() == 50
+            assert travelcalculator.position_reached()
+
     def test_change_direction(self):
         """Test changing direction while travelling."""
         travelcalculator = TravelCalculator(50, 25)