HSV command is not HSV set

[CrazyIvan359]

Bug description
HSV commands do not apply the H, S, and V in the command.

Steps to reproduce
Send an HSV command to a device with Color enabled.

Expected behavior
HSV set should be the same as the command.

Device information

• Arduino Core 2.3.0 rev 159542381
• ESPurna version 1.13.5

Solution
The "Value" of the command is being used as the brightness, then the calculated RGB channel values are scaled by it. This should not be done, as the conversion from HSV to RGB takes the V component into consideration already.

[mcspr]

Can you give an example of expected values?

Following the brightness function, it does some other transformations when "useWhite" mode is on. However, basic brightness method will simply multiply each channel by the "current" brightness divided by "max" brightness, resulting in the same values as this conversion shows:
https://www.wolframalpha.com/input/?i=0,100,57+hsv+to+rgb

espurna/code/espurna/light.ino

Line 1212 in 43a44cf

_light_brightness_func = []() { _lightApplyBrightness(3); };

espurna/code/espurna/light.ino

Line 117 in 43a44cf

_light_channel[i].value = _light_channel[i].inputValue * brightness;

Extracted light.ino algorithm:
https://gist.github.com/mcspr/4a0a7001c9b52c0dd09ca1af13b287c5

espurna/code/espurna/light.ino

Lines 274 to 276 in 43a44cf

	//INPUT: [0,100,57]
	//IS: [145,0,0]
	//SHOULD: [255,0,0]

$ ./hsv 0,100,57
brightness=0, rgb=0,0,0
input=0,100,57
_setRGBInputValue(255,0,0)
brightness=145, rgb=255,0,0

Note about Arduino.h => std::...
constrain => std::clamp
round => std::lround
floor => std::floor

[CrazyIvan359]

I'm running the latest release, not dev. I fixed the issue by preventing the brightness from being calculated when an HSB command is received.

I think if you were to use the maximum Brightness value (100% not MAX_BRIGHTNESS) when calculating the RGB values, and then set actual brightness normally, the _light_brightness_func should adjust the values to what they should be. I ran out of time to keep testing different solutions though.

I do see that another issue I found has been fixed now, I saw that channels 4 and 5 would be set by brightness if colour was enabled, regardless of white settings.

[mcspr]

Ping. Was #1874 sufficient?

[CrazyIvan359]

Sorry Max, I did not have time to load it on any of my devices yet. I will let you know the results when I do, hopefully tonight if not then tomorrow.

[CrazyIvan359]

@mcspr it works perfectly! Thank you!

Hopefully I'll be able to contribute more next time I find a bug. The new brightness function was over my head in terms of my C++ knowledge.

[mcspr]

Wait a sec. Display value is still wrong tho

espurna/code/espurna/light.ino

Line 395 in 2478847

v = 100.0 * max;

The first message, before edit, v is 12700

Let's ignore c++ for now, and look at this purely by what's happening with math part. If r,g,b above are 0...255, should'nt they be divided by 255 (aka VALUE_MAX)?

espurna/code/espurna/light.ino

Lines 388 to 393 in 2478847

	double r = static_cast<double>(target ? _light_channel[0].target : _light_channel[0].value);
	double g = static_cast<double>(target ? _light_channel[1].target : _light_channel[1].value);
	double b = static_cast<double>(target ? _light_channel[2].target : _light_channel[2].value);
	double min = std::min(r, std::min(g, b));
	double max = std::max(r, std::max(g, b));

[CrazyIvan359]

Right... so my lighting automation was putting values on channel/{0-2}/set which is why I didn't catch it at first. When that happens, where is the brightness being calculated from? Max out of r, g, b?

Btw I switched off color mode to get around this issue until you or Xose had some time to look into it, that's why commands were going to channels directly.

[mcspr]

Yes, since channel value already has brightness applied, and we want to use percentages instead of raw numbers
https://en.wikipedia.org/wiki/HSL_and_HSV#From_RGB
https://www.rapidtables.com/convert/color/rgb-to-hsv.html for some interactivity
I'll test a bit more and add the patch.

cc @xoseperez anyway
And this might be a good time to apply some tests to the calculations (e.g., that value == fromHSV(toHSV(value))), but I am not yet sure how to properly integrate it here. Another candidate was #1729 module, but there I just built a host executable with bunch of test asserts for internal structures and mocked serial input / output.

[mcspr]

And also thank you for the full logs! Because at first I thought it was formatting issue again. Not quite...

[CrazyIvan359]

Yes, since channel value already has brightness applied, and we want to use percentages instead of raw numbers

It is possible what I was seeing for V was the channel set command * 100. I can retest for more detailed results tomorrow if you need.

Also let me know if you have a branch you want to test before merging. I should be more available for testing this weekend.

[mcspr]

Using _light_channel[...].target / Light::VALUE_MAX does indeed show proper values. The only question is whether inputValue is useful.

Running magichome-led-controller on wemos board, I do see one significant difference.

• target/value. this is what it is right now:
  
• target/inputValue. showing values without brightness. web receives inputValue aka value without brightness:
  
  Since we get the hsv params for the web via _fromHSV method.

[CrazyIvan359]

Brightness doesn't follow V?

[mcspr]

Yes. Basically, we should not use values without brightness, _light_channel[...].target / Light::VALUE_MAX is the only proper conversion
I'm also seeing some rounding weirdness and sending values twice, need to check that out too.

[CrazyIvan359]

Wouldn't the original arrangement work as long as fromHSV sets inputValue using H, S, 100 and then brightness from V? The issue was that the values were being scaled to V in fromHSV and then again in the brightness function.

[mcspr]

It would, but it was incorrect in other direction :) If target value is multiplied by brightness, we got a different hsv there. Why I focused on web is because colorpicker returns the same value both times, we push back a hsv which it parses properly

[CrazyIvan359]

In the other direction? Wouldn't that be toHSV?

[mcspr]

It would. Loading webui, it receives hsv value via toHSV. Then, we change it via the wheelcolorpicker to something else, and it sends it back. Receiving side applies it via fromHSV and immediatly reports back via toHSV again, causing colorpicker to update. That's where the rounding issue comes from, since it slightly moves one of the sliders.

[CrazyIvan359]

Oh I see, WebUI gets its values back from toHSV before brightness is applied. Could it be changed to update based on the channels changing and not the command coming in?