Skip to content

Add zwave_js speed configuration for Leviton ZW4SF fans#60677

Merged
MartinHjelmare merged 9 commits intohome-assistant:devfrom
mkowalchuk:levfans
Sep 9, 2022
Merged

Add zwave_js speed configuration for Leviton ZW4SF fans#60677
MartinHjelmare merged 9 commits intohome-assistant:devfrom
mkowalchuk:levfans

Conversation

@mkowalchuk
Copy link
Copy Markdown
Contributor

@mkowalchuk mkowalchuk commented Dec 1, 2021

Breaking change

The fan speed levels of Leviton ZW4SF fan controllers are now mapped correctly to corresponding percentage levels in Home Assistant. This may change which fan speed is selected at certain percentages. Users should adjust any automations and scripts that target these devices as needed.

Proposed change

Add speed information for Leviton ZW4SF fan controllers. I got the speed information used here from Leviton's tech support.

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 fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.

I verified that HA starts and works and tests pass, but I haven't verified this on an actual Leviton ZW4SF.

  • 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:

@mkowalchuk mkowalchuk requested a review from a team as a code owner December 1, 2021 04:39
@probot-home-assistant probot-home-assistant bot added integration: zwave_js small-pr PRs with less than 30 lines. labels Dec 1, 2021
@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)

@mkowalchuk mkowalchuk changed the title Add zwave_js speed configurations for Leviton ZW4SF fans Add zwave_js speed configuration for Leviton ZW4SF fans Dec 1, 2021
@MartinHjelmare
Copy link
Copy Markdown
Member

Please try to find a user that can test this.

@drewancil
Copy link
Copy Markdown

This is my first time posting on this project, but I have two of these controllers and can test if that helps.

@MartinHjelmare
Copy link
Copy Markdown
Member

That would be great, thanks!

@mkowalchuk mkowalchuk marked this pull request as draft March 20, 2022 02:22
@mkowalchuk
Copy link
Copy Markdown
Contributor Author

Switched back to draft; going to update this PR to use the new format from #60947.

@drewancil
Copy link
Copy Markdown

Great. After that is done and if it would be helpful for testing, please let me know.

Side note: I came to HA 6 months ago and have gone “all-in” in my home. I’ve read the architecture documents and read the GitHub PRs and am on various discord channels. Is there a document or other resources that show how to load and test code like this in an otherwise production environment? I’ll do the homework but haven’t found this information yet (although this is my first time asking).

@raman325
Copy link
Copy Markdown
Contributor

Great. After that is done and if it would be helpful for testing, please let me know.

Side note: I came to HA 6 months ago and have gone “all-in” in my home. I’ve read the architecture documents and read the GitHub PRs and am on various discord channels. Is there a document or other resources that show how to load and test code like this in an otherwise production environment? I’ll do the homework but haven’t found this information yet (although this is my first time asking).

Coincidentally, I have a PR open to add this info to the dev docs: https://github.com/home-assistant/developers.home-assistant/pull/1241/files#diff-3cd9dd50e143937424e6fd9cce939e07cfa3c5780103cbc8a5664d0205dd3751R25

@mkowalchuk mkowalchuk marked this pull request as ready for review March 21, 2022 21:37
@michaelkasper
Copy link
Copy Markdown

Hope all is well, i just installed a couple of these switches and was wondering if there was any timeline on this PR being closed out. Thanks

@MartinHjelmare
Copy link
Copy Markdown
Member

If you can help test this PR we can move it forward.

@michaelkasper
Copy link
Copy Markdown

Happy too, im a developer, but dont have any experience with hass dev env and how to test code with my system. Any docs on how to go about it/config my env?

@raman325
Copy link
Copy Markdown
Contributor

https://developers.home-assistant.io/docs/development_environment for setting up your environment. You just have to clone this locally where you are setting up your environment and follow the instructions to start HA locally. Your zwave-js-server instance will need to be exposed to the network if it's running on another system. If you are using the addon, you can expose the port in the configuration section of the addon

@mkowalchuk
Copy link
Copy Markdown
Contributor Author

I now have a Leviton ZW4SF in my possession, so I'll be able to test this soon myself.

@mkowalchuk
Copy link
Copy Markdown
Contributor Author

Everything seems to work well from my testing.

Here's the sequence I ran through:

  • Stepped through all speed settings (off, 1, 2, 3, 4) on the device and confirmed that they propagated correctly to HA (off, 25%, 50%, 75%, 100%)
  • Stepped through all speed settings on HA and confirmed that they propagated correctly to the device.

I also experimented a bit with the 'Minimum Dim Level' and 'Maximum Dim Level settings', but don't appear to have any impact on Z-Wave commands - they only affect what happens when you hit the buttons on the device.

@raman325
Copy link
Copy Markdown
Contributor

raman325 commented Sep 9, 2022

Great! Now we just need to improve test coverage and then we can merge

@mkowalchuk
Copy link
Copy Markdown
Contributor Author

mkowalchuk commented Sep 9, 2022

Updating the branch again seemed to fix the code coverage complaints. It looks like either codecov was doing something strange and not just looking at the files changed in the PR, or I was doing something wrong with the merge.

@MartinHjelmare
Copy link
Copy Markdown
Member

Do we already have a test for this fan? It would be good to exercise the changed discovery code in a fan test for this device.

@mkowalchuk
Copy link
Copy Markdown
Contributor Author

Do we already have a test for this fan? It would be good to exercise the changed discovery code in a fan test for this device.

Added a fixture+test.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great! Thanks for your commitment to this PR.

@MartinHjelmare MartinHjelmare merged commit 19cf5df into home-assistant:dev Sep 9, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 10, 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