Make generic_thermostat only turn on when current temp is below target temp
Proposed change
Change the generic_thermostat so that it only turns on when the current temperature is below (not equal to) the target (including tolerance).
Reasoning is that if the current temperature is within range of the target (including tolerance), it shouldn't be heating/cooling because the temperature is considered OK. Only when the current temperature drops above/below the target temperature should the thermostat turn on.
Type of change
- [ ] Dependency upgrade
- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New integration (thank you!)
- [ ] New feature (which adds functionality to an existing integration)
- [ ] Deprecation (breaking change to happen in the future)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests
Additional information
- This PR fixes or closes issue: fixes #79667
- This PR is related to issue: #79667
- Link to documentation pull request: home-assistant/home-assistant.io#37545
- Link to developer documentation pull request:
- Link to frontend pull request:
Checklist
- [x] The code change is tested and works locally.
- [x] Local tests pass. Your PR cannot be merged unless tests pass
- [x] There is no commented out code in this PR.
- [x] I have followed the development checklist
- [x] I have followed the perfect PR recommendations
- [x] The code has been formatted using Ruff (
ruff format homeassistant tests) - [x] Tests have been added to verify that the new code works.
If user exposed functionality or configuration variables are added/changed:
- [x] Documentation added/updated for www.home-assistant.io
If the code communicates with devices, web services, or third-party tools:
- [ ] The manifest file has all fields filled out correctly.
Updated and included derived files by running:python3 -m script.hassfest. - [ ] New or updated dependencies have been added to
requirements_all.txt.
Updated by runningpython3 -m script.gen_requirements_all. - [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
To help with the load of incoming pull requests:
- [ ] I have reviewed two other open pull requests in this repository.
We need a testcase for this.
We need a testcase for this.
Ok - I'll try my best to see if I can figure those out - I'm very new to Python and never done test cases... I'll try and seek some help :slightly_smiling_face:
I see it's failing two tests where it's checking if the current temperature is "within tolerances". Thank you for flagging the tests (leading to me figuring out how they work). I've converted this back to draft while I re-work the logic on the conditions causing it to fail the tests.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:
The title and description mentions only the lower/cold temp, but the PR also changes the behavior of the higher temp, right?
Also, this changes the current behavior of the integration. I think it should be noted as a breaking change on the PR description.
Yes and no. You could argue that turning on when the room temperature was at the target temp was a bug. 😄
Yes it is a change that at the lower end, for heating, it has to be below target before turning on rather than when = to target. I'd argue it isn't a breaking change, but it is one that may need a configuration change (simplest is to change the step to make it more sensitive).
The title and description mentions only the lower/cold temp, but the PR also changes the behavior of the higher temp, right?
It's been a while since I worked on this, so I don't recall exactly if it also affects higher temp, but the title could be changed so that it was "...current temp is above/below target temp" since the change does ensure that the current temp is outside of the tolerable range before turning on.
Also, this changes the current behavior of the integration. I think it should be noted as a breaking change on the PR description.
I'd agree with what @borpin said in that I would argue this isn't a breaking change, but a bugfix. It only impacts edge cases where it would turn the thermostat on when the current temperature was on the cusp of the tolerable range. Having a zero tolerance caused the thermostat to toggle on/off when the temperature was exactly at the set point.
This fixes the issue so that the thermostat will only turn on once the temperature is beyond the tolerable range. If you have a zero tolerance set, it ensures that the thermostat doesn't toggle on/off if the temperature remains at the set point.
The title and description mentions only the lower/cold temp, but the PR also changes the behavior of the higher temp, right?
I have updated the PR title so that it's clearer that the fix is to prevent the thermostat from turning on if the current temp is within the target temp range.
Also, this changes the current behavior of the integration. I think it should be noted as a breaking change on the PR description.
I'd agree with what @borpin said in that I would argue this isn't a breaking change, but a bugfix. It only impacts edge cases where it would turn the thermostat on when the current temperature was on the cusp of the tolerable range. Having a zero tolerance caused the thermostat to toggle on/off when the temperature was exactly at the set point.
Lets assume the following example:
- mode: heat
- target temp: 20
- cold tolerance: 1
- current temp: 19
With this change, the switch stays off, because 19 is within the range. It will turn on only at 18.9.
Before the change, the switch would turn on at 19.
So, since this fixes an issue but changes the current behavior, I think we should mark it as breaking change so users are made aware of the change in behavior.
Or did I misinterpret it?
So, since this fixes an issue but changes the current behavior, I think we should mark it as breaking change so users are made aware of the change in behavior.
I've made the change, though I'd argue this is on the cusp of a breaking change, as it doesn't break anything - it just alters when the thermostat would engage in that the defined range is now inclusive... though based on the documentation for the settings, it should have always functioned the way this PR makes it work.
It will turn
ononly at 18.9
Strictly, I think it will turn on depending on the step size of your sensor. If the temperature sensor is to 2 DP it will switch on at 18.99, if it is half a degree, at 18.5.
It will turn
ononly at 18.9Strictly, I think it will turn on depending on the step size of your sensor. If the temperature sensor is to 2 DP it will switch on at 18.99, if it is half a degree, at 18.5.
Right, I was assuming 0.1 resolution in my example. Still, before this change it would have triggered at 19 already. That is the behavior change that is a breaking change, in case someone is relying on that behavior right now.
This is another example of a similar change that was also flagged as a breaking change: https://github.com/home-assistant/core/pull/138819