update current position from bus while moving

changelog.md

@@ -26,9 +26,11 @@
 - enable multicast on macOS and fix a bug where unknown cemi frames raise a TypeError on routing connections
 - BinarySensor: reset_after is now implemented as asyncio.Task to prevent blocking the loop
 - ClimateMode: binary climate modes should be fully functional now (sending, receiving and syncing)
+- Cover: position update from bus does update current position, but not target position (when moving)
 
 ### Internals
 
+- Cover travelcalculator doesn't start from 0% but is initialized by first movement or status telegram
 - Cover uses 0% for open cover and 100% for closed cover now
 - DPT classes can now be searched via value_type string or dpt number from any parent class (DPTBase for all) to be used in Sensor
 - Use RemoteValue class in BinarySensor, DateTime and ClimateMode device

test/devices_tests/cover_test.py

@@ -315,6 +315,7 @@ def test_position_without_position_address_up(self):
         # DPT 1.008 - 0:up 1:down
         self.assertEqual(telegram,
                          Telegram(GroupAddress('1/2/1'), payload=DPTBinary(0)))
+        self.assertEqual(cover.travelcalculator.travel_to_position, 50)
 
     def test_position_without_position_address_down(self):
         """Test moving cover down - with no absolute positioning supported."""
@@ -331,6 +332,50 @@ def test_position_without_position_address_down(self):
         telegram = xknx.telegrams.get_nowait()
         self.assertEqual(telegram,
                          Telegram(GroupAddress('1/2/1'), payload=DPTBinary(1)))
+        self.assertEqual(cover.travelcalculator.travel_to_position, 80)
+
+    def test_position_without_position_address_uninitialized_up(self):
+        """Test moving uninitialized cover to absolute position - with no absolute positioning supported."""
+        xknx = XKNX(loop=self.loop)
+        cover = Cover(
+            xknx,
+            'TestCover',
+            group_address_long='1/2/1',
+            group_address_short='1/2/2',
+            group_address_position_state='1/2/4')
+
+        with patch('logging.Logger.warning') as mock_warn:
+            self.loop.run_until_complete(asyncio.Task(cover.set_position(50)))
+            self.assertEqual(xknx.telegrams.qsize(), 0)
+            mock_warn.assert_called_with('Current position unknown. Initialize cover by moving to end position.')
+
+        self.loop.run_until_complete(asyncio.Task(cover.set_position(0)))
+        self.assertEqual(xknx.telegrams.qsize(), 1)
+        telegram = xknx.telegrams.get_nowait()
+        self.assertEqual(telegram,
+                         Telegram(GroupAddress('1/2/1'), payload=DPTBinary(0)))
+
+    def test_position_without_position_address_uninitialized_down(self):
+        """Test moving uninitialized cover to absolute position - with no absolute positioning supported."""
+        xknx = XKNX(loop=self.loop)
+        cover = Cover(
+            xknx,
+            'TestCover',
+            group_address_long='1/2/1',
+            group_address_short='1/2/2',
+            group_address_position_state='1/2/4')
+
+        with patch('logging.Logger.warning') as mock_warn:
+            self.loop.run_until_complete(asyncio.Task(cover.set_position(50)))
+            self.assertEqual(xknx.telegrams.qsize(), 0)
+            mock_warn.assert_called_with('Current position unknown. Initialize cover by moving to end position.')
+
+        self.loop.run_until_complete(asyncio.Task(cover.set_position(100)))
+        print(cover.travelcalculator.position_closed)
+        self.assertEqual(xknx.telegrams.qsize(), 1)
+        telegram = xknx.telegrams.get_nowait()
+        self.assertEqual(telegram,
+                         Telegram(GroupAddress('1/2/1'), payload=DPTBinary(1)))
 
     def test_angle(self):
         """Test changing angle."""
@@ -366,7 +411,7 @@ def test_angle_not_supported(self):
     #
     # TEST PROCESS
     #
-    def test_process(self):
+    def test_process_position(self):
         """Test process / reading telegrams from telegram queue. Test if position is processed correctly."""
         xknx = XKNX(loop=self.loop)
         cover = Cover(
@@ -376,9 +421,29 @@ def test_process(self):
             group_address_short='1/2/2',
             group_address_position='1/2/3',
             group_address_position_state='1/2/4')
+        # initial position process - position is unknown so this is the new state - not moving
+        telegram = Telegram(GroupAddress('1/2/3'), payload=DPTArray(213))
+        self.loop.run_until_complete(asyncio.Task(cover.process(telegram)))
+        self.assertEqual(cover.current_position(), 84)
+        self.assertFalse(cover.is_traveling())
+        # state telegram updates current position - we are not moving so this is new state - not moving
+        telegram = Telegram(GroupAddress('1/2/4'), payload=DPTArray(42))
+        self.loop.run_until_complete(asyncio.Task(cover.process(telegram)))
+        self.assertEqual(cover.current_position(), 16)
+        self.assertFalse(cover.is_traveling())
+        self.assertEqual(cover.travelcalculator.travel_to_position, 16)
+        # new position - movement starts
+        telegram = Telegram(GroupAddress('1/2/3'), payload=DPTArray(255))
+        self.loop.run_until_complete(asyncio.Task(cover.process(telegram)))
+        self.assertEqual(cover.current_position(), 16)
+        self.assertTrue(cover.is_closing())
+        self.assertEqual(cover.travelcalculator.travel_to_position, 100)
+        # new state while moving - movement goes on; travelcalculator updated
         telegram = Telegram(GroupAddress('1/2/4'), payload=DPTArray(213))
         self.loop.run_until_complete(asyncio.Task(cover.process(telegram)))
         self.assertEqual(cover.current_position(), 84)
+        self.assertTrue(cover.is_closing())
+        self.assertEqual(cover.travelcalculator.travel_to_position, 100)
 
     def test_process_angle(self):
         """Test process / reading telegrams from telegram queue. Test if position is processed correctly."""
@@ -461,19 +526,36 @@ def test_process_callback(self):
             'TestCover',
             group_address_long='1/2/1',
             group_address_short='1/2/2',
-            group_address_position='1/2/3',
-            group_address_position_state='1/2/4')
+            group_address_stop='1/2/3',
+            group_address_position='1/2/4',
+            group_address_position_state='1/2/5',
+            group_address_angle='1/2/6',
+            group_address_angle_state='1/2/7')
 
         after_update_callback = Mock()
 
         async def async_after_update_callback(device):
             """Async callback."""
             after_update_callback(device)
         cover.register_device_updated_cb(async_after_update_callback)
-
-        telegram = Telegram(GroupAddress('1/2/4'), payload=DPTArray(42))
+        for address, payload, feature in [
+                ('1/2/1', DPTBinary(1), "long"),
+                ('1/2/2', DPTBinary(1), "short"),
+                ('1/2/4', DPTArray(42), "position"),
+                ('1/2/5', DPTArray(42), "position state"),
+                ('1/2/6', DPTArray(42), "angle"),
+                ('1/2/7', DPTArray(51), "angle state")]:
+            with self.subTest(address=address, feature=feature):
+                telegram = Telegram(GroupAddress(address), payload=payload)
+                self.loop.run_until_complete(asyncio.Task(cover.process(telegram)))
+                after_update_callback.assert_called_with(cover)
+                after_update_callback.reset_mock()
+        # Stop only when cover is travelling
+        telegram = Telegram(GroupAddress('1/2/3'), payload=DPTBinary(1))
+        self.loop.run_until_complete(asyncio.Task(cover.process(telegram)))
+        after_update_callback.assert_not_called()
+        self.loop.run_until_complete(asyncio.Task(cover.set_down()))
         self.loop.run_until_complete(asyncio.Task(cover.process(telegram)))
-
         after_update_callback.assert_called_with(cover)
 
     #
@@ -497,7 +579,8 @@ def test_is_traveling(self):
             self.assertFalse(cover.is_opening())
             self.assertFalse(cover.is_closing())
             self.assertTrue(cover.position_reached())
-            # we start with open covers (up)
+            # we start with state open covers (up)
+            cover.travelcalculator.set_position(0)
             self.loop.run_until_complete(asyncio.Task(cover.set_down()))
             self.assertTrue(cover.is_traveling())
             self.assertTrue(cover.is_open())
@@ -565,6 +648,9 @@ def test_auto_stop(self):
             mock_stop.return_value = fut
 
             mock_time.return_value = 1517000000.0
+            # we start with state 0 - open covers (up) this is assumed immediately
+            self.loop.run_until_complete(asyncio.Task(cover.set_position(0)))
+
             self.loop.run_until_complete(asyncio.Task(cover.set_position(50)))
 
             mock_time.return_value = 1517000001.0

test/devices_tests/travelcalculator_test.py

@@ -15,6 +15,16 @@ class TestTravelCalculator(unittest.TestCase):
     #
     # INIT
     #
+    def test_init(self):
+        """Test initial status."""
+        travelcalculator = TravelCalculator(25, 50)
+        self.assertFalse(travelcalculator.is_closed())
+        self.assertFalse(travelcalculator.is_closing())
+        self.assertFalse(travelcalculator.is_opening())
+        self.assertFalse(travelcalculator.is_traveling())
+        self.assertTrue(travelcalculator.position_reached())
+        self.assertIsNone(travelcalculator.current_position())
+
     def test_set_position(self):
         """Test set position."""
         travelcalculator = TravelCalculator(25, 50)
@@ -202,23 +212,27 @@ def test_travel_full_down(self):
             self.assertFalse(travelcalculator.is_open())
 
     def test_is_traveling(self):
-        """Test if cover is traveling."""
+        """Test if cover is traveling and position reached."""
         travelcalculator = TravelCalculator(25, 50)
         with patch('time.time') as mock_time:
             mock_time.return_value = 1580000000.0
             self.assertFalse(travelcalculator.is_traveling())
+            self.assertTrue(travelcalculator.position_reached())
 
             travelcalculator.set_position(80)
             self.assertFalse(travelcalculator.is_traveling())
+            self.assertTrue(travelcalculator.position_reached())
 
             mock_time.return_value = 1580000000.0
             travelcalculator.start_travel_down()
 
             mock_time.return_value = 1580000004.0
             self.assertTrue(travelcalculator.is_traveling())
+            self.assertFalse(travelcalculator.position_reached())
 
             mock_time.return_value = 1580000005.0
             self.assertFalse(travelcalculator.is_traveling())
+            self.assertTrue(travelcalculator.position_reached())
 
     def test_is_opening_closing(self):
         """Test reports is_opening and is_closing."""

xknx/devices/cover.py

@@ -43,12 +43,14 @@ def __init__(self,
         """Initialize Cover class."""
         # pylint: disable=too-many-arguments
         super().__init__(xknx, name, device_updated_cb)
-
+        # self.after_update for position changes is called after updating the
+        # travelcalculator (in process_group_write and set_*) - angle changes
+        # are updated from RemoteValue objects
         self.updown = RemoteValueUpDown(
             xknx,
             group_address_long,
             device_name=self.name,
-            after_update_cb=self.after_update)
+            after_update_cb=None)
 
         self.step = RemoteValueStep(
             xknx,
@@ -60,7 +62,7 @@ def __init__(self,
             xknx,
             group_address=group_address_stop,
             device_name=self.name,
-            after_update_cb=self.after_update)
+            after_update_cb=None)
 
         position_range_from = 100 if invert_position else 0
         position_range_to = 0 if invert_position else 100
@@ -70,7 +72,7 @@ def __init__(self,
             group_address_position_state,
             device_name=self.name,
             feature_name="Position",
-            after_update_cb=self.after_update,
+            after_update_cb=None,
             range_from=position_range_from,
             range_to=position_range_to)
 
@@ -166,11 +168,13 @@ async def set_down(self):
         """Move cover down."""
         await self.updown.down()
         self.travelcalculator.start_travel_down()
+        await self.after_update()
 
     async def set_up(self):
         """Move cover up."""
         await self.updown.up()
         self.travelcalculator.start_travel_up()
+        await self.after_update()
 
     async def set_short_down(self):
         """Move cover short down."""
@@ -194,26 +198,33 @@ async def stop(self):
 
     async def set_position(self, position):
         """Move cover to a desginated postion."""
-        # No direct positioning group address defined
         if not self.position.writable:
+            # No direct positioning group address defined
+            # fully open or close is always possible even if current position is not known
             current_position = self.current_position()
-            if position < current_position:
+            if current_position is None:
+                if position == self.travelcalculator.position_open:
+                    await self.updown.up()
+                elif position == self.travelcalculator.position_closed:
+                    await self.updown.down()
+                else:
+                    self.xknx.logger.warning("Current position unknown. Initialize cover by moving to end position.")
+                    return
+            elif position < current_position:
                 await self.updown.up()
             elif position > current_position:
                 await self.updown.down()
-            self.travelcalculator.start_travel(position)
-            return
-
-        await self.position.set(position)
+        else:
+            await self.position.set(position)
         self.travelcalculator.start_travel(position)
+        await self.after_update()
 
     async def set_angle(self, angle):
         """Move cover to designated angle."""
         if not self.supports_angle:
             self.xknx.logger.warning('Angle not supported for device %s', self.get_name())
             return
         await self.angle.set(angle)
-        await self.after_update()
 
     async def auto_stop_if_necessary(self):
         """Do auto stop if necessary."""
@@ -246,10 +257,6 @@ async def do(self, action):
 
     async def sync(self):
         """Read states of device from KNX bus."""
-        # TODO: if not self.travelcalculator.is_traveling():
-        # when Cover is traveling, requesting state will return false results
-        # but this is manual syncing so it may be ok
-        # should state updater wait for cover to stop?
         await self.position.read_state()
         await self.angle.read_state()
 
@@ -266,9 +273,17 @@ async def process_group_write(self, telegram):
         if await self.stop_.process(telegram) or await self.step.process(telegram):
             if self.is_traveling():
                 self.travelcalculator.stop()
+                await self.after_update()
 
         if await self.position.process(telegram):
-            self.travelcalculator.set_position(self.position.value)
+            # distinction between new target position and position update from bus
+            if telegram.group_address == self.position.group_address_state:
+                if self.is_traveling():
+                    self.travelcalculator.update_position(self.position.value)
+                else:
+                    self.travelcalculator.set_position(self.position.value)
+            else:
+                self.travelcalculator.start_travel(self.position.value)
             await self.after_update()
 
         await self.angle.process(telegram)

xknx/devices/travelcalculator.py

@@ -42,7 +42,7 @@ def __init__(self, travel_time_down, travel_time_up):
         self.travel_time_down = travel_time_down
         self.travel_time_up = travel_time_up
 
-        self.travel_to_position = 0
+        self.travel_to_position = None
         self.travel_started_time = 0
         self.travel_direction = TravelStatus.STOPPED
 
@@ -51,20 +51,30 @@ def __init__(self, travel_time_down, travel_time_up):
         self.position_open = 0
 
     def set_position(self, position):
-        """Set known position of cover."""
-        self.last_known_position = position
+        """Set position and target of cover."""
         self.travel_to_position = position
-        self.position_type = PositionType.CONFIRMED
+        self.update_position(position)
+
+    def update_position(self, position):
+        """Update known position of cover."""
+        self.last_known_position = position
+        if position == self.travel_to_position:
+            self.position_type = PositionType.CONFIRMED
 
     def stop(self):
         """Stop traveling."""
+        if self.position_type == PositionType.UNKNOWN:
+            return
         self.last_known_position = self.current_position()
         self.travel_to_position = self.last_known_position
         self.position_type = PositionType.CALCULATED
         self.travel_direction = TravelStatus.STOPPED
 
     def start_travel(self, travel_to_position):
         """Start traveling to position."""
+        if self.position_type == PositionType.UNKNOWN:
+            self.set_position(travel_to_position)
+            return
         self.stop()
         self.travel_started_time = time.time()
         self.travel_to_position = travel_to_position
@@ -87,6 +97,8 @@ def current_position(self):
         """Return current (calculated or known) position."""
         if self.position_type == PositionType.CALCULATED:
             return self._calculate_position()
+        if self.position_type == PositionType.UNKNOWN:
+            return None
         return self.last_known_position
 
     def is_traveling(self):