Allow modbus min/max temperature to be negative. #109977
Conversation
home-assistant
bot
added
bugfix
cla-signed
has-tests
integration: modbus
small-pr
| CONF_MIN_TEMP: -23, | ||
| CONF_MAX_TEMP: -57, |
There was a problem hiding this comment.
Having the max temp lower than the min temp doesn't make sense, maybe a check for that would be good?
There was a problem hiding this comment.
Not true, it would be fairly normal for a freezer.
it really depends on your interpretation of max/min e.g, the freezer can max freeze to x or a aircon can max cool down to x.
We do not want to make checks that impose not needed limits on configurations
There was a problem hiding this comment.
Just to be sure I was not being theoretical, I checked my refrigerator, it says "max: -19", and "min: -5", lthe current temp is "-7", so max lower than min is making sense for that one.
There was a problem hiding this comment.
Okay, then I haven't said anything 😅 Thanks for checking!
There was a problem hiding this comment.
@janiversen The convention is that the negative number closest to 0 is the maximum number.
I've known this since lower school, a long time ago :)
And internet "says' the same, google it.
Don't rely on a single wrong occurrence please :)
There was a problem hiding this comment.
The thing is unfortunately that freezer logic is not neccessarily real math logic. The question is if we want to be strict and say that max > min should always be true.
There was a problem hiding this comment.
@autinerd good point, calculation-rules show that -23 is the max value.
Freezer logic does not exist, that's just a wrong construct by someone that doesn't know the normal conventions :)
There was a problem hiding this comment.
This is a nice theoretical discussion, but this PR allows min/max temperatures to be negative, not more not less.
Adding a check on min/max values (of which there are 7-8 different in modbus) is another PR...and I for one believe if a user wants to configure max lower than min, then he sure have reasons, and does it make HA less robust to allow that...
There was a problem hiding this comment.
But in order to avoid a heated discussion, I have changed the test (it really does not matter to the test), so that it now reads:
CONF_MIN_TEMP: 23,
CONF_MAX_TEMP: 57,
in the hope we can move on, and actually get the bug fixed, so the user who filed the issue can continue using his configuration.
janiversen
force-pushed
the
solve_109958
branch
from
71999c6 to
bd01962
Compare
janiversen
force-pushed
the
solve_109958
branch
from
bd01962 to
a911b4b
Compare
|
Thanks Guys really appreciate your fast and very good work!! |
Breaking change
Proposed change
Min/Max temperature in a household climate control are positive, but some configurations use HA to control a freezer or similar, so they need to set a negative min/max.
Test case added to avoid further regressions.
Type of change
Additional information
fixes Modbus climate is no longer accepting negative values. #109958
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: