[BUG] New `heat_cool` mode causes UI-Issues with target_temp

[david-kalbermatten]

Context

I already hinted at this issue in my PR for the Fan and Dryer mode #8. However, it seems to be a general problem where you either have a dual-mode thermostat that supports :

target_temp_low
target_temp_high

OR

a single-mode thermostat that just supports:

target_temp

Issue

Right now the integration supports all three which doesn't seem to be supported by Home Assistant's UI...

There is no way to directly control the target_temp from the UI, which cannot be the intended use case. (second picture)

Possible Solution (1)

Ditch target_temp altogether. I don't have enough experience with how other integrations handle this but I think that you are only supposed to support SUPPORT_TARGET_TEMPERATURE or SUPPORT_TARGET_TEMPERATURE_RANGE and not both which seems to be the case at the moment.

Possible Solution (2)

Tell the GUI to only show the relevant controls. When the thermostat is in cool mode, only expose target_temp and hide target_temp_low and target_temp_high. And vice-versa when in heat_cool mode. (I doubt this is how you are supposed to do it...)

[david-kalbermatten]

@lossanarch I'd highly appreciate it if you could tell us whether you tested the integration in heat and cool mode after adding heat_cool. Being able to set the target_temp is crucial if you don't intend to always have it running in heat_cool mode.

Not to cause bad blood, I love the added functionality but I think they came at the cost of what was already working...

[lossanarch]

@david-kalbermatten I thought I tested this during early development changes and it would switch back to target temp mode when in heat/cool modes, but you are right, it certainly isn't working now. Whether I broke it later and didn't notice since I use it almost exclusively in heat_cool mode or whether an update has changed something, I don't know.

I've taken a look around at a few codebases and it does seem that there is some feature gating going on based on config. swingerman/ha-dual-smart-thermostat, for example, gates support_target_temperature_range based on whether there is a cooler configured. Although some testing would be required to verify, I feel as though this would be a load-time flag and would require a reload to change, which I think would rule out Possible Solution (2).

I think there are two short term solutions here:

1. Add a configurable feature flag, something like a boolean use_temperature_range. I think this might be better than dropping support altogether for target_temperature. It would also not take too long to push the fix for as it's a simple check.
2. Revert the commits from the PR and I can resubmit later.

I don't particularly like 1 as it won't be particularly satisfying for those that want to use all modes, however the limitation may be on home-assistant's side and it may not be fixable in which case even long term it may be the only option. Personally I think it's better to expose the behaviour in a flag explicitly rather than taking the swingerman/ha-dual-smart-thermostat approach of implicitly changing the behaviour, but I'm happy to be convinced otherwise.

Finally, no bad blood - I apologise for not testing the previous functionality more thoroughly before opening the PR.

@zacs I'll try to have a PR for 1 in about 12h or so, but if you'd like to go with 2 to avoid possible quick-fix bugs or spend more time on investigating possible solutions/workarounds feel free to revert.

[milandzuris]

I like the new BUG I have new tweaks that I would very much like to adopt in HA.

[zacs]

@lossanarch It might make the most sense to check for the _low and _high vars at instantiation and if present, ignore the target_temp, otherwise use only target_temp. I agree that your option 1 is a little hacky (or overly explicit) but it at least solves the issue, particularly since it's backwards compatible.

I'm mostly just surprised that more than 3 people use this component ;). We'll get it sorted in the next day or so.

[zacs]

@david-kalbermatten Would you mind trying out d1670c9 (the heat_cool-fix branch) and seeing if this solves the issue? Or if anyone else has the desire to do so, it would be much appreciated.

I believe this should fix it during initialization of the component, but unfortunately I don't use the component in my production env any more and I have no test (though I will now fix that for the future!) environment.

[lossanarch]

@lossanarch It might make the most sense to check for the _low and _high vars at instantiation and if present, ignore the target_temp, otherwise use only target_temp. I agree that your option 1 is a little hacky (or overly explicit) but it at least solves the issue, particularly since it's backwards compatible.

I like this a lot better than my one 👌 I’ll test out the fix branch tonight, sorry I couldn’t get to it sooner.

[david-kalbermatten]

@david-kalbermatten Would you mind trying out d1670c9 (the heat_cool-fix branch) and seeing if this solves the issue? Or if anyone else has the desire to do so, it would be much appreciated.

I believe this should fix it during initialization of the component, but unfortunately I don't use the component in my production env any more and I have no test (though I will now fix that for the future!) environment.

Didn't work because the code of @lossanarch expects to always have _low and _high available and it would also completely remove the feature of heat_cool.

I propose that we should rather use the target_temp_high for the heater when checking if the temperature has been reached and target_temp_low for the cooler and ditch target_temp.

[david-kalbermatten]

Alright, I fixed it on my fork, I set it up that when both heater and cooler are configured the flag SUPPORT_TARGET_TEMPERATURE_RANGE is used otherwise it uses SUPPORT_TARGET_TEMPERATURE.

I also adjusted the temperature check functions to check for the SUPPORT_FLAG instead of the HVAC_MODE

    def _is_too_hot_activate(self):
        if self._support_flags == SUPPORT_TARGET_TEMPERATURE_RANGE:
            return self._cur_temp >= self._target_temp_high + self._hot_tolerance
        else:
            return self._cur_temp >= self._target_temp + self._hot_tolerance

So in summary: If only one entity is set, the regular target_temp is used if both entities are set it uses the range instead and the temperature checks are set up to do the same. I fixed it on my Merge Request though, so if you want the fix you'll have to (finally) merge it. It should be fully backward compatible.

[lossanarch]

I've been working on the heat_cool-fix branch, it seems the right way to handle it but unfortunately there are a lot of checks to do or AttributeError exceptions to handle since everything (inc. the original generic thermo) is written with the assumption that those properties are available. I've now opened a draft PR #24.

@david-kalbermatten I don't think it's robust to check the support flags that way, as they are bitwise OR'd during init. I've used if self._support_flags & SUPPORT_TARGET_TEMPERATURE_RANGE == SUPPORT_TARGET_TEMPERATURE_RANGE: in async_set_temperature in my PR and I think it works (i was able to set temperatures in all modes), although i haven't done bitwise checks in python before so if something looks amiss let me know.. As it stands above the check will break if away_mode is configured due to the extra PRESET flag.

I also figured out why I thought it worked - a few months ago when I started working on this I used homekit as my frontend, and the exposed thermo in homekit works great with both and will switch between temp modes based on the operation mode. I've since moved to lovelace but obviously hadn't checked it again.

It does appear that the support flags are the key, if target temp range is enabled then that is the only thing the lovelace card shows. I almost think this should be a bug lodged against the frontend repo given the amount of stuff we need to do to work around it.

[david-kalbermatten]

It does appear that the support flags are the key, if target temp range is enabled then that is the only thing the lovelace card shows. I almost think this should be a bug lodged against the frontend repo given the amount of stuff we need to do to work around it.

Either that or we're missing something... I noticed the same behaviour in Google Home as well, so it would seem that you are supposed to use both target_temp and target_temp_range but Home Assistant doesn't support that properly or needs more tweaking :/

[zacs]

Thanks for digging into this @david-kalbermatten and @lossanarch - it does actually seem like a bug in the frontend. Even in the docs, there is no mention of one needing to limit the number of modes.

That said, I think there is still the issue of support flags. I think we can solve that by:

1. Reverting line 81 to be just SUPPORT_FLAGS = SUPPORT_TARGET_TEMPERATURE
2. Revert my conditional fixes that skip the _high and _low but continue to have a conditional for the modes
3. Add a check just line line 216, but make it check for _high and _low and do the bitwise or with SUPPORT_TARGET_TEMPERATURE_RANGE there

Outside of the UI, I think this would fix it, but again I have no test environment at the moment. Do those changes make sense to you both? Am I missing anything?

[zacs]

Outside of the funky UI, I believe 884bc66 should fix things. I think the three of us are nearly exactly a third of a planet away as far as time zones, but let me know if either of you can kick the tires. I'm going to setup a new docker env to test this junk in the meantime.

Again, appreciate the patience and engagement.

[david-kalbermatten]

Outside of the funky UI, I believe 884bc66 should fix things.

I left a comment on this commit with a reference to the lines in my PR. I set it up so it's backwards compatible and also allows you to completely turn of heat_cool with the optional flag:

enable_heat_cool: false

By default, if you have both configured (heater and cooler), you would also get the heat_cool-mode with SUPPORT_TARGET_TEMPERATURE_RANGE. Otherwise, you'll get SUPPORT_TARGET_TEMPERATURE and the heat_cool-mode will not be shown and the integration works like before.

If you want the default to be the other way around I would only have to change one line of code to achieve it... https://github.com/zacs/ha-dualmodegeneric/blob/4c2e5e01b0450ec8ade1001bf7f5e7bf5537f031/custom_components/dualmode_generic/climate.py#L128

[david-kalbermatten]

@zacs If you want my implementation to work exactly the same as yours in https://github.com/zacs/ha-dualmodegeneric/commit/884bc663ec19b559f0bd1bd5943870fca792486e, I can do that. In which case I would set the default of enable_heat_cool to False and set the support flags like this for when heat_cool is enabled

self._support_flags = SUPPORT_FLAGS | SUPPORT_TARGET_TEMPERATURE_RANGE

...and like this for when it is not:

self._support_flags = SUPPORT_FLAGS

This approach would work correctly in Google Home and Apple HomeKit, whereas my previous implementation would have caused issues when trying to use heat/ cool mode when only SUPPORT_TARGET_TEMPERATURE_RANGE was set in supported flags for heat_cool.

[zacs]

My goal was to enable heat_cool implicitly based on whether the _high and _low targets were both set. Just trying to avoid extra config params if at all possible while maintaining back compat.

[david-kalbermatten]

I do see why you might want that but that doesn't make a whole lot of sense in my opinion since those values are adjusted frequently... And it makes it more confusing for people who stumble across this custom integration since there is no documentation about it on the generic thermostat from the core.

But if you insist on having it implicit I'll change it ;D

[zacs]

I think I see what you're saying. I want to see how it works in the frontend either way to make sure it isn't a terrible experience. Will add to docs either way. Thanks!

[lossanarch]

@zacs Apologies for the radio silence, had a busy couple of days. I've checked 884bc66 and found it works as I would expect it to. For thermos with a /_high|_low/ value set they only show the temperature range slider (as expected). If switched to heat or cool modes they begin operating from the target_temperature setting (as expected) which unfortunately must be accessed via the details pane in lovelace (at this point, also probably expected). Thermos without /_high|_low/ set show the correct single temperature set point and appear to operate correctly (as expected).

This means a trade off, either: A. Specify target_temp_/high|low/ when heat_cool mode is desired, and accept having to dive into the details pane to adjust the target_temperature setting when using heat/cool mode for now. B. Avoid using target_temp_/high|low/ when the main modes in use are heat/cool or if heat_cool is not desired at all.

This is a bit of a bummer but also think it really shouldn't be fixed/worked around here but instead in the frontend repo.

Personally I think it's intuitive that the temperature range support flag be activated implicitly as soon as /_high|_low/ targets are specified - without it they are completely unusable (unlike the regular target temp which can at least be accessed via a less-than-optimal method). So imho I like the implicit method rather than a boolean param.

[david-kalbermatten]

So, that's 2 out of 3 in favor of the implicit approach. I'll change my PR to reflect this decision. Can we merge my changes with the fix or do you want to only incorporate the fix for heat_cool? @zacs

I'll change it to implicit checking instead of the boolean flag, but if I don't get to do it this evening, it'll be Friday until I have time to work on it again.

[hacker-cb]

I also suggest to make heat_cool mode optional. In most cases user can select exact heat or cool mode by himself.

Three controls (target,min,max) in GUI is too complicated for simple user like my wife :)

[david-kalbermatten]

Alright, I had time to make the changes and it also fixes issue #25 since there's always a high and low value defined when using heat_cool. (heat_cool is only available when target_temp_high/low are set in config)

@zacs Everything is set up in a way that people can just update and not notice any difference since I made all my changes (#8) backwards compatible. This should allow people to smoothly transition.

[girlpunk]

Been trying out a few of the proposed solutions to this today. David's fix currently looks the best, though I think it may need one small change. With one of the earlier commits in the fix, the SUPPORT_TARGET_TEMPERATURE flag was only added if the SUPPORT_TARGET_TEMPERATURE_RANGE wasn't added. This results in the UI working much better (see attached screenshots)

As a semi-related note that might be of interest to folks here, I've just submitted a very early PR for Schedy, to allow it to work correctly with the dual-mode thermostat in heat/cool mode.

[david-kalbermatten]

With one of the earlier commits in the fix, the SUPPORT_TARGET_TEMPERATURE flag was only added if the SUPPORT_TARGET_TEMPERATURE_RANGE wasn't added. This results in the UI working much better (see attached screenshots)

Yes, this was my initial approach. However, it seems that you are supposed to expose both flags at the same time because Google Home and Apple HomeKit work without an issue. This indicates that Home Assistants UI has a bug or a poorly documented workaround... Either way, we shouldn't put my initial workaround in because it breaks functionality with Google Home and Apple HomeKit.

My current version uses implicit checking of the YAML-Config for the following values:

• target_temp_high
• target_temp_low
• heater
• cooler

If all of them are set, you "unlock" the heat_cool mode and SUPPORT_TARGET_TEMPERATURE_RANGE is set together with SUPPORT_TARGET_TEMPERATURE. So if you just want to use my additions without heat_cool you just need to omit target_temp_high/low from your config and everything works as it used to before.

[zacs]

Thanks for your ongoing work here @david-kalbermatten!

One question while I am looking through your code (finally -- sorry, life is busy): should target_temp_high (and low) be config values? They come from the UI/state of the entity, right? I'm trying to understand why they'd even be declared when target_temperature itself was never a config param.

I feel like we have to (internally) have all three in this case. And then we either do either:

1. Have a boolean config param for "do you want to do ranges?" (your original solution, which I think I am coming around to).
2. Always have HEAT_COOL available (since by definition heater and cooler are required), and use the _high and _low values when in that mode. But when in HEAT or COOL mode the normal target_temperature is used as it always was. This introduces a whole new slew of conditional logic in the guts of the plugin's logic.

Your PR has gotten quite huge now so I am still walking through it, but thought it may be useful to discuss those 2 options sooner than later.

[david-kalbermatten]

One question while I am looking through your code (finally -- sorry, life is busy): should target_temp_high (and low) be config values? They come from the UI/state of the entity, right? I'm trying to understand why they'd even be declared when target_temperature itself was never a config param.

I don't know, I didn't introduce them. That came from @lossanarch.

1. Have a boolean config param for "do you want to do ranges?" (your original solution, which I think I am coming around to).

I love it, you're finally starting to see reason :D. I can revert back to an explicit boolean flag for enabling heat_cool. But keep in mind, this would also mean that issue #25 will have to be investigated further.

2. Always have HEAT_COOL available (since by definition heater and cooler are required), and use the _high and _low values when in that mode. But when in HEAT or COOL mode the normal target_temperature is used as it always was. This introduces a whole new slew of conditional logic in the guts of the plugin's logic.

No, I wouldn't do that for two reasons:

1. Not everyone has both set up (heater & cooler). My PR aims to bring more modes (fan_only, dryer) and also makes both cooler and heater optional. This way you can have a truly generic thermostat (in my case: cooler, dryer, fan_only). This allows me to correctly represent my HVAC in Home Assistant.
2. Not everyone wants to use heat_cool most people probably live in a climate where they only have to heat in winter and only have to cool in summer. It would suck if you forced people to use both target temp and target temp range because right now the Lovelace UI is still broken in this regard.

I think we should make heat_cool optional until the Lovelace UI catches up or we find a workaround that doesn't impede existing functionality i.e. only show the relevant target temps when in a specific mode (maybe we can change the SUPPORT flag based on the currently selected HVAC_MODE)

[zacs]

I'm still a bit wary to turn this from "dual mode generic" to full on "template thermostat" but if that's the direction folks are interested in, I don't object to increased functionality, as long as thing remain backwards compatible.

In any case if we add back your original boolean for supporting temp range, then we can just set SUPPORT based on that and be done with it, though we'd also need to check that both heater and cooler are present.

Going to need a full table of params in the README for sure if we go down this path. Feel free to pull your boolean back into the PR and I'll finish up review in the next couple days. Work and family continue to be 99% of my time so appreciate you sticking with it.

[david-kalbermatten]

@zacs Alright, I added back the boolean flag enable_heat_cool and set its default value to False. This should allow people to update without having issues.

But we still have to investigate issue #25. Seems like it only restores some of the target_temperatures when starting HA...

[girlpunk]

Personally, I'd be in favour of both enable_heat_cool (with a sensible default for existing users) and extending the integration's functionality further.

I was considering building one myself, to fully handle all the sensors (multiple temperature, humidity, and air quality) and actions (heat, cool, dehumidify, fans, and air purifiers) that Home Assistant can handle, because it seems like there's no good integration to actually manage all of that yet. If the dual mode thermostat (or a clone/fork of it) is moving in that direction, I'd be more than happy to help.

[bkvargyas]

I've been watching all the activity from behind the scenes, and while I'm not a developer, I am interested in seeing this project move forward into a full fledged software thermostat, agreed that there seems to be no good integration today that can do this. With ESPHome now into the ecosystem, this project becomes more relevant for those that wish to "manually" control IoT devices. Personally, I need humidity and dehumidify controls (in addition to stage 1 and stage 2 heating and cooling), but willing to test anything at this point.

[girlpunk]

Guess I'll post this one here, as @david-kalbermatten's branch has issues disabled (looks like a GitHub default fwiw). I'm still running the latest version of David's branch (5e5d90f) and have noticed an error popping up fairly regularly, though I'm not certian it's only since the last commit. It appears to occur when the thermostat is running in heat/cool mode and idle, when the room temperature moves above the upper setpoint (i.e. the system now needs to start cooling). As a result the thermostat remains in idle, and the cooling doesn't switch on.

Full traceback, which suggests the problem is around here somewhere:

Traceback (most recent call last):
  File "/config/custom_components/dualmode_generic/climate.py", line 654, in _async_sensor_changed
    await self._async_control_heating()
  File "/config/custom_components/dualmode_generic/climate.py", line 722, in _async_control_heating
    active_entity,
UnboundLocalError: local variable 'active_entity' referenced before assignment

Not needed any heating recently, so I can't be sure this only affects cooling. Happy to post the relevent config and do any other testing if required. I'm about to turn on debug logging so will post that too if it turns up anything useful.

[david-kalbermatten]

@cyberjacob Yea, you're right I see the issue. I overlooked a case there.

This was the original code:

if self.min_cycle_duration:
    entity = self.cooler_entity_id if self._hvac_mode == HVAC_MODE_COOL else self.heater_entity_id

    if self._is_device_active:
        current_state = STATE_ON
    else:
        current_state = HVAC_MODE_OFF
    long_enough = condition.state(
        self.hass,
        entity,
        current_state,
        self.min_cycle_duration,
    )
    if not long_enough:
        return

This is my version of the code:

if self.min_cycle_duration:
    if self._hvac_mode == HVAC_MODE_COOL:
        active_entity = self.cooler_entity_id
    if self._hvac_mode == HVAC_MODE_HEAT:
        active_entity = self.heater_entity_id
    if self._hvac_mode == HVAC_MODE_FAN_ONLY:
        active_entity = self.fan_entity_id
    if self._hvac_mode == HVAC_MODE_DRY:
        active_entity = self.dryer_entity_id
    if self._hvac_mode == HVAC_MODE_HEAT_COOL:
        if self.hass.states.is_state(self.heater_entity_id, STATE_ON):
            active_entity = self.heater_entity_id
        elif self.hass.states.is_state(self.cooler_entity_id, STATE_ON):
            active_entity = self.cooler_entity_id

    if self._is_device_active:
        current_state = STATE_ON
    else:
        current_state = HVAC_MODE_OFF
    long_enough = condition.state(
        self.hass,
        active_entity,
        current_state,
        self.min_cycle_duration,
    )
    if not long_enough:
        return

The original check always returned either the cooler-Entity or the heater-Entity. And my original version didn't have any case where the active_entity wasn't set. However, I had to add a case for heat_cool where @lossanarch used what was there already which always returned the heater-Entity.

My approach is missing a fallback assignment. I could use the heater-Entity as the original version used but I don't know whether or not this will cause other problems.

My solution would look something like this...

[...]
if self._hvac_mode == HVAC_MODE_HEAT_COOL:
    if self.hass.states.is_state(self.cooler_entity_id, STATE_ON):
        active_entity = self.cooler_entity_id
    else:
        active_entity = self.heater_entity_id
[...]

I'll push this change to my fork. @cyberjacob Please tell me whether this fixes your issue.

[girlpunk]

Given the AC immidiately switched on, looks like that's fixed it. I'll keep an eye out for any side-effects.

[david-kalbermatten]

I'll close this issue for now, since the problem lies with the frontend (or that's at least what we think the issue is...). And the workaround gives the user the choice of using heat_cool and dealing with three target temperatures or leaving it disabled and have it work like it used to before.