Skip to content

deconz: fix light.turn_off with transition (2)#15227

Merged
Kane610 merged 1 commit intohome-assistant:devfrom
lbschenkel:deconz
Jul 1, 2018
Merged

deconz: fix light.turn_off with transition (2)#15227
Kane610 merged 1 commit intohome-assistant:devfrom
lbschenkel:deconz

Conversation

@lbschenkel
Copy link
Copy Markdown
Contributor

@lbschenkel lbschenkel commented Jun 30, 2018

Description:

Previous commit d4f7dfa successfully fixed the bug in which lights
would not turn off if a transition was specified, however if bri is not
present in the payload of the PUT request set to deCONZ, then any
transitiontime ends up being ignored. This commit addresses the
unintended side effect by reintroducing bri, resulting in the following
payload:

{ "on": false, "bri": 0, "transitiontime": ... }

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

Previous commit d4f7dfa successfully fixed the bug in which lights
would not turn off if a transition was specified, however if 'bri' is not
present in the payload of the PUT request set to deCONZ, then any
'transitiontime' ends up being ignored. This commit addresses the
unintended side effect by reintroducing 'bri', resulting in the following
payload:

{ "on": false, "bri": 0, "transitiontime": ... }
@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jun 30, 2018

Have you done some extra verification that this actually gets the wanted behavior? ;)

@lbschenkel
Copy link
Copy Markdown
Contributor Author

Yes. This is the exact version confirmed to be working with for both the "off" state and also for transitions being honoured. But I would welcome if @vandalon could do the same modification in his system and confirm that it also works for him, before this gets merged.

@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jun 30, 2018

@lbschenkel yes that would be good!

@vandalon
Copy link
Copy Markdown

Will do in an hour. At swimming lessons with the kids now :)

@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jun 30, 2018

No hurry, we got two weeks to get this merged :)

@vandalon
Copy link
Copy Markdown

To bad i can not remotely see how the bulb reacts :) I've got everything in place just need to see the blub :)

@vandalon
Copy link
Copy Markdown

vandalon commented Jun 30, 2018

Ok, girlfriend confirmed slowly going off :) so it works

@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jun 30, 2018

Tell me when you're satisfied with the testing

@vandalon
Copy link
Copy Markdown

vandalon commented Jun 30, 2018

Well there is still something weird. Running the command from HA works, running it via Node Red it still turns off the lights instantly. where node red sends the exact same command to HA.

@lbschenkel
Copy link
Copy Markdown
Contributor Author

Please enable pydeconz debug logging and post the payloads here from both scenarios, let's see if there's any difference.

@vandalon
Copy link
Copy Markdown

My bad! I made a config error within Node Red. All works perfectly!

@lbschenkel
Copy link
Copy Markdown
Contributor Author

I am having a serious trouble typing transitiontime correctly in commit messages. I have taken the opportunity to amend it before merging. No code changes.

@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jul 1, 2018

Good to go then?

@lbschenkel
Copy link
Copy Markdown
Contributor Author

Yes from me. @vandalon?

@vandalon
Copy link
Copy Markdown

vandalon commented Jul 1, 2018

Yes, absolutely

@Kane610 Kane610 merged commit 3c04b07 into home-assistant:dev Jul 1, 2018
@ghost ghost removed the in progress label Jul 1, 2018
@lbschenkel lbschenkel deleted the deconz branch July 1, 2018 10:35
@lbschenkel
Copy link
Copy Markdown
Contributor Author

Thanks @Kane610. I'm not so familiar with the release process, how much time can we typically expect for code landing in dev to appear on a release?

@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jul 1, 2018

If you're running on stable release you can expect this to be available in 0.74, in ~3 weeks

@vandalon
Copy link
Copy Markdown

vandalon commented Jul 1, 2018

In the meanwhile you can place the adjusted deconz.py in custom_components/light/

awarecan pushed a commit to awarecan/home-assistant that referenced this pull request Jul 16, 2018
Previous commit d4f7dfa successfully fixed the bug in which lights
would not turn off if a transition was specified, however if 'bri' is not
present in the payload of the PUT request set to deCONZ, then any
'transitiontime' ends up being ignored. This commit addresses the
unintended side effect by reintroducing 'bri', resulting in the following
payload:

{ "on": false, "bri": 0, "transitiontime": ... }
@balloob balloob mentioned this pull request Jul 20, 2018
girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018
Previous commit d4f7dfa successfully fixed the bug in which lights
would not turn off if a transition was specified, however if 'bri' is not
present in the payload of the PUT request set to deCONZ, then any
'transitiontime' ends up being ignored. This commit addresses the
unintended side effect by reintroducing 'bri', resulting in the following
payload:

{ "on": false, "bri": 0, "transitiontime": ... }
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants