core icon indicating copy to clipboard operation
core copied to clipboard

Make generic_thermostat only turn on when current temp is below target temp

Open esand opened this issue 10 months ago • 6 comments

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:

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 running python3 -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:

esand avatar Feb 10 '25 15:02 esand

We need a testcase for this.

elupus avatar Feb 10 '25 20:02 elupus

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:

esand avatar Feb 10 '25 21:02 esand

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.

esand avatar Feb 11 '25 02:02 esand

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Mar 14 '25 18:03 home-assistant[bot]

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).

borpin avatar Mar 20 '25 08:03 borpin

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.

esand avatar Mar 20 '25 23:03 esand

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.

esand avatar Mar 28 '25 23:03 esand

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?

abmantis avatar Mar 29 '25 00:03 abmantis

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.

esand avatar Mar 29 '25 15:03 esand

It will turn on only 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.

borpin avatar Mar 30 '25 14:03 borpin

It will turn on only 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.

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

abmantis avatar Mar 30 '25 18:03 abmantis