From 7ec7cefa1ebfc5d6b423ea2400c75545a2b81fd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Le=C5=9Bniewski?= Date: Wed, 16 Oct 2024 19:08:42 +0200 Subject: [PATCH 1/2] Ensure Device.update_state returns only after properties are updated This change makes the `Device.update_state` wait for the properties to actually be updated before returning. The wait time is limited by the `wait_for` parameter (which was previously present, but not used). This way, when `Device.update_state` returns, properties are guaranteed to be updated. This commit also fixes a few tests, which appear to have had some bugs. --- greeclimate/device.py | 12 +++++++++++- tests/test_device.py | 19 ++++++++++++++----- tests/test_issues.py | 4 ++-- 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/greeclimate/device.py b/greeclimate/device.py index 486a0dc..04ffd0f 100644 --- a/greeclimate/device.py +++ b/greeclimate/device.py @@ -171,6 +171,7 @@ def __init__(self, device_info: DeviceInfo, timeout: int = 120, bind_timeout: in self.device_info: DeviceInfo = device_info self._bind_timeout = bind_timeout + self._update_state_complete = asyncio.Event() """ Device properties """ self.hid = None @@ -262,11 +263,14 @@ async def request_version(self) -> None: except asyncio.TimeoutError: raise DeviceTimeoutError - async def update_state(self, wait_for: float = 30): + async def update_state(self, wait_for: float = 30) -> None: """Update the internal state of the device structure of the physical device, 0 for no wait Args: wait_for (object): How long to wait for an update from the device + + Raises: + DeviceTimeoutError: The device didn't respond within the timeout """ if not self.device_cipher: await self.bind() @@ -278,8 +282,12 @@ async def update_state(self, wait_for: float = 30): props.append("hid") try: + self._update_state_complete.clear() await self.send(self.create_status_message(self.device_info, *props)) + if wait_for > 0: + task = asyncio.create_task(self._update_state_complete.wait()) + await asyncio.wait_for(task, timeout=wait_for) except asyncio.TimeoutError: raise DeviceTimeoutError @@ -304,6 +312,8 @@ def handle_state_update(self, **kwargs) -> None: self._logger.info(f"Device version changed to {self.version}, hid {self.hid}") self._logger.debug(f"Using device temperature {self.current_temperature}") + self._update_state_complete.set() + async def push_state_update(self, wait_for: float = 30): """Push any pending state updates to the unit diff --git a/tests/test_device.py b/tests/test_device.py index 111953b..5e94f21 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -232,7 +232,7 @@ def fake_send(*args, **kwargs): async def test_device_bind_timeout(cipher, send): """Check that the device handles timeout errors when binding.""" info = DeviceInfo(*get_mock_info()) - device = Device(info, timeout=1) + device = Device(info, bind_timeout=1) with pytest.raises(DeviceTimeoutError): await device.bind() @@ -271,7 +271,7 @@ def fake_send(*args, **kwargs): device.ready.set() send.side_effect = fake_send - await device.update_state() + await device.update_state(0) assert send.call_count == 2 assert device.device_cipher.key == fake_key @@ -344,7 +344,7 @@ def fake_send(*args, **kwargs): @pytest.mark.asyncio async def test_update_properties_timeout(cipher, send): - """Check that timeouts are handled when properties are updates.""" + """Check that timeouts are handled when properties are updated.""" device = await generate_device_mock_async() send.side_effect = asyncio.TimeoutError @@ -352,6 +352,15 @@ async def test_update_properties_timeout(cipher, send): await device.update_state() +@pytest.mark.asyncio +async def test_update_properties_timeout_reply(cipher, send): + """Check that reply timeouts are handled when properties are updated.""" + device = await generate_device_mock_async() + + with pytest.raises(DeviceTimeoutError): + await device.update_state(wait_for=1) + + @pytest.mark.asyncio async def test_set_properties_not_dirty(cipher, send): """Check that the state isn't pushed when properties unchanged.""" @@ -694,7 +703,7 @@ async def test_mismatch_temrec_farenheit(temperature, cipher, send): def fake_send(*args, **kwargs): device.handle_state_update(**state) - send.side_effect = None + send.side_effect = fake_send await device.update_state() @@ -733,4 +742,4 @@ def test_device_key_set_get(): device.device_cipher = CipherV1() device.device_key = "fake_key" assert device.device_key == "fake_key" - \ No newline at end of file + diff --git a/tests/test_issues.py b/tests/test_issues.py index ba08eb0..eb38e01 100644 --- a/tests/test_issues.py +++ b/tests/test_issues.py @@ -18,7 +18,7 @@ async def test_issue_69_TemSen_40_should_not_set_firmware_v4(): def fake_send(*args, **kwargs): device.handle_state_update(**mock_v3_state) - with patch.object(Device, "send", wraps=fake_send()): + with patch.object(Device, "send", wraps=fake_send): await device.update_state() assert device.version is None @@ -38,7 +38,7 @@ async def test_issue_87_quiet_should_set_2(): def fake_send(*args, **kwargs): device.handle_state_update(**mock_v3_state) - with patch.object(Device, "send", wraps=fake_send()) as mock: + with patch.object(Device, "send", wraps=fake_send) as mock: await device.push_state_update() mock.assert_called_once() From 5dbd56ee7b7a15be63d1cef9db162b19e445cbfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Le=C5=9Bniewski?= Date: Wed, 16 Oct 2024 20:31:22 +0200 Subject: [PATCH 2/2] Ensure Device.push_state_update returns only after a getting a response from the AC This change makes the `Device.push_state_update` wait for the a response from the AC to be received before returning. The wait time is limited by the `wait_for` parameter (which was previously present, but not used). This way callers of `Device.push_state_update` can be sure that their request reached the AC when the method returns. --- greeclimate/device.py | 4 +++ tests/test_device.py | 59 +++++++++++++++++++++++++++++++------------ 2 files changed, 47 insertions(+), 16 deletions(-) diff --git a/greeclimate/device.py b/greeclimate/device.py index 04ffd0f..b50ce4e 100644 --- a/greeclimate/device.py +++ b/greeclimate/device.py @@ -342,8 +342,12 @@ async def push_state_update(self, wait_for: float = 30): self._dirty.clear() try: + self._update_state_complete.clear() await self.send(self.create_command_message(self.device_info, **props)) + if wait_for > 0: + task = asyncio.create_task(self._update_state_complete.wait()) + await asyncio.wait_for(task, timeout=wait_for) except asyncio.TimeoutError: raise DeviceTimeoutError diff --git a/tests/test_device.py b/tests/test_device.py index 5e94f21..5021fc8 100644 --- a/tests/test_device.py +++ b/tests/test_device.py @@ -278,7 +278,7 @@ def fake_send(*args, **kwargs): device.power = True send.side_effect = None - await device.push_state_update() + await device.push_state_update(0) assert device.device_cipher is not None assert device.device_cipher.key == fake_key @@ -392,6 +392,11 @@ async def test_set_properties(cipher, send): device.power_save = True device.target_humidity = 30 + def fake_send(*args, **kwargs): + state = get_mock_state_on() + device.handle_state_update(**state) + send.side_effect = fake_send + await device.push_state_update() send.assert_called_once() @@ -439,6 +444,34 @@ async def test_set_properties_timeout(cipher, send): await device.push_state_update() +@pytest.mark.asyncio +async def test_set_properties_timeout_reply(cipher, send): + """Check timeout handling when pushing state changes.""" + device = await generate_device_mock_async() + + device.power = True + device.mode = 1 + device.temperature_units = 1 + device.fan_speed = 1 + device.fresh_air = True + device.xfan = True + device.anion = True + device.sleep = True + device.light = True + device.horizontal_swing = 1 + device.vertical_swing = 1 + device.quiet = True + device.turbo = True + device.steady_heat = True + device.power_save = True + + assert len(device._dirty) + + send.reset_mock() + with pytest.raises(DeviceTimeoutError): + await device.push_state_update(1) + + @pytest.mark.asyncio async def test_uninitialized_properties(cipher, send): """Check uninitialized property handling.""" @@ -578,14 +611,12 @@ async def test_send_temperature_celsius(temperature, cipher, send): device.temperature_units = TemperatureUnits.C device.target_temperature = temperature - await device.push_state_update() - assert send.call_count == 1 - def fake_send(*args, **kwargs): device.handle_state_update(**state) send.side_effect = fake_send - await device.update_state() + await device.push_state_update() + assert send.call_count == 1 assert device.current_temperature == temperature @@ -608,14 +639,12 @@ async def test_send_temperature_farenheit(temperature, cipher, send): device.temperature_units = TemperatureUnits.F device.target_temperature = temperature - await device.push_state_update() - assert send.call_count == 1 - def fake_send(*args, **kwargs): device.handle_state_update(**state) send.side_effect = fake_send - await device.update_state() + await device.push_state_update() + assert send.call_count == 1 assert device.current_temperature == temperature @@ -662,18 +691,18 @@ async def test_send_temperature_out_of_range_farenheit_get(temperature, cipher, @pytest.mark.asyncio async def test_enable_disable_sleep_mode(cipher, send): - """Check that properties can be updates.""" + """Check that properties can be updated.""" device = await generate_device_mock_async() device.sleep = True - await device.push_state_update() + await device.push_state_update(0) assert send.call_count == 1 assert device.get_property(Props.SLEEP) == 1 assert device.get_property(Props.SLEEP_MODE) == 1 device.sleep = False - await device.push_state_update() + await device.push_state_update(0) assert send.call_count == 2 assert device.get_property(Props.SLEEP) == 0 @@ -698,14 +727,12 @@ async def test_mismatch_temrec_farenheit(temperature, cipher, send): device.temperature_units = TemperatureUnits.F device.target_temperature = temperature - await device.push_state_update() - assert send.call_count == 1 - def fake_send(*args, **kwargs): device.handle_state_update(**state) send.side_effect = fake_send + await device.push_state_update() - await device.update_state() + assert send.call_count == 1 assert device.current_temperature == temperature