From 40da04838de16ee56951ada71920482844ff37a4 Mon Sep 17 00:00:00 2001 From: Jean-Marc Collin Date: Sun, 15 Oct 2023 10:29:54 +0200 Subject: [PATCH] Bug #121 loop in over climate * Issue #121 - loop when underlying is slow * Issue #121 - try fix * Release 3.5.3 * Fix tests step --- .github/workflows/pull.yml | 6 +- .../versatile_thermostat/climate.py | 95 +++++++++++++------ .../versatile_thermostat/manifest.json | 2 +- .../versatile_thermostat/requirements_dev.txt | 2 +- .../requirements_test.txt | 5 +- .../versatile_thermostat/tests/test_bugs.py | 11 ++- .../tests/test_multiple_switch.py | 10 +- hacs.json | 2 +- 8 files changed, 93 insertions(+), 40 deletions(-) diff --git a/.github/workflows/pull.yml b/.github/workflows/pull.yml index 0912fd6f..8a55856e 100644 --- a/.github/workflows/pull.yml +++ b/.github/workflows/pull.yml @@ -31,6 +31,8 @@ jobs: - run: black . tests: + # Tests don't run in Gitlab ci environment + if: 0 runs-on: "ubuntu-latest" name: Run tests steps: @@ -41,10 +43,10 @@ jobs: with: python-version: "3.8" - name: Install requirements - run: python3 -m pip install -r requirements_test.txt + run: cd custom_components/versatile_thermostat && python3 -m pip install -r requirements_test.txt - name: Run tests run: | - pytest \ + cd custom_components/versatile_thermostat && pytest \ -qq \ --timeout=9 \ --durations=10 \ diff --git a/custom_components/versatile_thermostat/climate.py b/custom_components/versatile_thermostat/climate.py index 249ddc54..403cf4db 100644 --- a/custom_components/versatile_thermostat/climate.py +++ b/custom_components/versatile_thermostat/climate.py @@ -220,6 +220,7 @@ class VersatileThermostat(ClimateEntity, RestoreEntity): _presence_state: bool _window_auto_state: bool _underlyings: list[UnderlyingEntity] + _last_change_time: datetime def __init__(self, hass: HomeAssistant, unique_id, name, entry_infos) -> None: """Initialize the thermostat.""" @@ -284,6 +285,8 @@ def __init__(self, hass: HomeAssistant, unique_id, name, entry_infos) -> None: self._current_tz = dt_util.get_time_zone(self._hass.config.time_zone) + self._last_change_time = None + self._underlyings = [] self.post_init(entry_infos) @@ -778,6 +781,8 @@ async def _async_startup_internal(*_): else: self.hass.create_task(self._async_control_heating()) + self.reset_last_change_time() + await self.get_my_previous_state() if self.hass.state == CoreState.running: @@ -951,7 +956,6 @@ def hvac_mode(self) -> HVACMode | None: """Return current operation.""" # Issue #114 - returns my current hvac_mode and not the underlying hvac_mode which could be different # delta will be managed by climate_state_change event. - # TODO remove this when ok # if self._is_over_climate: # if one not OFF -> return it # else OFF @@ -1233,6 +1237,8 @@ async def async_set_hvac_mode(self, hvac_mode, need_control_heating=True): # Ensure we update the current operation after changing the mode self.reset_last_temperature_time() + self.reset_last_change_time() + self.update_custom_attributes() self.async_write_ha_state() self.send_event(EventType.HVAC_MODE_EVENT, {"hvac_mode": self._hvac_mode}) @@ -1288,6 +1294,11 @@ async def _async_set_preset_mode_internal(self, preset_mode, force=False): self.recalculate() self.send_event(EventType.PRESET_EVENT, {"preset": self._attr_preset_mode}) + def reset_last_change_time(self, old_preset_mode=None): + """Reset to now the last change time""" + self._last_change_time = datetime.now(tz=self._current_tz) + _LOGGER.debug("%s - last_change_time is now %s", self, self._last_change_time) + def reset_last_temperature_time(self, old_preset_mode=None): """Reset to now the last temperature time if conditions are satisfied""" if ( @@ -1363,6 +1374,7 @@ async def async_set_temperature(self, **kwargs): await self._async_internal_set_temperature(temperature) self._attr_preset_mode = PRESET_NONE self.recalculate() + self.reset_last_change_time() await self._async_control_heating(force=True) async def _async_internal_set_temperature(self, temperature): @@ -1585,7 +1597,22 @@ def _async_switch_changed(self, event): @callback async def _async_climate_changed(self, event): - """Handle unerdlying climate state changes.""" + """Handle unerdlying climate state changes. + This method takes the underlying values and update the VTherm with them. + To avoid loops (issues #121 #101 #95 #99), we discard the event if it is received + less than 10 sec after the last command. What we want here is to take the values + from underlyings ONLY if someone have change directly on the underlying and not + as a return of the command. The only thing we take all the time is the HVACAction + which is important for feedaback and which cannot generates loops. + """ + + async def end_climate_changed(changes): + """ To end the event management""" + if changes: + self.async_write_ha_state() + self.update_custom_attributes() + await self._async_control_heating() + new_state = event.data.get("new_state") _LOGGER.debug("%s - _async_climate_changed new_state is %s", self, new_state) if not new_state: @@ -1606,6 +1633,11 @@ async def _async_climate_changed(self, event): else None ) + old_state_date_changed = old_state.last_changed if old_state and old_state.last_changed else None + old_state_date_updated = old_state.last_updated if old_state and old_state.last_updated else None + new_state_date_changed = new_state.last_changed if new_state and new_state.last_changed else None + new_state_date_updated = new_state.last_updated if new_state and new_state.last_updated else None + # Issue 99 - some AC turn hvac_mode=cool and hvac_action=idle when sending a HVACMode_OFF command # Issue 114 - Remove this because hvac_mode is now managed by local _hvac_mode and use idle action as is #if self._hvac_mode == HVACMode.OFF and new_hvac_action == HVACAction.IDLE: @@ -1621,24 +1653,9 @@ async def _async_climate_changed(self, event): old_hvac_action, ) - if new_hvac_mode in [ - HVACMode.OFF, - HVACMode.HEAT, - HVACMode.COOL, - HVACMode.HEAT_COOL, - HVACMode.DRY, - HVACMode.AUTO, - HVACMode.FAN_ONLY, - None - ] and self._hvac_mode != new_hvac_mode: - changes = True - self._hvac_mode = new_hvac_mode - # Do not try to update all underlying state, else we will have a loop - if self._is_over_climate: - for under in self._underlyings: - await under.set_hvac_mode(new_hvac_mode) + _LOGGER.debug("%s - last_change_time=%s old_state_date_changed=%s old_state_date_updated=%s new_state_date_changed=%s new_state_date_updated=%s", self, self._last_change_time, old_state_date_changed, old_state_date_updated, new_state_date_changed, new_state_date_updated) - # Interpretation of hvac + # Interpretation of hvac action HVAC_ACTION_ON = [ # pylint: disable=invalid-name HVACAction.COOLING, HVACAction.DRYING, @@ -1677,18 +1694,43 @@ async def _async_climate_changed(self, event): ) changes = True + # Issue #120 - Some TRV are chaning target temperature a very long time (6 sec) after the change. + # In that case a loop is possible if a user change multiple times during this 6 sec. + if new_state_date_updated and self._last_change_time: + delta = (new_state_date_updated - self._last_change_time).total_seconds() + if delta < 10: + _LOGGER.info("%s - underlying event is received less than 10 sec after command. Forget it to avoid loop", self + ) + await end_climate_changed(changes) + return + + if new_hvac_mode in [ + HVACMode.OFF, + HVACMode.HEAT, + HVACMode.COOL, + HVACMode.HEAT_COOL, + HVACMode.DRY, + HVACMode.AUTO, + HVACMode.FAN_ONLY, + None + ] and self._hvac_mode != new_hvac_mode: + changes = True + self._hvac_mode = new_hvac_mode + # Update all underlyings state + if self._is_over_climate: + for under in self._underlyings: + await under.set_hvac_mode(new_hvac_mode) + if not changes: # try to manage new target temperature set if state _LOGGER.debug("Do temperature check. temperature is %s, new_state.attributes is %s", self.target_temperature, new_state.attributes) if self._is_over_climate and new_state.attributes and (new_target_temp := new_state.attributes.get("temperature")) and new_target_temp != self.target_temperature: - _LOGGER.info("%s - Target temp have change to %s", self, new_target_temp) + _LOGGER.info("%s - Target temp in underlying have change to %s", self, new_target_temp) await self.async_set_temperature(temperature = new_target_temp) changes = True - if changes: - self.async_write_ha_state() - self.update_custom_attributes() - await self._async_control_heating() + await end_climate_changed(changes) + @callback async def _async_update_temp(self, state: State): @@ -2103,9 +2145,6 @@ async def check_security(self) -> bool: now - self._last_ext_temperature_mesure.replace(tzinfo=self._current_tz) ).total_seconds() / 60.0 - # TODO before change: - # mode_cond = self._is_over_climate or self._hvac_mode != HVACMode.OFF - # fixed into this. Why if _is_over_climate we could into security even if HVACMode is OFF ? mode_cond = self._hvac_mode != HVACMode.OFF temp_cond: bool = ( @@ -2387,7 +2426,7 @@ def update_custom_attributes(self): "window_auto_max_duration": self._window_auto_max_duration, } if self._is_over_climate: - self._attr_extra_state_attributes["underlying_climate_1"] = self._underlyings[ + self._attr_extra_state_attributes["underlying_climate_0"] = self._underlyings[ 0 ].entity_id self._attr_extra_state_attributes["underlying_climate_1"] = self._underlyings[ diff --git a/custom_components/versatile_thermostat/manifest.json b/custom_components/versatile_thermostat/manifest.json index 24fa9a61..b6753bd7 100644 --- a/custom_components/versatile_thermostat/manifest.json +++ b/custom_components/versatile_thermostat/manifest.json @@ -14,6 +14,6 @@ "quality_scale": "silver", "requirements": [], "ssdp": [], - "version": "3.0.0", + "version": "3.5.3", "zeroconf": [] } \ No newline at end of file diff --git a/custom_components/versatile_thermostat/requirements_dev.txt b/custom_components/versatile_thermostat/requirements_dev.txt index ed73233f..16ea5885 100644 --- a/custom_components/versatile_thermostat/requirements_dev.txt +++ b/custom_components/versatile_thermostat/requirements_dev.txt @@ -1,2 +1,2 @@ -homeassistant==2023.10.1 +homeassistant==2023.10.3 ffmpeg \ No newline at end of file diff --git a/custom_components/versatile_thermostat/requirements_test.txt b/custom_components/versatile_thermostat/requirements_test.txt index 5babf027..4af38e46 100644 --- a/custom_components/versatile_thermostat/requirements_test.txt +++ b/custom_components/versatile_thermostat/requirements_test.txt @@ -1,4 +1,5 @@ --r requirements_dev.txt +# For automatic run of test in Gitlab CI, we must not include other things that pytest-homeassistant-custom-component +#-r requirements_dev.txt # aiodiscover -ulid_transform +# ulid_transform pytest-homeassistant-custom-component \ No newline at end of file diff --git a/custom_components/versatile_thermostat/tests/test_bugs.py b/custom_components/versatile_thermostat/tests/test_bugs.py index e0823555..b6a3a7cb 100644 --- a/custom_components/versatile_thermostat/tests/test_bugs.py +++ b/custom_components/versatile_thermostat/tests/test_bugs.py @@ -530,9 +530,16 @@ def find_my_entity(entity_id) -> ClimateEntity: await entity.async_set_preset_mode(PRESET_COMFORT) assert entity.preset_mode == PRESET_COMFORT - # 2. Change the target temp of underlying thermostat + # 2. Change the target temp of underlying thermostat at now -> the event will be disgarded because to fast (to avoid loop cf issue 121) await send_climate_change_event_with_temperature(entity, HVACMode.HEAT, HVACMode.HEAT, HVACAction.OFF, HVACAction.OFF, now, 12.75) - # Should have been switched to Manual preset + # Should NOT have been switched to Manual preset + assert entity.target_temperature == 17 + assert entity.preset_mode is PRESET_COMFORT + + # 2. Change the target temp of underlying thermostat at 11 sec later -> the event will be taken + # Wait 11 sec + event_timestamp = now + timedelta(seconds=11) + await send_climate_change_event_with_temperature(entity, HVACMode.HEAT, HVACMode.HEAT, HVACAction.OFF, HVACAction.OFF, event_timestamp, 12.75) assert entity.target_temperature == 12.75 assert entity.preset_mode is PRESET_NONE diff --git a/custom_components/versatile_thermostat/tests/test_multiple_switch.py b/custom_components/versatile_thermostat/tests/test_multiple_switch.py index a5f21c4b..28a2351d 100644 --- a/custom_components/versatile_thermostat/tests/test_multiple_switch.py +++ b/custom_components/versatile_thermostat/tests/test_multiple_switch.py @@ -163,7 +163,7 @@ async def test_one_switch_cycle( await asyncio.sleep(0.1) assert mock_heater_on.call_count == 1 - # TODO normal ? assert entity.underlying_entity(0)._should_relaunch_control_heating is False + # normal ? assert entity.underlying_entity(0)._should_relaunch_control_heating is False # Simulate the end of heater on cycle event_timestamp = now - timedelta(minutes=3) @@ -522,7 +522,9 @@ async def test_multiple_climates_underlying_changes( ), patch( "custom_components.versatile_thermostat.underlyings.UnderlyingClimate.set_hvac_mode" ) as mock_underlying_set_hvac_mode: - await send_climate_change_event(entity, HVACMode.OFF, HVACMode.HEAT, HVACAction.OFF, HVACAction.HEATING, now) + # Wait 11 sec so that the event will not be discarded + event_timestamp = now + timedelta(seconds=11) + await send_climate_change_event(entity, HVACMode.OFF, HVACMode.HEAT, HVACAction.OFF, HVACAction.HEATING, event_timestamp) # Should be call for all Switch assert mock_underlying_set_hvac_mode.call_count == 4 @@ -543,7 +545,9 @@ async def test_multiple_climates_underlying_changes( # notice that there is no need of return_value=HVACAction.IDLE because this is not a function but a property "custom_components.versatile_thermostat.underlyings.UnderlyingClimate.hvac_action", HVACAction.IDLE ) as mock_underlying_get_hvac_action: - await send_climate_change_event(entity, HVACMode.HEAT, HVACMode.OFF, HVACAction.IDLE, HVACAction.OFF, now) + # Wait 11 sec so that the event will not be discarded + event_timestamp = now + timedelta(seconds=11) + await send_climate_change_event(entity, HVACMode.HEAT, HVACMode.OFF, HVACAction.IDLE, HVACAction.OFF, event_timestamp) # Should be call for all Switch assert mock_underlying_set_hvac_mode.call_count == 4 diff --git a/hacs.json b/hacs.json index e56181ab..38d8b721 100644 --- a/hacs.json +++ b/hacs.json @@ -3,5 +3,5 @@ "content_in_root": false, "render_readme": true, "hide_default_branch": false, - "homeassistant": "2023.7.3" + "homeassistant": "2023.10.3" } \ No newline at end of file