Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mode control for Modbus climate entities #73906

Merged
merged 28 commits into from
Oct 20, 2022
Merged

Conversation

avishorp
Copy link
Contributor

@avishorp avishorp commented Jun 23, 2022

Proposed change

Add support for more features for a climate entity using Modbus platform.

This PR adds the ability to configure registers to control and monitor a climate
entity's state (heat, cool, on/off, etc.)

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

This PR adds two optional configuration entries to a Modbus climate entity. The first one is
a mode register, which is used to determine the operating mode of an HVAC. An additional
set of configuration items map between the possible states and the values read/written to this
register.

The second register is an On/Off register, which enables turning a climate entity on or off (in cases
where this functionality is not provided by the mode register).

The code was tested against a Samsung air condition system, and from the documentation also seem
to be compatible with systems that are controlled by Intesis interface system which
is available for many AC brands.

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
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@homeassistant
Copy link
Contributor

Hi @avishorp,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@probot-home-assistant
Copy link

Hey there @adamchengtkc, @janiversen, @vzahradnik, mind taking a look at this pull request as it has been labeled with an integration (modbus) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@janiversen
Copy link
Member

The idea quite good I will take a deeper look later.

Copy link
Member

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the suggestions made.

Once you have made the configuration changes, you need to document it.

homeassistant/components/modbus/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/modbus/const.py Show resolved Hide resolved
Copy link
Member

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry hit the wrong button.

@avishorp
Copy link
Contributor Author

Documentation is on the way, will submit a PR soon.

@avishorp avishorp requested a review from janiversen July 5, 2022 03:43
@jiagangxu
Copy link

@avishorp, is it possible to also add fan mode control to modbus climate? I know the modbus component updates from time to time, and there are quite a lot users customize modbus climate to add more registers to suit for their devices. It will save the trouble for end users to update customized components along with modbus update. Thanks for your support!

@avishorp
Copy link
Contributor Author

avishorp commented Aug 29, 2022 via email

@janiversen
Copy link
Member

Waiting for review, and if you look at CI you have a doc problem. I will review it once CI is green.

@avishorp
Copy link
Contributor Author

avishorp commented Aug 29, 2022 via email

@janiversen
Copy link
Member

See the PR text (Proposed change etc.), seems you have modified the template, normally it contains:

## Additional information
<!--
  Details are important, and help maintainers processing your PR.
  Please be sure to fill out additional details, if applicable.
-->

- This PR fixes or closes issue: fixes #
- This PR is related to issue: 
- Link to documentation pull request: 

Btw. you need to link this PR in the documentation PR as well.

@@ -87,6 +89,33 @@ def __init__(
self._attr_target_temperature_step = config[CONF_TARGET_TEMP]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though this part remains the same, there is a question.
line 89 and line 90 have the same lvalue. Maybe line 89 should be self._attr_target_temperature = config[CONF_TARGET_TEMP]?

@avishorp
Copy link
Contributor Author

avishorp commented Sep 13, 2022 via email

Copy link
Member

@janiversen janiversen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Sep 26, 2022
@frenck
Copy link
Member

frenck commented Sep 28, 2022

@avishorp There is a merge conflict, could you take a look?

Thanks! 👍

../Frenck

@frenck frenck removed the smash Indicator this PR is close to finish for merging or closing label Sep 28, 2022
@avishorp
Copy link
Contributor Author

Fixed the merge conflict.

@stillartem
Copy link

@frenck Any updates or concerns? does production see soon this MR ?

@janiversen
Copy link
Member

There is a pylint error prohibiting merge, please fix.

@stillartem
Copy link

@avishorp you did a brilliant job, could you make one more fix to complete all checks, please

@avishorp
Copy link
Contributor Author

Fixed

@janiversen janiversen merged commit ae7eb9c into home-assistant:dev Oct 20, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants