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

Lights: properly divide rgb values for hsv conversion #1902

Merged
merged 3 commits into from
Sep 10, 2019

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Sep 7, 2019

resolves #1864

@CrazyIvan359
Copy link
Contributor

CrazyIvan359 commented Sep 7, 2019

With color off ch0-2 mqtt reports 32,64,32 brightness 64, the webui shows 128,255,128 and brightness 64.
Changing brightness to 0 makes all mqtt messages 0.
Changing brightness to 255 and mqtt and webui show 128,255,128 and brightness 255
No output at all from the device though, all physical lights remain off.

With color on results are identical. Also tried controlling ch3-4 (white options are off) and they respond in webui and mqtt, but also no actual output from device.
Sending HSV commands works perfectly though, as does the HSV returned on mqtt, but again no physical output from device.

@mcspr
Copy link
Collaborator Author

mcspr commented Sep 8, 2019

But that's how it always was? Web shows value and everything else sends out modified value (value*brightness,value_cct). Only useful change would be to drop "value" part of hsv from displayed properties, leaving only hue and saturation, since we already define brightness.

As a baseline, using Home Assistant rgb topic via mqtt.
root/brightness is 100, root/rgb 8,12,100. using mosquitto_pub and sending root/rgb 22,31,255 to the HA instance keeps the colorpicker intact, since it also applies brightness to the resulting color.

I'll play around with hue and saturation there to make more sense of it
https://www.home-assistant.io/components/light.mqtt/#hs_command_topic
https://www.home-assistant.io/components/light.mqtt/#hs_state_topic

No idea about the output, does it break because of a95a83e?

@CrazyIvan359
Copy link
Contributor

But that's how it always was?

With color on, ok, but it did the same with color off. Should that be happening?

Only useful change would be to drop "value" part of hsv from displayed properties, leaving only hue and saturation, since we already define brightness.

Are the Value and brightness essentially linked? Setting V should change B, but not the other way.

No idea about the output, does it break because of a95a83e?

I will check.

@mcspr
Copy link
Collaborator Author

mcspr commented Sep 9, 2019

Channel change was part of #1575 & #1826, as you too had noticed in the issue. So the modes are CCT, Color (brightness affects first 3 channels) and No Color (brightness affects all channels)
And my understanding was that HSV and HSB are interchangeable and V is B in this case too. e.g. HA example above ignores V/B and only sends / receives Hue and Saturation. Brightness is controlled separately.

And I tried using basic LED attached to the wemos pins and I did see it changing brightness. Using this branch, no changes otherwise

@CrazyIvan359
Copy link
Contributor

Didn't get to testing this today.

If brightness affecting channels when color is off is by design then yes, working well.

My understanding as well is that HSV and HSB are just different names for the same thing.

I will reflash the device when I have a minute at test. If it's working for you I'd say it's fairly safe to merge. By the numbers it was working in my tests as well, so my lack of output may be a different issue.

@mcspr mcspr merged commit 7959863 into xoseperez:dev Sep 10, 2019
@mcspr mcspr deleted the light/hsv-target branch September 10, 2019 09:45
@mcspr
Copy link
Collaborator Author

mcspr commented Sep 10, 2019

@CrazyIvan359 thank you! I also merged this with the #1901 with updated webui
And I had noticed what is wrong with lights too, will update later today.

@CrazyIvan359
Copy link
Contributor

Awesome! I haven't had a chance to test further yet, I don't have much time during the week for that. I'll wait for you to push changes before I test again.

@mcspr
Copy link
Collaborator Author

mcspr commented Sep 10, 2019

See #1905
This is really dumb problem which would've probably shown up with some additional gcc warning flags... value variable (type uint8_t) was reused inside of the function and was used to store a larger type (uint32_t)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HSV command is not HSV set
2 participants