Skip to content

Add possibility to have multiple values for every modbus hvac mode #98829

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 29, 2023
Merged

Add possibility to have multiple values for every modbus hvac mode #98829

merged 3 commits into from
Aug 29, 2023

Conversation

escoand
Copy link
Contributor

@escoand escoand commented Aug 22, 2023 •
edited
Loading

Breaking change

Proposed change

With this change it's also possible to set multiple values for every hvac mode. When setting a mode the first value is used.

My boiler has no special register for just on-off but a register with the current active power mode (0=off, 1=500W, 2=1000W, ...). But this is not possible to handle with the current configuration.

modbus:
  - name: hub1
    climates:
      - name: "Bedroom Air Condition"
        hvac_mode_register:
          address: 11
          values:
            # old way
            state_off: 0
            state_heat: 1
            # new way
            state_heat: [1, 2, 3, 4, 5, 6 ,7]

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

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 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.

To help with the load of incoming pull requests:

@escoand escoand requested a review from janiversen as a code owner August 22, 2023 15:52
@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!

Code owner commands

Code owners of modbus can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign modbus Removes the current integration label and assignees on the pull request, add the integration domain after the command.

@escoand escoand force-pushed the modbus_hvac_modes branch from d3dca59 to bb55e33 Compare August 22, 2023 15:56
@janiversen
Copy link
Member

Before we review it you need to solve the linter errors.

You also have to think about, if your way, also works when setting the hvac_mode, please do not forget this climates inherit from the general climate entity, so all its functionality must be guaranteed.

Without having thought too long about it, another (might be even simpler) solution is to allow multiple integers at each state, and define that the first integer is used to set hvac_mode.

@janiversen
Copy link
Member

Why have you marked the manifest file, as far as I can see you have not touched it (and please do not touch it).

@escoand
Copy link
Contributor Author

escoand commented Aug 22, 2023

Ah you're right, setting is a thing I didn't think about.

To your approach: the change it not that big and the workflow wouldn't differ that mich. For me the question rather is what's the preferred way for the configuration?

@janiversen
Copy link
Member

To put it simply, I am not sure.....of the head it seems a bit more natural and consistent with other parameters to have : register value, but I have NO firm opinion.

@MartinHjelmare MartinHjelmare changed the title Add possibility to reverse map hvac modes Add possibility to reverse map modbus hvac modes Aug 23, 2023
@escoand escoand marked this pull request as draft August 23, 2023 09:49
@escoand escoand force-pushed the modbus_hvac_modes branch from 4e0cd22 to 17595bb Compare August 25, 2023 12:11
@escoand escoand changed the title Add possibility to reverse map modbus hvac modes Add possibility to have multiple values for every modbus hvac mode Aug 25, 2023
@escoand
Copy link
Contributor Author

escoand commented Aug 25, 2023 •
edited
Loading

@janiversen I switched to your idea of setting an array of multiple values. And the code is already able to just set the first mentioned value in the config.

core/homeassistant/components/modbus/climate.py

Lines 172 to 188 in 17595bb

for value, mode in self._hvac_mode_mapping:
if mode == hvac_mode:
if self._hvac_mode_write_type:
await self._hub.async_pymodbus_call(
self._slave,
self._hvac_mode_register,
[value],
CALL_TYPE_WRITE_REGISTERS,
)
else:
await self._hub.async_pymodbus_call(
self._slave,
self._hvac_mode_register,
value,
CALL_TYPE_WRITE_REGISTER,
)
break

@escoand escoand marked this pull request as ready for review August 25, 2023 14:21
@janiversen
Copy link
Member

I will review it later, but please do not forget to submit a pull request with a documentation update.

janiversen
janiversen requested changes Aug 25, 2023
View reviewed changes
Copy link
Member

@janiversen janiversen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

the changes look ok to me.

But we need a documentation pull request also, to tell others how it works.

Be aware that "current" on home assistant.io have just had a major rewrite of the modbus integration. it seems these changes are not available on "next". Let me ask someone who knows. Waiting for an answer.

If you want to start on documentation, then please base your PR on "next" but use modbus.markdown from "current", I will update this information once I know more.

Please do not forget to change the status of this PR, it is currently "draft".

@home-assistant
Copy link

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

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft August 25, 2023 17:47
@janiversen
Copy link
Member

I have just been promised that "next" will be synced with "current", so in a couple of hours I presume you can "git pull" next, and get started....but I will confirm when I see that "next" is updated.

@janiversen
Copy link
Member

"next" is updated, so now is the time, if you want your change to be part of 2023.9

@escoand escoand mentioned this pull request Aug 29, 2023
Merged
8 tasks
@janiversen janiversen marked this pull request as ready for review August 29, 2023 08:14
@home-assistant home-assistant bot requested a review from janiversen August 29, 2023 08:14
janiversen
janiversen approved these changes Aug 29, 2023
View reviewed changes
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, thanks.

@janiversen janiversen merged commit 4632a07 into home-assistant:dev Aug 29, 2023
@escoand escoand deleted the modbus_hvac_modes branch August 29, 2023 09:13
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@janiversen janiversen janiversen approved these changes

Assignees

@janiversen janiversen

Labels
cla-signed integration: modbus new-feature Quality Scale: gold
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@escoand @janiversen