jmcollin78 / versatile_thermostat

A full featured Thermostat for Home Assistant: presets, window, motion, presence and overpowering management
MIT License
327 stars 34 forks source link

Window States should turn off Underlying Entities, but not the VTherm entity. #468

Closed mag2352 closed 1 month ago

mag2352 commented 5 months ago

I'm just making a dedicated feature request for this, as this isn't related to the restoring of states issue, #465.

I'm proposing that if a window is open, the VTherm should change the hvac_mode of the underlying entities, but leave its own hvac_mode unchanged.

Here is why I think this change is valuable:

Let's say that I have a window open. The VTherm would have an HVAC mode of "off". Upon looking at this, I can't tell if the VTherm is actually off (but the window happens to be open), or "off" because the window is open (and the VTherm would turn back on if the window closes). If the VTherm is on but the window is open, I can't turn the VTherm off unless I close the window, wait for it to turn on, and then turn it off again. Additionally, if the window closes, and I make a change to the HVAC mode manually before the window timeout triggers, the change I made would be overwritten by restore_hvac_mode. This makes it difficult to use the VTherm with the scheduler integration (the scheduler might turn off the VTherm when it's in the "off" state, which would not actually turn off the VTherm, as closing the window would make the VTherm turn back on).

Since I have begun working on this, I have more or less got it working. I would appreciate insights as to whether or not this is actually a good way to implement this feature. I created two functions that are essentially copies of their counterparts. I created async_set_temp_hvac_mode, and restore_temp_hvac_mode. They appear as below:

    async def async_set_temp_hvac_mode(self, hvac_mode: HVACMode, need_control_heating=True):
        """Set new target hvac mode."""
        _LOGGER.info("%s - Set hvac mode: %s", self, hvac_mode)

        if hvac_mode is None:
            return

        # Delegate to all underlying
        sub_need_control_heating = False
        for under in self._underlyings:
            sub_need_control_heating = (
                await under.set_hvac_mode(hvac_mode) or need_control_heating
            )

        # If AC is on maybe we have to change the temperature in force mode, but not in frost mode (there is no Frost protection possible in AC mode)
        if self._hvac_mode == HVACMode.COOL and self.preset_mode != PRESET_NONE:
            if self.preset_mode != PRESET_FROST_PROTECTION:
                await self._async_set_preset_mode_internal(self.preset_mode, True)
            else:
                await self._async_set_preset_mode_internal(PRESET_ECO, True, False)

        if need_control_heating and sub_need_control_heating:
            await self.async_control_heating(force=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})
    async def restore_temp_hvac_mode(self, need_control_heating=False):
        """Restore a previous hvac_mod"""
        await self.async_set_temp_hvac_mode(self._hvac_mode, need_control_heating)
        _LOGGER.debug(
            "%s - Restored hvac_mode - hvac_mode is %s",
            self,
            self._hvac_mode,
        )

These functions are essentially the same as the original functions, but they do not change the state of the VTherm itself. I then changed the calls to these functions in change_window_detection_state, and service_set_window_bypass_state in base_thermostat.py. I then commented this line out in thermostat_climate.py:

https://github.com/jmcollin78/versatile_thermostat/blob/2fd60074c7c0c2b483f7e2af3134d2b82dfd3cec/custom_components/versatile_thermostat/thermostat_climate.py#L774

The commit where I did this is here:

mag2352/versatile_thermostat@c004a91007e9624643543532f91839b255e07785

mag2352 commented 5 months ago

I've used this overnight, and have noticed an issue with the window behavior. I'm not really sure what is happening (the logs are not verbose enough). Maybe it has to do with the regulation algorithm still running, so I'll try to disable that. I also have trouble reproducing this issue (I can't get it to happen again, but there is definitely something wrong), but I'll try to work on this further. I suspect that @jmcollin78 would know the most about how to implement this feature request, as I am relatively new to working on this integration.

I'm trying the following commit to see if this resolves the issue:

mag2352/versatile_thermostat@ef785ac09e38ae729d93f52b86a1a6730bfa5e49

The logs below show that the mode restored is cool, but the event change shows that the hvac_mode is off, so something isn't lining up there. I'll see if the commit above changes anything.

2024-05-30 04:00:00.008 INFO (MainThread) [custom_components.versatile_thermostat.thermostat_climate] VersatileThermostat-Boys Room VTherm - Calling ThermostatClimate._send_regulated_temperature force=False
2024-05-30 04:00:00.008 INFO (MainThread) [custom_components.versatile_thermostat.thermostat_climate] VersatileThermostat-Boys Room VTherm - period (0.0) min is < 1 min -> forget the regulation send
2024-05-30 04:00:00.008 INFO (MainThread) [custom_components.versatile_thermostat.base_thermostat] VersatileThermostat-Boys Room VTherm - Window is closed. Restoring hvac_mode 'cool' if central_mode is not STOPPED
2024-05-30 04:00:00.008 INFO (MainThread) [custom_components.versatile_thermostat.base_thermostat] VersatileThermostat-Boys Room VTherm - Set temp hvac mode: cool
2024-05-30 04:00:00.009 DEBUG (MainThread) [custom_components.versatile_thermostat.base_thermostat] VersatileThermostat-Boys Room VTherm - Restored hvac_mode - hvac_mode is cool
2024-05-30 04:00:00.813 DEBUG (MainThread) [custom_components.versatile_thermostat.thermostat_climate] VersatileThermostat-Boys Room VTherm - _async_climate_changed new_state is <state climate.midea_climate_3=off; hvac_modes=[<HVACMode.OFF: 'off'>, <HVACMode.HEAT_COOL: 'heat_cool'>, <HVACMode.COOL: 'cool'>, <HVACMode.HEAT: 'heat'>, <HVACMode.FAN_ONLY: 'fan_only'>, <HVACMode.DRY: 'dry'>], min_temp=63, max_temp=86, target_temp_step=1.0, fan_modes=['auto', 'low', 'medium', 'high', 'silent', 'turbo'], preset_modes=['none', 'boost', 'eco', 'sleep', 'freeze protection'], swing_modes=['off', 'both', 'vertical', 'horizontal'], current_temperature=78, temperature=78, fan_mode=auto, preset_mode=none, swing_mode=off, friendly_name=Boys Room, supported_features=441 @ 2024-05-30T02:43:43.659526-04:00>
2024-05-30 04:00:00.814 INFO (MainThread) [custom_components.versatile_thermostat.thermostat_climate] VersatileThermostat-Boys Room VTherm - Underlying climate climate.midea_climate_3 changed. Event.new_hvac_mode is off, current_hvac_mode=cool, new_hvac_action=None, old_hvac_action=None

Edit: It ran overnight without issue after this commit (I don't know if this commit really fixed anything). At this point, I believe it is working properly, but I will leave it on for longer to make sure it still works as expected.

mag2352 commented 4 months ago

Has anyone took a look at this? I've been using my fork without issue for a while now, but I suspect that @jmcollin78 is hesitant on this idea because this would decouple the state of the VTherm and the underlying climate entity. Is there a reason that the two climate entities have to match in status? I feel like the current behavior is a large oversight when considering window detection and scheduling integrations.

jmcollin78 commented 4 months ago

but I suspect that @jmcollin78 is hesitant on this idea because this would decouple the state of the VTherm and the underlying climate entity

Exactly. I suspect some user impact: when looking to the thermostat it could looks like 'on' and the underlying 'off'.

This is specially annoying for Vtherm over climate: in that case, the VTherm should follow the underlying state and both should be synchronized. The VTherm state must reflect the device state.

But I keep this idea as enhancement to have a deeper look inside this idea when I will have time.

jmcollin78 commented 1 month ago

See comment in https://github.com/jmcollin78/versatile_thermostat/issues/465

So I do not wish to implement this. I hope you will understand. If I miss something, please comment or reopen.