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

Turn light off if brightness is 0 #22400

Merged
merged 6 commits into from Mar 31, 2019

Conversation

@emontnemery
Copy link
Contributor

commented Mar 25, 2019

Breaking Change:

Lights will now be turned off when brightness is set to 0

Description:

Turn light off if when brightness is set to 0 as discussed in home-assistant/architecture#119

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#8967

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

If the code does not interact with devices:

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

@ghost ghost assigned emontnemery Mar 25, 2019

@ghost ghost added the in progress label Mar 25, 2019

await light.async_turn_on(**pars)
if ATTR_BRIGHTNESS in pars and pars[ATTR_BRIGHTNESS] == 0:
# Zero brightness: Turn the light off
for k in list(pars.keys()):

This comment has been minimized.

Copy link
@dgomes

dgomes Mar 26, 2019

Contributor

pars = {k: v for k,v in pars.items() if k not in [ATTR_TRANSITION, ATTR_FLASH]}

This comment has been minimized.

Copy link
@balloob

balloob Mar 26, 2019

Member

Minor typo: …k in [ATTR…

This comment has been minimized.

Copy link
@emontnemery

emontnemery Mar 27, 2019

Author Contributor

Fixed!

@dgomes

dgomes approved these changes Mar 26, 2019

@@ -282,7 +282,14 @@ class SetIntentHandler(intent.IntentHandler):
pars = params.copy()
pars[ATTR_PROFILE] = Profiles.get_default(light.entity_id)
preprocess_turn_on_alternatives(pars)
await light.async_turn_on(**pars)
if ATTR_BRIGHTNESS in pars and pars[ATTR_BRIGHTNESS] == 0:

This comment has been minimized.

Copy link
@balloob

balloob Mar 26, 2019

Member

Don't do this inside the loop. All parameters should be processed before we start looping over lights.

This comment has been minimized.

Copy link
@emontnemery

emontnemery Mar 26, 2019

Author Contributor

The parameters are written to in the loop though.

This comment has been minimized.

Copy link
@balloob

balloob Mar 27, 2019

Member

params is not.

This comment has been minimized.

Copy link
@emontnemery

emontnemery Mar 27, 2019

Author Contributor

Alright, I moved the processing to preprocess_turn_on_alternatives


if ATTR_BRIGHTNESS in params and params[ATTR_BRIGHTNESS] == 0:
# Zero brightness: Light will be turned off
for k in list(params.keys()):

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Mar 27, 2019

Member

Remove this for loop.

This comment has been minimized.

Copy link
@emontnemery

emontnemery Mar 28, 2019

Author Contributor

Fixed

if ATTR_BRIGHTNESS in params and params[ATTR_BRIGHTNESS] == 0:
# Zero brightness: Light will be turned off
for k in list(params.keys()):
params = {k: v for k, v in params.items() if k in [ATTR_TRANSITION,

This comment has been minimized.

Copy link
@balloob

balloob Mar 28, 2019

Member

You will need to add a test so that you will notice that this won't work, as preprocess_turn_on_alternatives currently does not have a return value

This comment has been minimized.

Copy link
@emontnemery

emontnemery Mar 28, 2019

Author Contributor

Wow, that was stupid..
Fixed, and tests updated.

emontnemery added some commits Mar 28, 2019

'brightness': 0
}
],
# Brightness < 0

This comment has been minimized.

Copy link
@emontnemery

emontnemery Mar 28, 2019

Author Contributor

This test was meaningless since brightness is clamped from 0..255 by the light schema

@@ -35,20 +35,12 @@
'brightness': 100
}
],
# Brightness == 0

This comment has been minimized.

Copy link
@emontnemery

emontnemery Mar 28, 2019

Author Contributor

Change to 1 which is now minimum possible brightness

@@ -266,8 +266,6 @@ def hs_color(self):
brightness = kwargs[ATTR_BRIGHTNESS]
if brightness > 254:
brightness = 254
elif brightness < 0:

This comment has been minimized.

Copy link
@emontnemery

emontnemery Mar 28, 2019

Author Contributor

This can not happen since brightness is clamped between 0..255 by the light schema, and async_turn_on will only be called if brightness is larger than 0 with this PR

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@lwis i removed some code in trådfri light which doesn't seem to be useful:

elif brightness < 0:
brightness = 0

It was added in #11187, do you remember if it was needed?

@lwis

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@lwis Right, I imagined it was something like that. However, it should not be possible to have negative brightness values since brightness is clamped by the light component's validation schema:

# Service call validation schemas
VALID_TRANSITION = vol.All(vol.Coerce(float), vol.Clamp(min=0, max=6553))
VALID_BRIGHTNESS = vol.All(vol.Coerce(int), vol.Clamp(min=0, max=255))
VALID_BRIGHTNESS_PCT = vol.All(vol.Coerce(float), vol.Range(min=0, max=100))

@ghost ghost removed the in progress label Mar 29, 2019

@emontnemery emontnemery reopened this Mar 29, 2019

@ghost ghost added the in progress label Mar 29, 2019

@balloob balloob merged commit 4d16338 into home-assistant:dev Mar 31, 2019

4 of 6 checks passed

Build Error Workflow: Build Error
Details
ci/circleci: Build Error Your tests failed on CircleCI
Details
Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 93.372%
Details

@ghost ghost removed the in progress label Mar 31, 2019

@emontnemery emontnemery deleted the emontnemery:zero_brightness_light_off branch Apr 3, 2019

unibeck pushed a commit to unibeck/home-assistant that referenced this pull request Apr 7, 2019

Turn light off if brightness is 0 (home-assistant#22400)
* Turn light off if brightness is 0

* Lint

* Review comments

* Lint

* Fixup, add tests

* Fix trådfri light + test
@xydix

This comment has been minimized.

Copy link

commented Apr 16, 2019

Should this affect all light platforms? I try to turn off a light by set brighness to 0 but it doesn't work. In this case I use Deconz.

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@xydix Yes, it should affect all platforms. Can you open a bug report / issue if its not working for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.