Skip to content

deconz: fix light.turn_off with transition#15222

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

deconz: fix light.turn_off with transition#15222
Kane610 merged 1 commit intohome-assistant:devfrom
lbschenkel:deconz

Conversation

@lbschenkel
Copy link
Copy Markdown
Contributor

@lbschenkel lbschenkel commented Jun 29, 2018

This is my first PR against this project, so apologies if there's something I'm missing. I'm submitting this so the review can happen while I set up my local environment to be able to run tox.

Description:

When light.turn_off is invoked with a transition, the following payload was
sent to deCONZ via PUT to /light/N/state:

{ "bri": 0, "transitiontime": transition }

However, on recent versions of deCONZ (latest is 2.05.31 at the time of
writing) this does not turn off the light, just sets it to minimum brightness
level (brightness is clamped to minimum level the light supports without
turning it off).

This commit makes the code send this payload instead:

{ "on": false, "transitiontime": transition }

This works as intended and the light does transition to the 'off' state.
This change was tested with Philips Hue colored lights, IKEA colored lights
and IKEA white spectrum lights: they were all able to be turned off
successfully with the new payload, and none of them could be turned off with
the old payload.

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.

When light.turn_off is invoked with a transition, the following payload was
sent to deCONZ via PUT to /light/N/state:

{ "bri": 0, "transitiontime": transition }

However, on recent versions of deCONZ (latest is 2.05.31 at the time of
writing) this does not turn off the light, just sets it to minimum brightness
level (brightness is clamped to minimum level the light supports without
turning it off).

This commit makes the code send this payload instead:

{ "on": false, "transitiontime": transition }

This works as intended and the light does transition to the 'off' state.
This change was tested with Philips Hue colored lights, IKEA colored lights
and IKEA white spectrum lights: they were all able to be turned off
successfully with the new payload, and none of them could be turned off with
the old payload.
@lbschenkel lbschenkel requested a review from Kane610 as a code owner June 29, 2018 22:21
@homeassistant
Copy link
Copy Markdown
Contributor

Hi @lbschenkel,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jun 29, 2018

What a coincidence, this was just requested over at the deconz GitHub. Looks good, can be merged as soon as tests pass

@lbschenkel
Copy link
Copy Markdown
Contributor Author

I just realized I typed transition and not transitiontime in the description of the payload. I just want to amend the commit message before merging.

@lbschenkel
Copy link
Copy Markdown
Contributor Author

Amended.

@Kane610 Kane610 merged commit 10d1e81 into home-assistant:dev Jun 29, 2018
@ghost ghost removed the in progress label Jun 29, 2018
@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jun 29, 2018

Thanks for your contribution 🎉

@lbschenkel lbschenkel deleted the deconz branch June 29, 2018 23:00
@vandalon
Copy link
Copy Markdown

I've just tested this fix by coping light/deconz.py to custom_components/light/deconz.py and remove line 177 , but now the light turns of immediately instead of after 30 seconds. "entity_id": "light.lamp_naast_de_kast","transition":30

@lbschenkel
Copy link
Copy Markdown
Contributor Author

lbschenkel commented Jun 30, 2018

If you enable debug logging for pydeconz, what do you see being sent to deCONZ when you run the command in HASS? Could it be that your light does not support transition when turning off? Which light is that?

@vandalon
Copy link
Copy Markdown

vandalon commented Jun 30, 2018

I'll try that out. it's a hue light which was previously connected via a hue bridge and supporting transition. So i'm pretty sure the light supports it :) Transition whith turn_on works as it should also.

@vandalon
Copy link
Copy Markdown

i've tried homeassistant.components.deconz: debug and homeassistant.components.pydeconz: debug but not getting any debug output. Which one should I use?

@lbschenkel
Copy link
Copy Markdown
Contributor Author

No, just pydeconz (without any prefix).

@vandalon
Copy link
Copy Markdown

2018-06-30 10:01:13 DEBUG (MainThread) [pydeconz.utils] Sending {'data': '{"on": false, "transitiontime": 100}'} to http://192.168.1.15:8090/api/431338872E/lights/10/state 2018-06-30 10:01:13 DEBUG (MainThread) [pydeconz.utils] HTTP request response: [{'success': {'/lights/10/state/on': False}}] 2018-06-30 10:01:13 DEBUG (MainThread) [pydeconz.websocket] Websocket data: b'\x81G{"e":"changed","id":"10","r":"lights","state":{"on":false},"t":"event"}' 2018-06-30 10:01:13 DEBUG (MainThread) [pydeconz.deconzdevice] Lamp Naast De Kast: update on with False 2018-06-30 10:01:13 DEBUG (MainThread) [pydeconz.websocket] Websocket data: b'\x81J{"e":"changed","id":"1","r":"groups","state":{"all_on":false},"t":"event"}' 2018-06-30 10:01:13 DEBUG (MainThread) [pydeconz.deconzdevice] Woonkamer: update all_on with False

@lbschenkel
Copy link
Copy Markdown
Contributor Author

OK. I also have a color Hue that I tested yesterday and just re-rested right now as I write this and the off+transition definitely works. What version of deCONZ do you have running? I'm running the latest one (2.05.31 from https://github.com/dresden-elektronik/deconz-rest-plugin/) and firmware 0x261f0500. It may be that old versions of deCONZ/firmware behave differently? I strongly recommend running the latest one because there have been tons of fixes in the recent versions.

@vandalon
Copy link
Copy Markdown

I'm also runnning 2.05.31 with the same firmware

@vandalon
Copy link
Copy Markdown

the deconz web frontend does not show a firmware version but i'm sure I flashed it.

@lbschenkel
Copy link
Copy Markdown
Contributor Author

And you're asking to turn off the light with a transition of 10s and it turns off immediately? That's odd. I cannot really explain this, different lamp firmware perhaps? Do you have other lamps you could test, just to see what happens?

@vandalon
Copy link
Copy Markdown

`
root@marvin:/usr/share/hassio/homeassistant# /usr/bin/GCFFlasher_internal -d 0 -f /usr/share/deCONZ/firmware/deCONZ_Rpi_0x261f0500.bin.GCF
pl2303
GCFFlasher V2_11 (c) dresden elektronik ingenieurtechnik gmbh 2017/12/10
using firmware file: /usr/share/deCONZ/firmware/deCONZ_Rpi_0x261f0500.bin.GCF
reset via FTDI

flashing 126188 bytes: |==============================|
verify: ....
SUCCESS
root@marvin:/usr/share/hassio/homeassistant# systemctl restart deconz
root@marvin:/usr/share/hassio/homeassistant# dpkg -l |grep deconz
ii deconz 2.05.31 amd64 A generic ZigBee monitoring and control tool
`

webui:

Version 2.05.31 / 22-6-2018
Firmware 00000000

@vandalon
Copy link
Copy Markdown

vandalon commented Jun 30, 2018

i've tested with 3 hue color bulbs, 1 hue white and 1 innr bulb. All go off instant.

@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jun 30, 2018

Can you really override a platform by just adding a custom component with the same name?

@lbschenkel
Copy link
Copy Markdown
Contributor Author

Oh, I missed that it was an override. I thought you edited the originaldeconz.py. I would try that instead.

@vandalon
Copy link
Copy Markdown

vandalon commented Jun 30, 2018 via email

@vandalon
Copy link
Copy Markdown

Thing is. Before it dimmed the light and did not turn them off. So behavior is certainly different

@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jun 30, 2018

Im asking so we don't diagnose an issue that is something else

@lbschenkel
Copy link
Copy Markdown
Contributor Author

lbschenkel commented Jun 30, 2018

But anyhow, the REST payload sent to deCONZ looks correct. Whatever is happening here and if there is a bug, I believe it is on the deCONZ side — at least in theory. This payload is the correct payload to request the light to be turned off with a transition. I cannot explain why this is not being honoured by deCONZ (or maybe the lights) in your case. I tried just now with 4 different models of bulbs from 2 different brands and they all worked, so it's odd.

@vandalon
Copy link
Copy Markdown

just te be sure; it's just deleting this one line in light/deconz.py right?

@vandalon
Copy link
Copy Markdown

i'm a bit unsure because the deconz web ui does not show my firmware version, so there might be something wrong there?

@lbschenkel
Copy link
Copy Markdown
Contributor Author

Actually I think I might have found the issue. I played with the REST interface a bit. You're not seeing the light go immediately off, but with a transition of ~1s, correct?

@vandalon
Copy link
Copy Markdown

yes, that is right. But that is quite normal with hue light anyway so I did not notice it being different then going off instantly

@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jun 30, 2018

It is just a cosmetic issue with Phoscon not showing the fw ID. There is an issue reported with DE to solve.

@vandalon
Copy link
Copy Markdown

Ok, just tested with the change to the original light/deconz.py file, same issue :)

@lbschenkel
Copy link
Copy Markdown
Contributor Author

lbschenkel commented Jun 30, 2018

OK, so I think this is on me. My PR ended up being incomplete. I was doing experimentation to figure out why the lights were not turning off and what was the exact payload to fix that. The payload that actually solves it is this one:

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

Without bri, the light turns off but apparently deCONZ then ignores the transition. When I was working on the PR I was testing with relatively short transitions, so I somehow missed the fact that when bri was missing the transition was actually being ignored.

What muddied the waters completely is that my "production" HASS, due to my experimentation, ended up having this version with the bri enabled (the one I was using now while writing to you). Since it was so odd that you were seeing a different thing I had to review my HASS logs and then caught the payload difference. I'm going to create a new PR to re-introduce the bri. Sorry for the confusion.

@vandalon
Copy link
Copy Markdown

awesome work man! thanks for the effort!

@Kane610
Copy link
Copy Markdown
Member

Kane610 commented Jun 30, 2018

@lbschenkel so you should change to not rewrite the dict when using transition as was previously the case

@lbschenkel
Copy link
Copy Markdown
Contributor Author

Yes: #15227

awarecan pushed a commit to awarecan/home-assistant that referenced this pull request Jul 16, 2018
When light.turn_off is invoked with a transition, the following payload was
sent to deCONZ via PUT to /light/N/state:

{ "bri": 0, "transitiontime": transition }

However, on recent versions of deCONZ (latest is 2.05.31 at the time of
writing) this does not turn off the light, just sets it to minimum brightness
level (brightness is clamped to minimum level the light supports without
turning it off).

This commit makes the code send this payload instead:

{ "on": false, "transitiontime": transition }

This works as intended and the light does transition to the 'off' state.
This change was tested with Philips Hue colored lights, IKEA colored lights
and IKEA white spectrum lights: they were all able to be turned off
successfully with the new payload, and none of them could be turned off with
the old payload.
@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
When light.turn_off is invoked with a transition, the following payload was
sent to deCONZ via PUT to /light/N/state:

{ "bri": 0, "transitiontime": transition }

However, on recent versions of deCONZ (latest is 2.05.31 at the time of
writing) this does not turn off the light, just sets it to minimum brightness
level (brightness is clamped to minimum level the light supports without
turning it off).

This commit makes the code send this payload instead:

{ "on": false, "transitiontime": transition }

This works as intended and the light does transition to the 'off' state.
This change was tested with Philips Hue colored lights, IKEA colored lights
and IKEA white spectrum lights: they were all able to be turned off
successfully with the new payload, and none of them could be turned off with
the old payload.
@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