Changing z-wave brightness calculation to respect 0x01 and 0x02 byte values #16420
Conversation
|
Hi @Harvtronix, 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! |
homeassistant
added
cla-needed
platform: light.zwave
small-pr
ghost
added
the
| @@ -62,6 +62,15 @@ def brightness_state(value): | |||
| return round((value.data / 99) * 255), STATE_ON | |||
| return 0, STATE_OFF | |||
|
|
|||
| def byte_to_zwave_brightness(value): | |||
| """ | ||
| if value > 0: | ||
| return max(1, int((value / 255) * 99)) | ||
| return 0 |
There was a problem hiding this comment.
If the value is 0, shouldn't we just return None (there is no brightness) ?
There was a problem hiding this comment.
@balloob The brightness value as it was used in the original code was an integer in the range of 0 to 99, with 255 representing a special case. I wanted to stay as close to this behavior as possible to minimize the risk of regressing existing code.
I'm open to whatever changes you'd like. I was just going for "minimally invasive" :)
There was a problem hiding this comment.
We should keep it as returning zero, since this is the brightness that we send to the ZWave device, not the brightness value for hass.
There was a problem hiding this comment.
I'm 👍 with this change overall. Great catch, and thanks for adding tests!
You'll also need to fix the picky lint issues Travis called out:
https://travis-ci.org/home-assistant/home-assistant/jobs/424420886#L561-L562
| """ | ||
| if value > 0: | ||
| return max(1, int((value / 255) * 99)) | ||
| return 0 |
There was a problem hiding this comment.
We should keep it as returning zero, since this is the brightness that we send to the ZWave device, not the brightness value for hass.
|
@armills Thanks! And thanks for pointing that out! Looks like some comment formatting issues. I'll search for a multiline comment and model after that. Strangely, the linter that was running locally after I imported the project into vscode wasn't reporting these same issues. |
- Add additional unit test to increase code coverage
| device.turn_on(**{ATTR_BRIGHTNESS: 0}) | ||
|
|
||
| assert device.is_on | ||
| assert device.brightness == 0 |
There was a problem hiding this comment.
@armills I thought about the second part of this test here that I added some more, and I'd like your opinion as to whether or not the result makes sense. Right now, calling turn on with a brightness val of 0, will result in device.is_on returning true... Is that the intended behavior? If not, I can add an else block to the turn_on method that sets the state to off in the case of 0 being passed in.
There was a problem hiding this comment.
Good catch. That really should be cleaned up.
If you don't mind we could also refactor it to make it a little simpler. self._state can just be a boolean. STATE_ON can be replaced with True, STATE_OFF can be replaced with False, and we can return it directly here: https://github.com/home-assistant/home-assistant/blob/a40a6e31ada250d48241698ed133a847f45508a0/homeassistant/components/light/zwave.py#L154 . Then in the turn_on method you can just assign self._state = brightness > 0.
|
I've merged this PR since the state problem existed already, and this is a solid bug fix. If you can help clean up that state problem you can open a new PR for that fix. Thanks for the contribution! 🌟 |
|
@armills Sure thing! I should hopefully be able to work on that over the next couple of days. |
Description:
Changing z-wave brightness calculation to respect 0x01 and 0x02 byte values
Related issue (if applicable): fixes #16406
Pull request in home-assistant.github.io with documentation (if applicable): n/a
Example entry for
configuration.yaml(if applicable): n/aChecklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices: