Add support for specifying on/off value for hvac_onoff_register

[illia-piskurov]

Breaking change

Proposed change

I propose to add the possibility to specify the value that will be written to turn on or off the climate device using hvac_onoff_register in the Modbus integration. Because in my work I have encountered a device that uses 0x55 and 0xAA instead of 0 and 1 which is strictly specified in the integration code. I want to solve my problem and let other users configure it.

Type of change

• [ ] Dependency upgrade
• [ ] Bugfix (non-breaking change which fixes an issue)
• [ ] New integration (thank you!)
• [x] 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 #
• This PR is related to issue: https://community.home-assistant.io/t/modbus-add-the-ability-to-specify-command-on-command-off-for-hvac-onoff-register-in-the-climates-configuration-instead-of-using-only-1-or-0/769641
• Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/35245

Checklist

• [x] The code change is tested and works locally.
• [x] Local tests pass. Your PR cannot be merged unless tests pass
• [x] There is no commented out code in this PR.
• [x] I have followed the development checklist
• [x] I have followed the perfect PR recommendations
• [x] The code has been formatted using Ruff (ruff format homeassistant tests)
• [x] Tests have been added to verify that the new code works.

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

• [x] 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.

[home-assistant[bot]]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

[illia-piskurov]

Quick preliminary review:

• Documentation not updated and linked to this PR. You are adding a new feature that should be documented
• async_update of the climate enitity should be also changed to apply this feature
• You miss completely to write the tests. You have to prove that the code you are submitting works and to do so you have to write the tests. Code coverage of the components should be mantained at least at the actual level (99% o more).

Do I understand correctly that the documentation should be a separate PR to home-assistant.io?

[crug80]

Yes, in this case you have to fork the dev branch, then apply the changes, submit a PR that it should be referenced here, under the "additional documentation". You miss also to complete the Checklist for this PR (ruff, tests and perfect pr).

[illia-piskurov]

Yes, in this case you have to fork the dev branch, then apply the changes, submit a PR that it should be referenced here, under the "additional documentation". You miss also to complete the Checklist for this PR (ruff, tests and perfect pr).

Documention PR: https://github.com/home-assistant/home-assistant.io/pull/35245

[illia-piskurov]

Please, remove the 3 flags above (manifest, requirements..). They are not related to such PR. Please check, instead, the documentation point.

Done

[illia-piskurov]

Please make sure ruff passes locally

I have confirmed that Ruff passes locally without any issues.

[joostlek]

It does not in the CI, so something is going wrong

[illia-piskurov]

It does not in the CI, so something is going wrong

[joostlek]

Ruff is still failing

[illia-piskurov]

Can someone help me with the tests?

[crug80]

Can someone help me with the tests?

You cannot request a review if all the changes have not been satisfied and if you cannot run test, it means that the PR is not ready for a review. Anyway, in the check list you flagged that the code change is tested and works locally. To have it working locally, I suppose you needed to install the requirements using pip. The same you have to do for the tests. You have to install the test requirements.

If you use Dev Container as development env, you can type pip install -r requirements_test_all.txt into the terminal.

[illia-piskurov]

I moved the initialization of on/off_values to the right place, but I want to make sure that the job that is launched will pass the tests or not. I assume it only runs when you click “Ready to review”?

[crug80]

I moved the initialization of on/off_values to the right place, but I want to make sure that the job that is launched will pass the tests or not. I assume it only runs when you click “Ready to review”?

did you run the tests in your dev env? pytest ./tests/components/modbus/* --cov=homeassistant.components.modbus --cov-report term-missing -vv

[illia-piskurov]

I moved the initialization of on/off_values to the right place, but I want to make sure that the job that is launched will pass the tests or not. I assume it only runs when you click “Ready to review”?

did you run the tests in your dev env? pytest ./tests/components/modbus/* --cov=homeassistant.components.modbus --cov-report term-missing -vv

For some reason, tests that aren't related to my change are crashing there. They crash even if I remove my changes and run the code from the core repository

[crug80]

I moved the initialization of on/off_values to the right place, but I want to make sure that the job that is launched will pass the tests or not. I assume it only runs when you click “Ready to review”?

did you run the tests in your dev env? pytest ./tests/components/modbus/* --cov=homeassistant.components.modbus --cov-report term-missing -vv

For some reason, tests that aren't related to my change are crashing there. They crash even if I remove my changes and run the code from the core repository

Did you install the requirements_test_all ?

[illia-piskurov]

I moved the initialization of on/off_values to the right place, but I want to make sure that the job that is launched will pass the tests or not. I assume it only runs when you click “Ready to review”?

did you run the tests in your dev env? pytest ./tests/components/modbus/* --cov=homeassistant.components.modbus --cov-report term-missing -vv

For some reason, tests that aren't related to my change are crashing there. They crash even if I remove my changes and run the code from the core repository

Did you install the requirements_test_all ?

Yes, but test_restore_state_climate is failed

[crug80]

If I run the tests, they pass, so I suppose that your failing tests are failing for some reason internal to your dev env. There are still some problems on this PR:

• you removed the test related to this new feature and if you add something new, you have to add specific tests to validate it
• you import CONF_HVAC_OFF_VALUE, CONF_HVAC_ON_VALUE, in test_climate, but you never use them, because you removed the test.

[illia-piskurov]

If I run the tests, they pass, so I suppose that your failing tests are failing for some reason internal to your dev env. There are still some problems on this PR:

• you removed the test related to this new feature and if you add something new, you have to add specific tests to validate it
• you import CONF_HVAC_OFF_VALUE, CONF_HVAC_ON_VALUE, in test_climate, but you never use them, because you removed the test.

This is good news for me that the fix helped and the old tests don't break now. I will try to make a test for the new functionality now, thank you

[illia-piskurov]

added a test

[illia-piskurov]

I have already performed the pre-commit validation locally, and my test passed without any issues.

[illia-piskurov]

I don't see a “re-request review” button for joostlek and frenk.

[crug80]

I don't see a “re-request review” button for joostlek and frenk.

They know, don't worry

[illia-piskurov]

Why is the merge not happening?

[illia-piskurov]

When to expect the merge?

[illia-piskurov]

@joostlek

[abmantis]

@illia-piskurov Please don't ping maintainers or ask for timelines. Even if your intention is good, it just adds extra notifications and overhead to maintainers, and may even sound rude to some. Most of the times, it ends up having the opposite effect of the desired one.

If you want to help speed up reviews, please do help and review some of the other PRs. Also, try to keep your PR up to date with dev.

Thanks!

[abmantis]

Thanks @illia-piskurov !