Skip to content

Adding support for a white value #3338

Merged
balloob merged 11 commits into
home-assistant:devfrom
marcpabst:mxtra_white
Sep 21, 2016
Merged

Adding support for a white value #3338
balloob merged 11 commits into
home-assistant:devfrom
marcpabst:mxtra_white

Conversation

@marcpabst
Copy link
Copy Markdown
Contributor

I'm quite new to GitHub, so please don't roast me. :)

Description:
Adding support for a white value to improve controlling of RGBW-lights that allow to set each channel independently.

This was done by simply copying the code for the color temp functionality but setting the range from 0 to 255.

There is also one file in the frontend that has to be changed. home-assistant/frontend#105

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@marcpabst marcpabst changed the title Mxtra white Adding support for a white value Sep 12, 2016
Marc Pabst added 2 commits September 12, 2016 11:21
@shmuelzon
Copy link
Copy Markdown
Contributor

Please forgive my ignorance here, but what is the difference between what you've added and the color temperature setting (also in mireds)?

@marcpabst
Copy link
Copy Markdown
Contributor Author

My fault. I actually forget to change the description. Some lights allow 4 channels (red, green, blue, white) to be controlled at the same time. So this is the the white component from 0 to 255.

@MartinHjelmare
Copy link
Copy Markdown
Member

Looks good! I'll use this for mysensors.

@MartinHjelmare
Copy link
Copy Markdown
Member

Add tests that test the function and service and use ATTR_WHITE_VALUE. See:
https://github.com/home-assistant/home-assistant/blob/dev/tests/components/light/test_init.py

When that is done this should be good to go.

@marcpabst
Copy link
Copy Markdown
Contributor Author

I'm not familiar with testing and therefore have absolutely no idea what I'm supposed to do. :(

@balloob
Copy link
Copy Markdown
Member

balloob commented Sep 14, 2016

I would prefer to only add this if it includes a platform that is using this method.

You should also add it to the demo light so we are able to test the UI part.

And tests like @MartinHjelmare mentioned.

@MartinHjelmare
Copy link
Copy Markdown
Member

@mxtra If you like, we could team up on this and I could help with tests and add the feature to the mysensors platform.

Let me know if that's OK and if I can push to your fork branch. I'll have time over the weekend.

@marcpabst
Copy link
Copy Markdown
Contributor Author

marcpabst commented Sep 14, 2016

@MartinHjelmare I think that is an excellent idea. I would really appreciate your help!
I already implemented it into the Flux/Magic Home platform but it's not ready yet.

@MartinHjelmare
Copy link
Copy Markdown
Member

Great! You can PM me on gitter if you want also. What's your timezone? I'm CET.

@marcpabst
Copy link
Copy Markdown
Contributor Author

I'm in Germany, so it's CET as well. :) I'll reach out to you when I come home tonight.

* Activate support for mysensors RGBW devices with support for
	white_value attribute.
* Add white_value support in light demo platform.
* Add tests for white_value and more for light component.
* Add tests for light demo platform.
* Fix import order in check_config.
@MartinHjelmare
Copy link
Copy Markdown
Member

Backend should be ok now. I haven't tested the frontend yet.

@MartinHjelmare
Copy link
Copy Markdown
Member

I guess we need to rebase off dev to have accurate coverage results. The changes I've made should be covered by new tests or excluded in case of mysensors, via coveragerc.

@balloob
Copy link
Copy Markdown
Member

balloob commented Sep 20, 2016

The coverage results are accurate, due to asyncio we have dropped to 93.6. We'll fix that soon ;-)

@balloob
Copy link
Copy Markdown
Member

balloob commented Sep 20, 2016

Code looks good 👍

What's the difference between brightness and white value?

@MartinHjelmare
Copy link
Copy Markdown
Member

Brightness is a general term for the "power" of a light, for one or many channels. Some devices control all channels at the same time with a brightness control, others control RGB separately from W. To be able to control only the W channel and support all device types, we need a parameter for White that is separate from Brightness.

@balloob balloob merged commit 138205a into home-assistant:dev Sep 21, 2016
@balloob
Copy link
Copy Markdown
Member

balloob commented Sep 21, 2016

🐬

@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants