Deprecation warning: implements HVACMode(s): heat, auto, off without setting the proper ClimateEntityFeature
Opened this issue · 14 comments
After upgrading to HA 2024.2.x I'm seeing this warning in the log:
2024-02-09 16:48:41.351 WARNING (MainThread) [homeassistant.components.climate] Entity None (<class 'custom_components.warmup.climate.WarmupThermostat'>) implements HVACMode(s): heat, auto, off and therefore implicitly supports the turn_on/turn_off methods without setting the proper ClimateEntityFeature. Please create a bug report at https://github.com/ha-warmup/warmup/issues/
I'm running the beta version "v2024.2.7" of ha-warmup.
I suspect it's related to this change in HA
https://developers.home-assistant.io/blog/2024/01/24/climate-climateentityfeatures-expanded
There's a 10 month window until HA 2025.1 for resolution
Just was going to open an issue for this :-). I looked a bit at the code and only have one question: what's the right behavior of "turn on"? Set to Auto? Heat? Not sure we always know the last state before off.
My vote would be for Auto. I assume this means if someone calls the service homeassistant.turn_on or climate.turn_on or turn_off.
I'll give it a try.
Thanks for the detailed info in the issue, @peternash
I'd second you, @robchandhok in going for Auto, in the absence of any other steer
@robchandhok - Auto for "turn on" behaviour makes sense to me
thanks. I have the warnings gone, but the functionality of being able to cal generic on/off doesn't work (throws an error). See robchandhok@7c64101
I'm not sure we can do async operations to the library we use for Warmup.
Thanks for making the wrapper happy about this issue, superficially @robchandhok. When you say 'library', although it historically was published to PyPi as a library, the code that is called by this integration now is actually shipped as an internal library in https://github.com/ha-warmup/warmup/blob/master/warmup4ie-PyPi/warmup4ie/warmup4ie.py which means that we are in full control of that code. Is it worth pasting in the details of the error that is thrown so that we can look at that library (which explains the odd name in the debug settings) and work out what we could do to fix the code in there?
It turns out there's no need to do asynchronous in this integration- HA handles that by using a different thread for "old style" integrations. So my comment was incorrect.
Ok, that's good. So where does that leave the code in these commits? Does it need tweaking to withdraw features or to catch and warn on errors it encounters? What needs doing before wrapping it for QA testing and release?
The code has been running in my production instance for a while now. I consider it ready, and it's a low impact change since previously you could not call the generic "turn on" ... so there's no backwards compatibility to worry about.
Along the way I discovered the long standing behavior that turning off a device turns off the entire location. That's because of the warmup API. This will be discussed in a different issue.
I would release both deprecation fixes to production version.
Hi @robchandhok - I've downloaded the latest "dev" version and confirmed it seems to work OK with some caveats (below) and eliminates the deprecation warning on my system (HA 2024.2.3, WarmUp 6iE, single room).
I'd also noticed that the "turn off" service affects the entire "Home", not just the selected "room". As I only have one room I can't test the implications of this but I assume that in a multi-room setup calling the generic "turn off" on a single room would turn off all rooms. The "turn off" service call also disables frost protection but that probably makes sense for a true "turn off".
I also noticed that the generic "turn on" seems to leave WarmUp in an inconsistent state. The "Home" is still set to "Off" as indicated on both the WarmUp App and their web interface, but the room appears to be set to "Auto" as expected. However, the "room" display in the app doesn't show the "Auto" timeline as it normally would. This doesn't seem to have any negative impact on the operation of the WarmUp system though.
The same behaviour occurs when using the Climate control UI in HA to switch modes between "Off" and "Auto" so this is an API issue as you noted, not a result of fixing the deprecation warning.
I can't see any reason not to release this either, the API issues exist in the current version and are a separate issue as you say. Also, with this change the generic "turn on" sets the mode to "Auto" which makes more sense than "Fixed Temperature" which appears to be the case in v2024.2.7 (and presumably earlier releases).
You have discovered the crux of the issue, which is been around since this was made.
The Warmup API does not appear to have a "turn room off" call, just a "turn location off" call. Every implementation I can find of the protocol has a comment to that effect.
I am looking at making a synthetic version of "room off" that turns the location off, and then restores the other rooms- having the effect of turning a single room off. That's in a different issue.
I agree with you that we are still better off releasing this, but should probably vote the effect of "turn off" in the notes.
@robchandhok - WarmUp's own implementation of this seems inconsistent. In their web UI there is a button to turn a room "Off" but in the iPhone app you have to "change mode" to "fixed" and then dial the temperature down to "Off" (in the web UI 5ºC is the minimum). I've noticed that this seems to set a fixed temperature of 0ºC. Out of interest I just tried calling the HA service to override the room temperature to 0ºC and that shows the room as "Off" in the iPhone app. In their web UI it shows "Fixed Temp 0ºC" which is not quite the same as "Off" but has the same effect. I wonder if that's a short term workaround?
Here's the service call:
service: climate.set_temperature
target:
entity_id: climate.es_bathroom_underfloor
data:
temperature: 0
hvac_mode: heat
Thanks for both adding plenty of detail on this matter, that helps us to build a good common understanding of the product as supplied by the vendor. I too have noticed 'particular behaviours' where these products do not necessarily act in a way that one might expect, nor in a way that fits the paradigms that an ecosystem like Home Assistant expects from generic HVAC or Climate device and services.
You have already noted some of them, but another one that I might add is that when you use the Override feature, the room will be set back to Auto once the time expires, even if it was set to Off before you set Override.
(I wrote this just before you posted your inconsistency, Peter). As you both mentioned, this seems to be because the logic is built into the operation of the product. My experience is that, whether you instigate a setting:
- via the API (e.g. from this integration in Home Assistant)
- via the vendor's web client (https://my.warmup.com/)
- via the physical control panel installed in the room and attached to the heating elements
these 'particular behaviours' seem to be consistent. I guess that on the whole that is a positive thing, because we have multiple ways to test and validate the product's operation. And yes, we should clarify in our documentation the actual behaviours of operation that we discover, to help set HA users' expectations.
However, I am slightly hesitant about deciding to code into this integration features that attempt to manipulate devices in clever ways, like the Room Off that you propose above Rob. There is a part of me that feels this might be better handled by an Automation in HA itself, rather than in code inside the integration. My reasoning is that this leaves each HA user in control of their own environment, by us recommending a way of implementing it, instead of us imposing usage patterns via our architectural decisions baked into code. I am open to persuasion on that, though.
Anyhow, back to the issue in hand, I will release this deprecation change into a Prerelease (Beta) at some point during the week. Cheers
Thanks to @peternash and @artmg for the comments.
I actually agree with Peter that "turn off" against a climate object in HA should not have side effects if possible, regardless of the warmup apps own behavior. (Specifically I think the point of an HA integration is to map native behavior to HA's semantic model. Otherwise nothing would be consistent)
I think the set fixed temp,approach is better than what is there currently, but will address in a new issue.
Pre-Released in https://github.com/ha-warmup/warmup/releases/tag/v2024.3.6
We can wrap the discussions and decisions up into a new issue at some point
Then after a couple weeks in pre-release we can release this fix to production and close this issue - thanks rob for your code and to both for all your input