Add separate scale for current temp for modbus climates

[illia-piskurov]

Proposed change

Now scale 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

Also, there are indeed devices that scale these temperatures differently, such as the Cooper air conditioners I work with.

Also, people on the forum have already repeatedly reported the need for this:
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

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)
• 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 https://community.home-assistant.io/t/wth-why-do-modbus-climate-entities-only-have-one-scale-setting-for-temperature-registers/803414
• This PR is related to issue: https://community.home-assistant.io/t/modbus-plataform-climate/323571
• Link to documentation pull request:

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
• I have followed the perfect PR recommendations
• The code has been formatted using Ruff (ruff format homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

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:

• I have reviewed two other open pull requests in this repository.

[simonmarwitz]

I do not have much experience with the process of merging PRs and I am probably not entitled to review changes. So apologies if that causes trouble.
However, I am rather experienced with python and can easily review these small changes. I have a personal interest in getting this PR merged, so I am giving it a try to approve it.

[illia-piskurov]

I do not have much experience with the process of merging PRs and I am probably not entitled to review changes. So apologies if that causes trouble. However, I am rather experienced with python and can easily review these small changes. I have a personal interest in getting this PR merged, so I am giving it a try to approve it.

If you understand Python well, it would be nice if you helped write tests for this.

[simonmarwitz]

I have had a quick look at the existing tests and I have to admit that I need to get into the usage of pytest first. I assume, I would have to set up a development version of home assistant on my local machine to actually develop and test code. Although this would probably come handy in the future to actually contribute in other places as well, I cannot spare the time besides my daytime job having a family. Or is there any other easier way, you know of, maybe some kind of cloud-based pre set up environment?

[illia-piskurov]

I have had a quick look at the existing tests and I have to admit that I need to get into the usage of pytest first. I assume, I would have to set up a development version of home assistant on my local machine to actually develop and test code. Although this would probably come handy in the future to actually contribute in other places as well, I cannot spare the time besides my daytime job having a family. Or is there any other easier way, you know of, maybe some kind of cloud-based pre set up environment?

The repository already has dev-container configured, it can be easily used from the VS Code and most likely even in PyCharm

[simonmarwitz]

Thank you, that was the hint i needed. I will sure give it a try whenever I find some time.

[illia-piskurov]

Added some test

[simonmarwitz]

Tests run and pass locally and seem to cover all cases

[simonmarwitz]

I am mistaken, but wouldn't it have to be
int((register_value - self._offset ) / self._scale * self._current_temperature_scale)
?
This approach seems a bit improvised though. Another option would be to temporarily change self._scale and self._offset prior to _async_read_register and revert afterwards. I am not sure if this is considered good coding practice after all.

[illia-piskurov]

I am mistaken, but wouldn't it have to be int((register_value - self._offset ) / self._scale * self._current_temperature_scale) ? This approach seems a bit improvised though. Another option would be to temporarily change self._scale and self._offset prior to _async_read_register and revert afterwards. I am not sure if this is consider good coding practice after all.

Yeah, I think you're right. As for the method, I'm willing to do whatever is accepted and considered good practice.

[simonmarwitz]

A quick search turned out, that overriding variables should be OK from a coding pratice point of view. I could not find any further guidelines related to the home-assistant coding style. Maybe some experienced developer could comment here? Personally, I this option cleaner. I suspect it should be executed in order even with asyncio (await).

[crug80]

Actually current and target temp are 2 different sensors which share 6 different parameters at least: scale, precision, limits (min-max) and offset. In my opinion, the choice is between an ugly change limited to solve the only problem behind this PR (to have a different scale) or to rewrite a little bit the component to make current and target temperature 2 distinct sensors. I asked for an help and probably we can talk also on discord, under the modbus thread.

[illia-piskurov]

I am mistaken, but wouldn't it have to be int((register_value - self._offset ) / self._scale * self._current_temperature_scale) ? This approach seems a bit improvised though. Another option would be to temporarily change self._scale and self._offset prior to _async_read_register and revert afterwards. I am not sure if this is considered good coding practice after all.

Thanks

[simonmarwitz]

Are there any updates on this PR? Can I do anything to help getting it merged?

[bingo6]

I have just come across this request. Would it also be possible to implement an extra option for current offset? In my heat pump, scale is the same for current and target, but current has an offset of 100, target does not.

[simonmarwitz]

As @crug80 pointed out in #135848 (comment) a rewrite of the component would be needed to make current and target temp two distinct sensors. This seems beyond the scope of this PR. I, too, have some further ideas for improvement, just did not find the time to implement them, yet. Your are welcome to contribute.

[crug80]

It seems that we just found a real use case where to have really 2 distinct sensors is required. I beginnning to think that probably we should migrate to that solution instead of to limit the implementation of this patch.