Skip to content

Generic thermostat presets#56080

Merged
bdraco merged 6 commits intohome-assistant:devfrom
brianegge:generic_thermostat-presets
Dec 24, 2021
Merged

Generic thermostat presets#56080
bdraco merged 6 commits intohome-assistant:devfrom
brianegge:generic_thermostat-presets

Conversation

@brianegge
Copy link
Copy Markdown
Contributor

@brianegge brianegge commented Sep 10, 2021

Proposed change

Generalized the away preset to suppose all listed presets in the climate const file. *Does not break anything - does not require any changes to yaml. *

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

The generic thermostats only supports one preset: away. This is sad. An abandoned PR attempted to refactor the whole class. This PR simply extends it to the other presets.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

PRESET_BOOST,
PRESET_COMFORT,
PRESET_ECO,
PRESET_HOME,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't reviewed other implementations of the climate component but does it make sense?
It seems confusing to me to have the PRESET_HOME and None because I understand that they are the same use case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely different. Consider None like manual or hold. None is the only preset you can't set.

@fjufju

This comment has been minimized.

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Oct 22, 2021
brianegge added a commit to brianegge/home-assistant.io that referenced this pull request Nov 11, 2021
@brianegge
Copy link
Copy Markdown
Contributor Author

Added documenation PR home-assistant/home-assistant.io#20264

@brianegge
Copy link
Copy Markdown
Contributor Author

hello?

brianegge added a commit to brianegge/home-assistant.io that referenced this pull request Nov 23, 2021
brianegge added a commit to brianegge/home-assistant.io that referenced this pull request Nov 23, 2021
@brianegge
Copy link
Copy Markdown
Contributor Author

Everyday I'm hopeful this will get merged

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Dec 20, 2021

I'm struggling to see the value here. How is this different than an automation to set a specific temperature ?

@Misiu
Copy link
Copy Markdown
Contributor

Misiu commented Dec 20, 2021

I'm struggling to see the value here. How is this different than an automation to set a specific temperature ?

Correct me if I'm wrong, but I think that the main advantage is the frontend control. Currently generic thermostat lacks preset selection. With presets you will get Eco mode, boost mode and you will be able to select them from thermostat control. This will also aligh generic thermostat with many other thermostats

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Dec 20, 2021

Correct me if I'm wrong, but I think that the main advantage is the frontend control. Currently generic thermostat lacks preset selection. With presets you will get Eco mode, boost mode and you will be able to select them from thermostat control. This will also aligh generic thermostat with many other thermostats

What is the advantage over simply selecting the temperature ?

@brianegge
Copy link
Copy Markdown
Contributor Author

What is the advantage over simply selecting the temperature ?

The advantage is I can have a routine set every thermostat to 'Away' when I leave the house. This works for my wall ACs, my Ecobees and even my woodburning stove. Same thing for Night mode. These temperatures are different for different rooms in the house. The 'Night' temperature for the bedrooms is higher than for the office.

Further, when the thermostat is running in a preset, you can assume it's on a schedule, but when it's changed up/down, it is not on a present and is on a hold. So I can have a routine which says turn all thermostats which are set to away to home when the house cleaners arrive, and when they leave set them back to away. The automation will skip my office because it's not in away mode, not adjust the temperature when they arrive or when the leave.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Dec 20, 2021

That's makes sense for everything except boost and eco. Those look more like modifiers vs schedule overrides. Is there a strong reason for keeping them?

@brianegge
Copy link
Copy Markdown
Contributor Author

I don't use either boost or eco, but there are about 19 other integrations which use boost and 25 which use eco. I think there is value in being able to set all thermostats to the same preset, which is why this PR adds all of them. Some heating systems use two stages, so boost means to enable both stages. For an AC, eco usually means don't run the fan when it's not cooling. For the generic thermostat, boost or eco would just translate to a different temp.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Dec 20, 2021

Not sure what went wrong with the CI, but I went ahead and restarted it

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Dec 20, 2021

running in a preset, you can assume it's on a schedule

I can accept that as a good reason to add them, but I don't think we should add them all since that doesn't justify boost or eco mode.

I am not comfortable approving this PR with them for that reason.

If you feel strongly about it, I can ask another dev for a second opinion.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Dec 20, 2021

The CI is failing because of https://status.docker.com/pages/history/533c6539221ae15e3f000031

I'll restart it later once it clears up

Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Dec 21, 2021

Looks like the docs need a small adjustment and this should be good to go

@bdraco bdraco merged commit 27e3a5b into home-assistant:dev Dec 24, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed has-tests integration: generic_thermostat new-feature smash Indicator this PR is close to finish for merging or closing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants