Skip to content

Add zwave_js fan preset modes and enable them for Inovelli LZW36#60947

Merged
MartinHjelmare merged 13 commits intohome-assistant:devfrom
mkowalchuk:inofans
Mar 19, 2022
Merged

Add zwave_js fan preset modes and enable them for Inovelli LZW36#60947
MartinHjelmare merged 13 commits intohome-assistant:devfrom
mkowalchuk:inofans

Conversation

@mkowalchuk
Copy link
Copy Markdown
Contributor

@mkowalchuk mkowalchuk commented Dec 3, 2021

Breaking change

This will change which fan speed is selected at certain percentages for Inovelli LZW36 fan controllers. Setting the percentage to 1% will no longer enable the 'breeze' mode; that functionality is instead accessed through a preset.

Proposed change

I've gone into a bit of a rabbit hole of trying to support a bunch of different fan controllers, thinking that after PR #60517 was in, any additional controllers would be trivial. So I e-mailed a bunch of manufacturers of Z-Wave fan controllers to get specs on their controllers, and Inovelli's support team eventually got back to me with the following data for their LZW36:

0- Off
1 - Breeze mode (this will cycle through low, medium, high settings to simulate a "breeze")
2-33 - Low
34-66 - Medium
67-99 - High

Oof. I didn't think about preset modes when I did PR #59697, so supporting this fan controller requires some rework.

So this PR does the following:

  1. Extends the fan discovery data structures to support preset mode definitions alongside speed range mapping definitions, and extend the zwave_js fan component to use the them. I renamed some of these from 'speed' definitions to 'value mapping' definitions because preset modes aren't exactly 'speeds' (since, for instance, 'breeze' actually cycles through different speeds).
  2. Adds preset and speed data for the LZW36

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

I don't have an Inovelli LZW36, so I haven't been able to test this on the target hardware yet. I did test this with an actual fan controller by locally replacing my HS200+'s device discovery data with the LZW36's, but it's not a perfect test, since the ranges are different. If anyone with an LZW36 can help test this PR, please give it a shot and write a comment in this PR!

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:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @home-assistant/z-wave, mind taking a look at this pull request as it has been labeled with an integration (zwave_js) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@github-actions
Copy link
Copy Markdown

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Mar 17, 2022
@mkowalchuk
Copy link
Copy Markdown
Contributor Author

Bump. Just need someone to help test.

@github-actions github-actions bot removed the stale label Mar 17, 2022
@MartinHjelmare
Copy link
Copy Markdown
Member

@firstof9 Do you have an LW36 to test with?

@firstof9
Copy link
Copy Markdown
Contributor

firstof9 commented Mar 17, 2022

@MartinHjelmare I do.
I'll see if I can test.

@firstof9
Copy link
Copy Markdown
Contributor

It appears to function as intended.
I am presented with a single preset of "breeze".

@MartinHjelmare
Copy link
Copy Markdown
Member

Great! Thanks!

@mkowalchuk please rebase on the latest dev branch and solve the merge conflict.

@mkowalchuk
Copy link
Copy Markdown
Contributor Author

Thanks. Working on merging and retesting now.

@firstof9
Copy link
Copy Markdown
Contributor

Looks like you have some formatting issues, make sure to run black.

@mkowalchuk mkowalchuk marked this pull request as ready for review March 18, 2022 00:51
@mkowalchuk mkowalchuk requested a review from a team as a code owner March 18, 2022 00:51
@mkowalchuk
Copy link
Copy Markdown
Contributor Author

Sorry for the large number of commits for the merge. My dev environment was in disarray, so I was merging in GitHub.

I tested locally (albeit with my hardware) and things seem good.

Copy link
Copy Markdown
Contributor

@raman325 raman325 left a comment

Choose a reason for hiding this comment

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

no worries about the commits, we squash them anyway. You don't want to see how many commits some of my PRs have 😅

Sorry for the late review on this but I hadn't actually looked this over before. Let me know if you have any questions, happy to help you get this finished out so we can merge soon 🎉

@MartinHjelmare MartinHjelmare changed the title Add support for preset modes in the zwave_js fan component, and enable them for the Inovelli LZW36 Add zwave_js fan preset modes and enable them for Inovelli LZW36 Mar 19, 2022
@MartinHjelmare MartinHjelmare merged commit e0b577f into home-assistant:dev Mar 19, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Mar 20, 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.

5 participants