From 0e0a40f21aeb81df10d8dd257cd1e7ecb84b7e6f Mon Sep 17 00:00:00 2001 From: Philip Allgaier Date: Tue, 23 Feb 2021 10:52:11 +0100 Subject: [PATCH 1/7] Use correct move_short direction when stopping --- xknx/devices/cover.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/xknx/devices/cover.py b/xknx/devices/cover.py index c250ef7c17..92324291ff 100644 --- a/xknx/devices/cover.py +++ b/xknx/devices/cover.py @@ -18,7 +18,7 @@ ) from .device import Device, DeviceCallbackType -from .travelcalculator import TravelCalculator +from .travelcalculator import TravelCalculator, TravelStatus if TYPE_CHECKING: from xknx.remote_value import RemoteValue @@ -217,9 +217,15 @@ async def set_short_up(self) -> None: async def stop(self) -> None: """Stop cover.""" if self.stop_.writable: - await self.stop_.on() + if self.travelcalculator.travel_direction == TravelStatus.DIRECTION_UP: + await self.stop_.off() + elif self.travelcalculator.travel_direction == TravelStatus.DIRECTION_DOWN: + await self.stop_.on() elif self.step.writable: - await self.step.increase() + if self.travelcalculator.travel_direction == TravelStatus.DIRECTION_UP: + await self.step.decrease() + elif self.travelcalculator.travel_direction == TravelStatus.DIRECTION_DOWN: + await self.step.increase() else: logger.warning("Stop not supported for device %s", self.get_name()) return @@ -271,6 +277,13 @@ async def set_angle(self, angle: int) -> None: if not self.supports_angle: logger.warning("Angle not supported for device %s", self.get_name()) return + + self.travelcalculator.travel_direction = ( + TravelStatus.DIRECTION_DOWN + if angle > self.current_angle() + else TravelStatus.DIRECTION_UP + ) + await self.angle.set(angle) async def auto_stop_if_necessary(self) -> None: From c052f6fe9e3ed783e11004174319dc7afb06e5f4 Mon Sep 17 00:00:00 2001 From: Philip Allgaier Date: Tue, 23 Feb 2021 11:06:13 +0100 Subject: [PATCH 2/7] Correct stop_address handling --- xknx/devices/cover.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/xknx/devices/cover.py b/xknx/devices/cover.py index 92324291ff..f9c5faaf95 100644 --- a/xknx/devices/cover.py +++ b/xknx/devices/cover.py @@ -217,10 +217,7 @@ async def set_short_up(self) -> None: async def stop(self) -> None: """Stop cover.""" if self.stop_.writable: - if self.travelcalculator.travel_direction == TravelStatus.DIRECTION_UP: - await self.stop_.off() - elif self.travelcalculator.travel_direction == TravelStatus.DIRECTION_DOWN: - await self.stop_.on() + await self.stop_.on() elif self.step.writable: if self.travelcalculator.travel_direction == TravelStatus.DIRECTION_UP: await self.step.decrease() From 6235e0e2441181ed40fb2dbd355d62602b6c2042 Mon Sep 17 00:00:00 2001 From: Philip Allgaier Date: Tue, 23 Feb 2021 11:39:01 +0100 Subject: [PATCH 3/7] Do not use TravelCalculator for tilting --- xknx/devices/cover.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/xknx/devices/cover.py b/xknx/devices/cover.py index f9c5faaf95..ddb1ea95c7 100644 --- a/xknx/devices/cover.py +++ b/xknx/devices/cover.py @@ -125,6 +125,7 @@ def __init__( self.travel_time_up = travel_time_up self.travelcalculator = TravelCalculator(travel_time_down, travel_time_up) + self.travel_direction_tilt = None self.device_class = device_class @@ -198,12 +199,14 @@ async def set_down(self) -> None: """Move cover down.""" await self.updown.down() self.travelcalculator.start_travel_down() + self.travel_direction_tilt = None await self.after_update() async def set_up(self) -> None: """Move cover up.""" await self.updown.up() self.travelcalculator.start_travel_up() + self.travel_direction_tilt = None await self.after_update() async def set_short_down(self) -> None: @@ -219,14 +222,17 @@ async def stop(self) -> None: if self.stop_.writable: await self.stop_.on() elif self.step.writable: - if self.travelcalculator.travel_direction == TravelStatus.DIRECTION_UP: + if (self.travel_direction_tilt == TravelStatus.DIRECTION_UP or + self.travelcalculator.travel_direction == TravelStatus.DIRECTION_UP): await self.step.decrease() - elif self.travelcalculator.travel_direction == TravelStatus.DIRECTION_DOWN: + elif (self.travel_direction_tilt == TravelStatus.DIRECTION_DOWN or + self.travelcalculator.travel_direction == TravelStatus.DIRECTION_DOWN): await self.step.increase() else: logger.warning("Stop not supported for device %s", self.get_name()) return self.travelcalculator.stop() + self.travel_direction_tilt = None await self.after_update() async def set_position(self, position: int) -> None: @@ -275,7 +281,7 @@ async def set_angle(self, angle: int) -> None: logger.warning("Angle not supported for device %s", self.get_name()) return - self.travelcalculator.travel_direction = ( + self.travel_direction_tilt = ( TravelStatus.DIRECTION_DOWN if angle > self.current_angle() else TravelStatus.DIRECTION_UP @@ -286,9 +292,9 @@ async def set_angle(self, angle: int) -> None: async def auto_stop_if_necessary(self) -> None: """Do auto stop if necessary.""" # If device does not support auto_positioning, - # we have to stop the device when position is reached. + # we have to stop the device when position is reached, # unless device was traveling to fully open - # or fully closed state + # or fully closed state. if ( self.supports_stop and not self.position_target.writable From a1126b9dcb50ee6fb2fcfef89c0cdeaeacb82aef Mon Sep 17 00:00:00 2001 From: Philip Allgaier Date: Tue, 23 Feb 2021 15:05:16 +0100 Subject: [PATCH 4/7] Fix + add tests --- test/devices_tests/cover_test.py | 114 +++++++++++++++++++++++++++++- xknx/devices/cover.py | 16 +++-- xknx/remote_value/remote_value.py | 2 +- 3 files changed, 123 insertions(+), 9 deletions(-) diff --git a/test/devices_tests/cover_test.py b/test/devices_tests/cover_test.py index cce88dccd3..cb2b1c3983 100644 --- a/test/devices_tests/cover_test.py +++ b/test/devices_tests/cover_test.py @@ -374,8 +374,23 @@ def test_stop(self): group_address_position="1/2/3", group_address_position_state="1/2/4", ) + # Attempt stopping while not actually moving self.loop.run_until_complete(cover_short_stop.stop()) - self.assertEqual(xknx.telegrams.qsize(), 1) + self.assertEqual(xknx.telegrams.qsize(), 0) + + # Attempt stopping while moving down + cover_short_stop.travelcalculator.set_position(0) + self.loop.run_until_complete(cover_short_stop.set_down()) + self.loop.run_until_complete(cover_short_stop.stop()) + self.assertEqual(xknx.telegrams.qsize(), 2) + telegram = xknx.telegrams.get_nowait() + self.assertEqual( + telegram, + Telegram( + destination_address=GroupAddress("1/2/1"), + payload=GroupValueWrite(DPTBinary(1)), + ), + ) telegram = xknx.telegrams.get_nowait() self.assertEqual( telegram, @@ -385,6 +400,28 @@ def test_stop(self): ), ) + # Attempt stopping while moving up + cover_short_stop.travelcalculator.set_position(100) + self.loop.run_until_complete(cover_short_stop.set_up()) + self.loop.run_until_complete(cover_short_stop.stop()) + self.assertEqual(xknx.telegrams.qsize(), 2) + telegram = xknx.telegrams.get_nowait() + self.assertEqual( + telegram, + Telegram( + destination_address=GroupAddress("1/2/1"), + payload=GroupValueWrite(DPTBinary(0)), + ), + ) + telegram = xknx.telegrams.get_nowait() + self.assertEqual( + telegram, + Telegram( + destination_address=GroupAddress("1/2/2"), + payload=GroupValueWrite(DPTBinary(0)), + ), + ) + cover_manual_stop = Cover( xknx, "TestCover", @@ -405,6 +442,79 @@ def test_stop(self): ), ) + def test_stop_angle(self): + """Test stopping cover during angle move / tilting.""" + xknx = XKNX() + cover_short_stop = Cover( + xknx, + "TestCover", + group_address_long="1/2/1", + group_address_short="1/2/2", + group_address_angle="1/2/5", + group_address_angle_state="1/2/6", + ) + # Attempt stopping while not actually tilting + self.loop.run_until_complete(cover_short_stop.stop()) + self.assertEqual(xknx.telegrams.qsize(), 0) + + # Set cover tilt to a dummy start value, since otherwise we cannot + # determine later on a tilt direction and without it, stopping the + # til process has no effect. + self.loop.run_until_complete( + cover_short_stop.angle.process( + Telegram( + destination_address=GroupAddress("1/2/5"), + payload=GroupValueWrite(DPTArray(0xAA)), + ) + ) + ) + + # Attempt stopping while tilting down + self.loop.run_until_complete(cover_short_stop.set_angle(100)) + self.loop.run_until_complete(cover_short_stop.stop()) + self.assertEqual(xknx.telegrams.qsize(), 2) + telegram = xknx.telegrams.get_nowait() + print(telegram) + self.assertEqual( + telegram, + Telegram( + destination_address=GroupAddress("1/2/5"), + payload=GroupValueWrite(DPTArray(0xFF)), + ), + ) + telegram = xknx.telegrams.get_nowait() + print(telegram) + self.assertEqual( + telegram, + Telegram( + destination_address=GroupAddress("1/2/2"), + payload=GroupValueWrite(DPTBinary(1)), + ), + ) + + # Attempt stopping while tilting up + self.loop.run_until_complete(cover_short_stop.set_angle(0)) + self.loop.run_until_complete(cover_short_stop.stop()) + self.assertEqual(xknx.telegrams.qsize(), 2) + telegram = xknx.telegrams.get_nowait() + print(telegram) + self.assertEqual( + telegram, + Telegram( + destination_address=GroupAddress("1/2/5"), + payload=GroupValueWrite(DPTArray(0x00)), + ), + ) + telegram = xknx.telegrams.get_nowait() + print(telegram) + self.assertEqual( + telegram, + Telegram( + destination_address=GroupAddress("1/2/2"), + payload=GroupValueWrite(DPTBinary(0)), + ), + ) + # # TEST POSITION # @@ -569,7 +679,7 @@ def test_angle(self): ) def test_angle_not_supported(self): - """Test changing angle on cover wich does not support angle.""" + """Test changing angle on cover which does not support angle.""" xknx = XKNX() cover = Cover( xknx, diff --git a/xknx/devices/cover.py b/xknx/devices/cover.py index ddb1ea95c7..a8d2bcc7df 100644 --- a/xknx/devices/cover.py +++ b/xknx/devices/cover.py @@ -125,7 +125,7 @@ def __init__( self.travel_time_up = travel_time_up self.travelcalculator = TravelCalculator(travel_time_down, travel_time_up) - self.travel_direction_tilt = None + self.travel_direction_tilt = Optional[TravelStatus] self.device_class = device_class @@ -222,11 +222,15 @@ async def stop(self) -> None: if self.stop_.writable: await self.stop_.on() elif self.step.writable: - if (self.travel_direction_tilt == TravelStatus.DIRECTION_UP or - self.travelcalculator.travel_direction == TravelStatus.DIRECTION_UP): + if ( + self.travel_direction_tilt == TravelStatus.DIRECTION_UP + or self.travelcalculator.travel_direction == TravelStatus.DIRECTION_UP + ): await self.step.decrease() - elif (self.travel_direction_tilt == TravelStatus.DIRECTION_DOWN or - self.travelcalculator.travel_direction == TravelStatus.DIRECTION_DOWN): + elif ( + self.travel_direction_tilt == TravelStatus.DIRECTION_DOWN + or self.travelcalculator.travel_direction == TravelStatus.DIRECTION_DOWN + ): await self.step.increase() else: logger.warning("Stop not supported for device %s", self.get_name()) @@ -283,7 +287,7 @@ async def set_angle(self, angle: int) -> None: self.travel_direction_tilt = ( TravelStatus.DIRECTION_DOWN - if angle > self.current_angle() + if self.current_angle() is not None and angle > int(self.angle.value) else TravelStatus.DIRECTION_UP ) diff --git a/xknx/remote_value/remote_value.py b/xknx/remote_value/remote_value.py index 9bcda02d62..e6d1ba875a 100644 --- a/xknx/remote_value/remote_value.py +++ b/xknx/remote_value/remote_value.py @@ -218,7 +218,7 @@ async def read_state(self, wait_for_result: bool = False) -> None: """Send GroupValueRead telegram for state address to KNX bus.""" if self.group_address_state is not None: # pylint: disable=import-outside-toplevel - # TODO: send a ReadRequset and start a timeout from here instead of ValueReader + # TODO: send a ReadRequest and start a timeout from here instead of ValueReader # cancel timeout form process(); delete ValueReader from xknx.core import ValueReader From 546693329008b10e760705b54545fb6018384d82 Mon Sep 17 00:00:00 2001 From: Philip Allgaier Date: Tue, 23 Feb 2021 15:09:08 +0100 Subject: [PATCH 5/7] Cleanup --- xknx/devices/cover.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xknx/devices/cover.py b/xknx/devices/cover.py index a8d2bcc7df..cfdd0d8e85 100644 --- a/xknx/devices/cover.py +++ b/xknx/devices/cover.py @@ -287,7 +287,7 @@ async def set_angle(self, angle: int) -> None: self.travel_direction_tilt = ( TravelStatus.DIRECTION_DOWN - if self.current_angle() is not None and angle > int(self.angle.value) + if self.current_angle() and angle > int(self.angle.value) else TravelStatus.DIRECTION_UP ) From 1cc4c8cb1e0d530842aff5efcd283fc74fe2b8b1 Mon Sep 17 00:00:00 2001 From: Philip Allgaier Date: Tue, 23 Feb 2021 15:41:26 +0100 Subject: [PATCH 6/7] Use function for angle retrieval --- xknx/devices/cover.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xknx/devices/cover.py b/xknx/devices/cover.py index cfdd0d8e85..318fc2f135 100644 --- a/xknx/devices/cover.py +++ b/xknx/devices/cover.py @@ -285,9 +285,10 @@ async def set_angle(self, angle: int) -> None: logger.warning("Angle not supported for device %s", self.get_name()) return + current_angle = self.current_angle() self.travel_direction_tilt = ( TravelStatus.DIRECTION_DOWN - if self.current_angle() and angle > int(self.angle.value) + if current_angle and angle > current_angle else TravelStatus.DIRECTION_UP ) From 1ff42f31eaa148baf43fe81ea3a81dea6b77b68a Mon Sep 17 00:00:00 2001 From: Philip Allgaier Date: Tue, 23 Feb 2021 15:48:55 +0100 Subject: [PATCH 7/7] Condition adjustment --- xknx/devices/cover.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xknx/devices/cover.py b/xknx/devices/cover.py index 318fc2f135..f6c5f7205a 100644 --- a/xknx/devices/cover.py +++ b/xknx/devices/cover.py @@ -288,7 +288,7 @@ async def set_angle(self, angle: int) -> None: current_angle = self.current_angle() self.travel_direction_tilt = ( TravelStatus.DIRECTION_DOWN - if current_angle and angle > current_angle + if current_angle and angle >= current_angle else TravelStatus.DIRECTION_UP )