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.

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

Support HitachiAirToAirHeatPump (ovp:HLinkMainController) in Overkiz integration #102159

Merged
merged 9 commits into from
Feb 29, 2024
Merged

Support HitachiAirToAirHeatPump (ovp:HLinkMainController) in Overkiz integration #102159

merged 9 commits into from
Feb 29, 2024

Conversation

dotvav
Copy link
Contributor

@dotvav dotvav commented Oct 17, 2023 •
edited
Loading

Proposed change

This adds support to Hitachi Air-To-Air Heat Pumps that use the OVP protocol on the Overkiz API. It follows the PR #95706 which adds support for the devices on the HLRRWIFI protocol. OVP protocol has enough differences with HLRRWIFI to be added as a separate class in order to make the code easier to maintain.

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:

@dotvav dotvav requested a review from iMicknl as a code owner October 17, 2023 12:46
@home-assistant
Copy link

Hey there @iMicknl, @vlebourl, @tetienne, @nyroDev, mind taking a look at this pull request as it has been labeled with an integration (overkiz) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of overkiz 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 overkiz Removes the current integration label and assignees on the pull request, add the integration domain after the command.

@dotvav dotvav marked this pull request as draft October 17, 2023 12:47
@dotvav dotvav mentioned this pull request Nov 1, 2023
Closed
@dotvav dotvav force-pushed the overkiz-hitachi-air-heat-pump-ovp branch 3 times, most recently from 434ca8e to 9f3bb04 Compare November 25, 2023 17:02
@dotvav dotvav closed this Nov 25, 2023
@dotvav dotvav force-pushed the overkiz-hitachi-air-heat-pump-ovp branch from 9f3bb04 to 48f8cec Compare November 25, 2023 17:05
@dotvav dotvav reopened this Nov 25, 2023
@dotvav dotvav force-pushed the overkiz-hitachi-air-heat-pump-ovp branch from 2092508 to a3578f7 Compare November 25, 2023 18:59
@dotvav dotvav marked this pull request as ready for review November 25, 2023 18:59
@dotvav dotvav force-pushed the overkiz-hitachi-air-heat-pump-ovp branch from a3578f7 to 241dddd Compare January 2, 2024 10:48
@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 February 16, 2024 11:58
@emontnemery
Copy link
Contributor

The PR looks good to me, except for the code style issues.
I think one of the code owners should review before merge though, @iMicknl, @vlebourl, @tetienne, @nyroDev, could you have a look?

@dotvav
Copy link
Contributor Author

dotvav commented Feb 17, 2024

Thanks for your comments @emontnemery I believe I have addressed them all.

@dotvav dotvav marked this pull request as ready for review February 17, 2024 07:13
@home-assistant home-assistant bot requested a review from emontnemery February 17, 2024 07:13
iMicknl
iMicknl requested changes Feb 18, 2024
View reviewed changes
Copy link
Contributor

@iMicknl iMicknl left a comment

Choose a reason for hiding this comment

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

Looks good! Just one minor remark.

homeassistant/components/overkiz/climate_entities/hitachi_air_to_air_heat_pump_ovp.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft February 18, 2024 10:32
@dotvav dotvav requested a review from iMicknl February 18, 2024 11:53
@dotvav dotvav marked this pull request as ready for review February 18, 2024 18:26
iMicknl
iMicknl approved these changes Feb 18, 2024
View reviewed changes
Copy link
Contributor

@iMicknl iMicknl left a comment

Choose a reason for hiding this comment

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

LGTM! Has this been tested by someone that owns the device?

@dotvav
Copy link
Contributor Author

dotvav commented Feb 18, 2024

It was tested by a user a couple months ago. Only changes since are these style fixes and constants cleanup. I can ask them to test again.

@dotvav
Copy link
Contributor Author

dotvav commented Feb 18, 2024

cc @Chris-NL

@iMicknl iMicknl dismissed emontnemery’s stale review February 25, 2024 22:12

Feedback has been implemented.

@iMicknl iMicknl force-pushed the overkiz-hitachi-air-heat-pump-ovp branch from 8fc9325 to b6d94d4 Compare February 28, 2024 22:12
@iMicknl iMicknl added this to the 2024.3.0 milestone Feb 28, 2024
balloob
balloob reviewed Feb 29, 2024
View reviewed changes
homeassistant/components/overkiz/climate_entities/hitachi_air_to_air_heat_pump_ovp.py
self._attr_supported_features |= ClimateEntityFeature.SWING_MODE

if self._attr_device_info:
self._attr_device_info["manufacturer"] = "Hitachi"
Copy link
Member

Choose a reason for hiding this comment

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

this is not good as it overrides the existing dictionary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if I fully understand. We set a default value in the entity.py, where we first check if the device has a specific vendor name, and otherwise fallback to the hub vendor (e.g. Atlantic or Somfy). For this device they don't provide the vendor name in the device information, and falling back to the hub vendor name doesn't make sense. Hence we overwrite the current value with 'Hitachi', which is the vendor of this specific device.

This pattern is used more broadly in Overkiz, so I will make more changes depending on the outcome :).

Copy link
Member

Choose a reason for hiding this comment

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

Ah jus checked the other code, looks like it's fine. I thought it was overriding a key in a global dictionary but it's generated in the constructor.

@balloob balloob merged commit af625a6 into home-assistant:dev Feb 29, 2024
23 checks passed
balloob pushed a commit that referenced this pull request Mar 1, 2024
@dotvav @iMicknl
@joostlek @balloob
Support HitachiAirToAirHeatPump (ovp:HLinkMainController) in Overkiz …
b6503f5 
…integration (#102159)

* Support OVP devices

Support OVP devices

* Fix coding style

* Fix coding style and unnecessary constants

* Move fanmodes inside class

* Apply suggestions from code review

Co-authored-by: Joost Lekkerkerker <[email protected]>

* Remove duplicate widget

* Update homeassistant/components/overkiz/climate_entities/hitachi_air_to_air_heat_pump_ovp.py

Co-authored-by: Joost Lekkerkerker <[email protected]>

* Format ruff

* Fix mypy

---------

Co-authored-by: Mick Vleeshouwer <[email protected]>
Co-authored-by: Joost Lekkerkerker <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Mar 2, 2024
@dotvav dotvav deleted the overkiz-hitachi-air-heat-pump-ovp branch March 30, 2024 16:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joostlek joostlek joostlek left review comments

@balloob balloob balloob left review comments

@iMicknl iMicknl iMicknl approved these changes

@emontnemery emontnemery Awaiting requested review from emontnemery

Assignees

@iMicknl iMicknl

Labels
cherry-picked cla-signed integration: overkiz new-feature Quality Scale: No score
Projects
None yet
Milestone
2024.3.0
Development

Successfully merging this pull request may close these issues.

Support HitachiAirToAirHeatPump (hlrrwifi:HLinkMainController) in Overkiz integration
5 participants
@dotvav @emontnemery @iMicknl @balloob @joostlek