Add separate scale and offset for current temperature for modbus climates#150985
Conversation
home-assistant
Bot
added
bugfix
cla-signed
has-tests
integration: modbus
new-feature
small-pr
janiversen
left a comment
janiversen
left a comment
There was a problem hiding this comment.
You are adding 2 new config parameters, that seems to compete with the existing scale/offset, and I cannot find the relationship ???
You remove _attr_current_temperature, so I wonder how the UI knows about the current temperature.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
The idea is not to compete with the existing scale/offset, but to add the ability to configure current temperature separately from the target temperature. Some Modbus devices expose registers that require different scaling for current vs. target, so the extra options handle that case. I did not remove _attr_current_temperature — it is still there (see lines 490 and 495). The UI continues to use it in the same way, the only change is how the value is calculated when custom scale/offset for current temperature are provided. |
I thought maybe I could use _async_read_register(raw=True) to avoid applying the global scale, and actually enter both CONF_CURRENT and CONF_TARGE. |
|
What do you think about doing it this way? |
janiversen
left a comment
janiversen
left a comment
There was a problem hiding this comment.
I still miss a validation of scale/offset vs current_, it seems I can use both without knowing how it is solved.
I still miss the correction of the bug you mention in the text !
Please make sure you solve all comments before requesting a review, not doing so is wasting my time.
|
The PR text goes out to all users, so please be specific what is not working in the code ? (if the documentation is incorrect you should open a PR in the documentation repo). When there is a bug in the code, it is important to inform users the specifics, so the6 can determine how it might have affected them. |
janiversen
left a comment
janiversen
left a comment
There was a problem hiding this comment.
Please solve the comments as well as the mypy problem.
I could not find the validation, that temp_offset is not used simultaneously with offset ?
I also cannot find the code you made to justify the comment in the PR text:
""Now scale and offset is applied to both target and current temperature at the same time. This is an error in the documentation:
https://www.home-assistant.io/integrations/modbus/#scale""
Please give me a pointer to where you solved this, without using the new parameters, if it is a bug is should be solved !
|
You ask for a review while you still have mypy problems, and while the PR was in draft mode, that means even if there were no review comments I could not approve it, so in essence, waste of time. Please do not ask me to do more reviews, unless you are absolutely sure everything is solved and ok ! |
|
This pull request is relevant because I personally have a heat pump where the current room temperature (address: 0x0000) is reported 40 units higher than the real value — according to both the documentation and actual behavior — so I need to apply an offset of -40. However, the target temperature (target_temp_register: 0x0034) works correctly without any offset, meaning I send the exact value. Currently, that means I can either set the temperature to, for example, 25 °C, but the reported room temperature shows something like 62 °C — or, if I apply the offset, the room temperature reads correctly as 22 °C, but then the target temperature becomes -15 °C. |
|
Hi @crug80 , @janiversen ,
Any chance this will get merged soon? |
|
I am not working on the project, so I cannot tell you, you probably have to find a modbus developer willing work in the conditions given. |
OK, are you willing to make a summary of the issues with this PR so they can be fixed? |
|
This PR is almost ready and it could be merged after its evaluation by someone has the possibility to merge it, IMHO. |
Just look at the PR comments. |
| if target_key not in config or config[target_key] == 0: | ||
| config[target_key] = default_value | ||
| if current_key not in config or config[current_key] == 0: | ||
| config[current_key] = default_value |
There was a problem hiding this comment.
This treats 0 as a missing value, which is incorrect. Somone might legitimately want offset: 0 or even scale: 0 (though the latter would cause division by zero). If a one explicitly sets current_temp_offset: 0, it gets overridden to the default.
| if target_key not in config or config[target_key] == 0: | |
| config[target_key] = default_value | |
| if current_key not in config or config[current_key] == 0: | |
| config[current_key] = default_value | |
| if target_key not in config: | |
| config[target_key] = default_value | |
| if current_key not in config: | |
| config[current_key] = default_value |
Tests for these cases are missing as well, not showing the issue.
There was a problem hiding this comment.
Thanks @frenck for your review. This implementation descends from a previous one where errors due to 0 were not managed (basically the same code of your suggestion). Actually it wants to prevent the division by 0 for scale and it applies also to offset because its default value is 0. This means that we cover the case where 0 is wanted for the offset and we prevent errors in case 0 is set for scale. The tests for those cases are inside the test_climate (i.e. current_temp_scale).
There was a problem hiding this comment.
@crug80 I'm confused, you replied to Frenck's comment, but you also approved the PR? Or was your point that implementing Frenck's suggestion would bring back old broken behavior?
An offset of 0 seems reasonable, why can't we support it? Shouldn't a scale of 0 cause a validation error instead of a silent replacement with the default value?
There was a problem hiding this comment.
@emontnemery thanks for your review.. I approved the PR mainly for 2 reasons: I think that frenk's suggestion shouldn't be applied and as a way to get his attention. (but I don't know if it is the correct way to do that, so excuse me in case I was wrong).
Anyway, as I said, the old code won't have those double checks, but correctly Jan suggested to insert them and I agreed.
The default value for offset is 0, so it is supported. (0 means don't apply the offset).
With regards to the scale, I don't know what is the best, I have simply implemented what in my opinion will make the user life easier: silent replacement to avoid unuseful wasting time to fix an error that we can easily fix under the wood without affecting the user's intent.
There was a problem hiding this comment.
OK. I'd suggest to remove the or config[current_key] == 0 (as Frenck suggested) because it's confusing when reading the code and prone to break (for example if the default changes in the future), and instead add an additional validator to the scales:
def not_zero(val: float) -> float:
if val == 0:
raise vol.Invalid("blablabla")- vol.Optional(CONF_SCALE): vol.Coerce(float),
+ vol.Optional(CONF_SCALE): vol.All(vol.Coerce(float), not_zero))Or is there some reason to allow the user to set the scale to 0?
There was a problem hiding this comment.
There is no reason to have 0 as scale, but instead you'll have an error in case you set due to division by 0. I'll change the code as per your suggestion. Thanks Erik
There was a problem hiding this comment.
@emontnemery and @frenck I committed the changes you request. Error is not related to this PR
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
|
Hi @illia-piskurov, I hope you’re doing well. I wanted to kindly ask if you plan to continue working on this PR. This PR is quite important for several users, and I’ve been following questions around this topic for several years now. I completely understand if you’re busy, but any update on your plans regarding this PR would be greatly appreciated. Thank you for your time and effort! |
emontnemery
left a comment
emontnemery
left a comment
There was a problem hiding this comment.
LGTM, thanks @illia-piskurov and @crug80 👍
7 participants
Breaking change
Proposed change
The documentation previously stated that scale and offset only applied to the target temperature.
This was incorrect: in reality they apply to both target and current temperature.
This PR adds the ability to configure separate scale and offset for current and target temperatures.
There are devices that require different scaling for target and current temperatures (for example, Cooper air conditioners). People have also reported this limitation on the forum:
https://community.home-assistant.io/t/wth-why-do-modbus-climate-entities-only-have-one-scale-setting-for-temperature-registers/803414
https://community.home-assistant.io/t/modbus-plataform-climate/323571
This was also discussed in PR: #135848.
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.To help with the load of incoming pull requests: