Add minimum setpoint deadband to Climate entity #118186
Conversation
home-assistant
bot
added
cla-signed
core
has-tests
integration: climate
new-feature
small-pr
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
This comment has been minimized.
This comment has been minimized.
gjohansson-ST
force-pushed
the
climate-deadband
branch
from
a4481f1 to
e194aa5
Compare
| # Guard target_low_temp isn't lower than entity low_temp | ||
| if target_low_temp < min_temp: | ||
| raise ServiceValidationError( |
There was a problem hiding this comment.
Do we want to raise if this happens or should we modify the high temp instead to ensure we're within tolerance?
In theory neither low or high could fit but we could guard for that and raise in that case.
There was a problem hiding this comment.
I think the behavior should be symmetric; if we try to "fix" the service call by adjusting the passed in low_temp, we should try to adjust the high_temp too.
If targetting multiple termostats is a valid use case, it might make sense to allow the user to pass in a narrow deadband, and then adjust it in the service call for thermostats which support it?
Is there a frontend design, or even better a draft PR, which takes the min_temp_range into account?
There was a problem hiding this comment.
Made it symmetric now in the sense that it first moves the low_temp and second the high_temp if needed.
It does not check if the deadband makes the equation impossible e.g. in theory an endless loop to get inside min/max temp and still ensure the deadband. Is such check necessary and should raise?
There was a problem hiding this comment.
If it's targeting multiple entities it would seem strange if the user is allowed to set a deadband as the integrations should be in control of that?
| # Ensure target_low_temp is not higher than target_high_temp. | ||
| if target_low_temp > target_high_temp: | ||
| raise ServiceValidationError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="low_temp_higher_than_high_temp", | ||
| ) |
There was a problem hiding this comment.
It's somewhat weird this is only checked if we have a min temperature range. Shouldn't this always be checked if we don't allow it? If base components don't check it, do integration check and handle it?
Edit: Based on a quick search for ATTR_TARGET_TEMP_LOW, it seems integrations do NOT check this.
There was a problem hiding this comment.
Prel PR: #124488
Then we should move this out of this PR 👍
| # Guard target_low_temp isn't lower than entity low_temp | ||
| if target_low_temp < min_temp: | ||
| raise ServiceValidationError( |
There was a problem hiding this comment.
I think the behavior should be symmetric; if we try to "fix" the service call by adjusting the passed in low_temp, we should try to adjust the high_temp too.
If targetting multiple termostats is a valid use case, it might make sense to allow the user to pass in a narrow deadband, and then adjust it in the service call for thermostats which support it?
Is there a frontend design, or even better a draft PR, which takes the min_temp_range into account?
This comment was marked as resolved.
This comment was marked as resolved.
gjohansson-ST
force-pushed
the
climate-deadband
branch
from
3d3ab3b to
5b5f0b5
Compare
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
Proposed change
Add minimum setpoint deadband according to arch discussion in home-assistant/architecture#996
Should rebase on top of #123941
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: