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 transition support to zwave_js lights #52160

Merged
merged 29 commits into from Jul 9, 2021

Conversation

firstof9
Copy link
Contributor

@firstof9 firstof9 commented Jun 24, 2021

Proposed change

Add transition support to zwave_js lights

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

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

@project-bot project-bot bot added this to Needs review in Dev Jun 24, 2021
@probot-home-assistant
Copy link

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

@firstof9
Copy link
Contributor Author

firstof9 commented Jun 24, 2021

Waiting for library update.

@raman325 raman325 added the waiting-for-upstream We're waiting for a change upstream label Jun 24, 2021
@firstof9 firstof9 marked this pull request as ready for review June 24, 2021 22:38
@raman325
Copy link
Contributor

Assuming this PR goes through: zwave-js/node-zwave-js#2906 we should also update the logic to determine whether or not to add SUPPORT_TRANSITIONS to the supported features to use that. This will also require upstream changes but I can make those pretty quickly once someone can give me a state dump

@homeassistant

This comment has been minimized.

@MartinHjelmare
Copy link
Member

We've bumped client library in core so we should be able to address this comment now:
#52160 (comment)

@MartinHjelmare MartinHjelmare removed the waiting-for-upstream We're waiting for a change upstream label Jun 29, 2021
@firstof9 firstof9 marked this pull request as draft June 29, 2021 18:04
@raman325
Copy link
Contributor

raman325 commented Jul 8, 2021

Strange because it's typed to be an int in the constructor.

That won't prevent a float from being passed in, it's just used in typechecking.

@menzy81

This comment has been minimized.

@MarvinSchenkel
Copy link

I have just updated to 2021.07 running the latest Zwave-JS version and I am also have the same issue of transition being ignored on my Aeotec dimmers.

What happens:
The light goes straight to 80% brightness.
What I expect to happen:
Over 30 seconds the light transitions from its current brightness to 80%

service: light.turn_on
target:
  entity_id: light.kitchen_ceiling_level
data:
  transition: 30
  brightness_pct: 80

This PR has not been merged yet, which means it is not part of the 2021.07 release. You have to manually download the zwave_js component, copy it to your custom_components folder and add a version number in the manifest.json in order to load this fix.

@firstof9
Copy link
Contributor Author

firstof9 commented Jul 8, 2021

That won't prevent a float from being passed in, it's just used in typechecking.

Ya, in other languages it'd convert them last I checked (ie: PHP, C++)

Dev automation moved this from Needs review to Reviewer approved Jul 8, 2021
Copy link
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.

LGTM, thanks!

One last question - do we definitely want to use a default transition when transitions are supported and one isn't specified? I don't know enough to make a determination on that, so will trust your judgement

@kpine
Copy link
Contributor

kpine commented Jul 8, 2021

One last question - do we definitely want to use a default transition when transitions are supported and one isn't specified? I don't know enough to make a determination on that, so will trust your judgement

When you don't specify a transition, node-zwave-js will use the V1 Set command, which doesn't use a duration, it's only the target value. When you specify transition, node-zwave-js will use the V2 Set command, which includes the duration. If you specify "default", it will set duration to 0x255.

Using the V1 Set on some switches seems to be problematic. Forcing node-zwave-js to set the transition to the default is a workaround for those devices. I would prefer that node-zwave-js just does it instead, but it doesn't. It seemed like an easy workaround though.

However, @jcam were you able to confirm that the change to use "default" actually solves your problem? It wasn't clear if the problem was actually in regards to default transitions in HA in general, or if it was about these particular switches not working. If it doesn't fix the problem, I would just remove setting of the default and let node-zwave-js handle things. Besides these particular switches, no similar issues have been reported from what I've seen.

@firstdominator
Copy link

Does this means this will be part of 2021.8 ?

@firstof9
Copy link
Contributor Author

firstof9 commented Jul 8, 2021

Highly likely, depends on if/when it get's merged.

@raman325
Copy link
Contributor

raman325 commented Jul 8, 2021

Does this means this will be part of 2021.8 ?

yes

EDIT: first's statement is more accurate

@@ -44,6 +44,8 @@
ColorComponent.PURPLE: "purple",
}

TRANSITION_DURATION = "transitionDuration"
Copy link
Member

Choose a reason for hiding this comment

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

Side note: This constant should be moved to the client library in the future.

homeassistant/components/zwave_js/light.py Outdated Show resolved Hide resolved
homeassistant/components/zwave_js/light.py Outdated Show resolved Hide resolved
homeassistant/components/zwave_js/light.py Outdated Show resolved Hide resolved
homeassistant/components/zwave_js/light.py Outdated Show resolved Hide resolved
homeassistant/components/zwave_js/light.py Outdated Show resolved Hide resolved
Dev automation moved this from Reviewer approved to Review in progress Jul 9, 2021
firstof9 and others added 5 commits July 9, 2021 07:33
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Copy link
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.

Looks good! Just needs to run black.

Dev automation moved this from Review in progress to Reviewer approved Jul 9, 2021
@raman325 raman325 merged commit 92ab471 into home-assistant:dev Jul 9, 2021
Dev automation moved this from Reviewer approved to Done Jul 9, 2021
@jcam
Copy link
Contributor

jcam commented Jul 9, 2021

Apologies I have not been able to re-test with sending v2 set with default to see if that resolves everything with the eaton switches.
I know that for my fibaro LED strip dimmers, the v1 command was problematic in that it would use random duration times of 5-500 seconds. But I've had a forced V2 w/2 second duration for months now, so I have not effectively tested the recent code branches. I will try to do that this weekend for both switches

@jcam
Copy link
Contributor

jcam commented Jul 10, 2021

Using:
zwavejs2mqtt: 5.2.1
zwave-js: 7.10.1
core-2021.7.1

With the current rel zwave-js component:

  • the fibaro led strip color switch fades on and off with a 5 second duration, changes colors with a 0 second duration, and misses about 70% of the color change requests when sending from the color wheel.
  • the eaton RF switch turns on and off with a 0 second duration, and leaves about 35 volts going through the wires when turning off (goes to 0-2 volts when operated from the switch's physical button)
  • above is the same regardless of passing in transition or not (as expected)

With the custom component in dev (this PR):

  • fibaro led strip color switch is exactly the same as before
  • the eaton RF switch turns on with about a 2 second fade, and off with about a 4 second fade, matching the operation of the physical switch. when off, it goes to 0-2 volts as it should.
  • when passing in a transition to the light turn on/turn off services, they are respected for both types of switch

So, @kpine looks like sending 'default' for the transition does work for this switch. This matches how smartthings functions (or at least how i remember it working, i binned it as soon as I got zjs working at all), which was the only other hub on the market that worked properly with these switches.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 11, 2021
@firstof9 firstof9 deleted the zwave-js-light-transitions branch August 9, 2021 15:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
9 participants