Build warnings from PR #1902 (300-level)

Issues
1

On first glance, most people would say that GCC is simply wrong. However, the problem does appear to be real. [[ and I’m not talking about the problems inherent with floating point math (NaN, +infinity, -infinity, etc.) ]]

PR 1902 resulted in additional build warnings. 
wled00/colors.cpp: In function 'void colorHSVtoRGB(float, float, float, byte&, byte&, byte&)':
wled00/colors.cpp:78:33: warning: 'b' may be used uninitialized in this function [-Wmaybe-uninitialized]
   blue = static_cast<uint8_t>(b * 255);
                                 ^
wled00/colors.cpp:77:34: warning: 'g' may be used uninitialized in this function [-Wmaybe-uninitialized]
   green = static_cast<uint8_t>(g * 255);
                                  ^
wled00/colors.cpp:76:32: warning: 'r' may be used uninitialized in this function [-Wmaybe-uninitialized]
   red = static_cast<uint8_t>(r * 255);
Here's the relevant code 
void colorHSVtoRGB(float hue, float saturation, float value, byte& red, byte& green, byte& blue)
{
  float r, g, b;

  auto i = static_cast<int>(hue * 6);
  auto f = hue * 6 - i;
  auto p = value * (1 - saturation);
  auto q = value * (1 - f * saturation);
  auto t = value * (1 - (1 - f) * saturation);

  switch (i % 6)
  {
  case 0: r = value, g = t, b = p; break;
  case 1: r = q, g = value, b = p; break;
  case 2: r = p, g = value, b = t; break;
  case 3: r = p, g = q, b = value; break;
  case 4: r = t, g = p, b = value; break;
  case 5: r = value, g = p, b = q; break;
  }

  red = static_cast<uint8_t>(r * 255);
  green = static_cast<uint8_t>(g * 255);
  blue = static_cast<uint8_t>(b * 255);
}
The remainder operator (%) can return negative values

See modulo - Modulus with negative numbers in C++ - Stack Overflow.

Summarizing, the local variable i is of type int and thus can contain negative values. The remainder operator (%) has implementation-defined behavior (until C++20). The gnu compiler returns the remainder (rounding towards zero). Thus, i % 6 can return negative values, none of which are handled in the switch statement.

See sample code online.

I first looked for a minimal fix:

  • Make i an unsigned integer?
  • Perform the remainder operation twice? [[ (((i % 6) + 6) % 6) ]]
  • Handle the cases of i % 6 being in { -5, -4, -3, -2, -1 }?
  • Both unsigned integer and remainder twice?

Unfortunately, none of the above resolved the GCC warning that this could be using uninitialized variables. So, what’s the right fix?

1 Like
2

To me it looks like someone with good knowledge of C++ syntax wrote the code but did not apply much semantics. :slight_smile:
There is really no need for C++ ninjitsu in this code (static_casts).

This should fix compile warning as well as bugs.

void colorHSVtoRGB(float hue, float saturation, float value, byte& red, byte& green, byte& blue)
{
  float r = float(red)/255, g  = float(green)/255, b = float(blue)/255;

  uint8_t i = hue * 6;
  float f = hue * 6 - i;
  float p = value * (1 - saturation);
  float q = value * (1 - f * saturation);
  float t = value * (1 - (1 - f) * saturation);

  switch (i % 6) {
    case 0: r = value, g = t, b = p; break;
    case 1: r = q, g = value, b = p; break;
    case 2: r = p, g = value, b = t; break;
    case 3: r = p, g = q, b = value; break;
    case 4: r = t, g = p, b = value; break;
    case 5: r = value, g = p, b = q; break;
  }

  red = r * 255;
  green = g * 255;
  blue = b * 255;
}

You can even shorten the first line to:
float r = red/255.0, g = green/255.0, b = blue/255.0;

3

There is no need to init r,g,b to red,green,blue, they are overwritten in the switch anyways. We can just init these to 0 to get rid of Wmaybe-uninitialized.

Otherwise I agree that the casts are probably not needed, though I’d change it to hue * 6.0f to ensure it does a float and not int multiplication.

Oh and welcome to the forum btw :smiley:

1 Like
4

Jokes aside, if for some obscure reason i%6 indeed doesn’t evaluate to [0,5] then it is best to leave red, green and blue in the same state they were when calling the function.
That’s why assignment.

6

Can it be optimized even more?

oid colorHSVtoRGB(float hue, float saturation, float value, byte& red, byte& green, byte& blue)
{
  float r = red, g  = green, b = blue;

  uint8_t i = hue * 6.0;
  float f = hue * 6.0 - i;
  value *= 255.0;
  float p = value * (1.0 - saturation);
  float q = value * (1.0 - f * saturation);
  float t = value * (1.0 - (1.0 - f) * saturation);

  switch (i % 6) {
    case 0: r = value, g = t, b = p; break;
    case 1: r = q, g = value, b = p; break;
    case 2: r = p, g = value, b = t; break;
    case 3: r = p, g = q, b = value; break;
    case 4: r = t, g = p, b = value; break;
    case 5: r = value, g = p, b = q; break;
  }

  red = r;
  green = g;
  blue = b;
}
7

Perhaps like this?

void colorHSVtoRGB(float hue, float saturation, float value, byte& r, byte& g, byte& b)
{
  uint8_t i = hue * 6.0;
  float f = hue * 6.0 - i;
  value *= 255.0;
  float p = value * (1.0 - saturation);
  float q = value * (1.0 - f * saturation);
  float t = value * (1.0 - (1.0 - f) * saturation);

  switch (i % 6) {
    case 0: r = value, g = t,     b = p;     break;
    case 1: r = q,     g = value, b = p;     break;
    case 2: r = p,     g = value, b = t;     break;
    case 3: r = p,     g = q,     b = value; break;
    case 4: r = t,     g = p,     b = value; break;
    case 5: r = value, g = p,     b = q;     break;
  }
}
8

Looking deeper, I have some questions on this PR, which make me wonder if there are better options…

This PR adds functionality to modify RGB data received via any of serial, e1.31 and udp, aka “realtime” data. Specifically, it allows (in 0.1 increments) to multiply, in the HSV representation of the RGB values, the saturation and value components.

Three global variables controlling behavior 
WLED_GLOBAL bool liveHSVCorrection _INIT(false);
WLED_GLOBAL uint16_t liveHSVSaturation _INIT(13);
WLED_GLOBAL uint16_t liveHSVValue _INIT(10);
My review of code flow
  • setRealtimePixel() is called when pixel data is received via serial, udp, and e1.31
  • when liveHSVCorrection is true, this calls correctColors()
    • This passes r, g, b as bytes, and a pointer to a local three-byte array for results
    • That local three-byte array then immediately overwrites r, g, and b :thinking:
  • correctColors() starts execution of substantially all new code…
    1. correctColors() first calls colorRGBtoHSV()
      • This is a new function, using floating point, rather than using FastLED routines?
      • Inputs are bytes for r, g, b
      • Output is three floats for h, s, v
    2. it then determines a value for saturated:
      • Checks if s is greater than 0.1
      • If true, then saturated is set to s / 10 (???)
      • Else, saturated is simply set equal to s (???)
    3. it then sets local variable saturation = min(saturated, 1)
      • OK, it’s not nearly as clean in the code, but that’s what it’s doing…
    4. it then sets local variable valued = min(v,1)
    5. finally, it calls colorHSVtoRGB()
      • Here, the original s and v are replaced with the two local variables…
  • colorsHSVtoRGB() has not comments suggesting it’s doing anything unusal, so presumed to be a straightforward “RAW” conversion
    • See FastLED functions in hsv2rgb.h for alternatives

Broader Questions from code review:

  1. What’s a more explicit wording of the intent here?
    a. relatedly, should correctColors() be renamed to more clearly indicate what aspect this function is actually adjusting?
    b. strawman: adjustLedVividity() (note: I made Vividity up, cause it’s fun to pronounce)
  2. It appears saturated has a non-smooth transition, near s == 0.1 … is this intentional, or just needed more testing/review?
  3. liveHSVSaturation config value … should this be a float parameter, not uint16_t?
  4. Can some (or all) of this use integer math, maybe using existing FastLED routines?
    a. for example, rgb2hsv_approximate() and hsv2rgb_raw()
    b. Neither ESP8266 nor ESP32-S2 have hardware FPU
    c. ESP32 doesn’t support hardware floating point division in hardware(!). This may explain why double may be faster than single-precision on the ESP32
  5. Would compiler optimizations improve if correctColors() defined to pass R,G,B as references?
9

@Aircoookie – If you are OK with me doing so, since FP math has zero hardware support on ESP32-S2 and ESP8266 (and FP division has none for ESP32), I’d like to rework these functions to use fixed-point uint32_t-based math (maybe 64k).

Note: I will write test code to verify that results are consistent with (within 1 for each of R, G, B) the current floating point based implementation, and ensure valid math (no overflow, no underflow) for entire range of inputs.

10

By all means feel free to do so! The fact that it uses so much floating point math held me back on merging #1902 for a long time, but in the end I did so since it is an optional feature multiple users requested.

Warning, non-scientific wording: floating point and the Arduino String class are cardinal sins

I agree on renaming to adjustVividity() or adjustLiveColors(), this is not as much of a “correction” as an adjustment to individual preference or a specific setup (e.g. TV backlighting might appear darker than wished for by the user)

Using fastLED routines sounds good. If they are functionally equivalent, the current re-implementation is inefficient in terms of code size and possibly performance.

I cannot answer this. Will notify the contributor of #1902 to this thread, maybe they can help us out with what the best intent would be.

11

Reviewing the code I got the impression that non-linearity of saturation and value is intended at low values to improve LED output.
In any case it is optional feature a few requested so I wouldn’t pour too much time into it.
And as @Aircoookie said it may be best to get information from original contributor.

12

Hi all,

Thank you for the review :slight_smile: , here are some replies to some of the queries above:

During my testing of this algorithm, I had to bump up colors that were very faint, to a more vivid saturated color, that would explain the odd bumps and non-linearity of the code.

I’m not familiar with FastLED routines, do you think would these improve the algorithm runtime? If so, I’m all for this as well, this would definitely reduce latency of the color correction, which would make it smoother and more “realtime”.

I’ve opted for int for easier calculations and avoiding floating point math (faster calculations), do you have a strong reason against this decision? I’m not married to that decision, open to improvement.

Happy to collaborate here, shall we create a GitHub issue collecting proposed improvements to this feature?

+1 for Vividity :slight_smile:

13

@ahmed-shehata , welcome to the discourse!

First, I want to say that you tackled a really difficult problem, and had great results for your setup. Well done.

At the same time, while it met your needs and was great for your particular strips and setup, this implementation appears to have been more properly configured as a user module.

Better as a user module

In summary, for mainline, the problem that needs to be solved is a method of applying color correction curve(s) to a given LED strip.

The solution provided essentially provides a linear scaling, and then clamps overly-large values to max. Thus, as an example, if saturation results in effectively ~1.5x the value of the primary color, all values from ~180 or higher are treated as 255 (full bright).

The solution provided does not allow for distinct correction curves for R, G, and B. It’s also not clear what type of HSV to RGB conversion is preferred (spectra vs. rainbow)

Therefore, as anything in the mainline essentially remains supported forever, because this isn’t quite complex enough, I’m recommending moving it to a user module.

Confused by your responses ...

Your comment left me confused… I must be missing something:

  1. Both colorHSVtoRGB() and colorRGBtoHSV() appear to be floating point heavy
  2. The config values are stored as uint16_t, but then converted to float in correctColors()
  3. Both the above occur for each LED, for each update (e.g., 40fps * number of LEDs).

Wouldn’t the odd bumps and non-linearity only be applicable to your particular LED strips? In other words, would those values be the same for other manufacturers (or even manufacturing runs)?

If I did understand it correctly, then it would likely benefit from conversion to fixed-point math…

Your algorithm can convert to fixed-point math... but....

First, it’s absolutely possible to convert your floating point algorithms to fixed-point math. In fact, I did this in under a day, using CNL library v1.5. See Compiler Explorer for both the converted code and the sampled test points. The new code was considered valid when each of the R,G,B values were +/-1 of the original code.

However, CNL does not appear to be available in PlatformIO, so integration this into WLED would be significant work.

Based on the above, I am strongly recommending the following:

  1. Immediately revert the PR, to avoid configurations (and perpetual support for) a partial fix for color-correction.
  2. Discuss hooks that would be needed to apply gamma correction, brightness correction, and other color correction
  3. Find out how to add CNL 1.x library to platformio. :slight_smile:
  4. Use the hooks from #2 to enable your usermod
  5. If #3 works, convert your usermod to fixed-point calculations

Does that sound reasonable to you?

14

Updates:

#3 and #5 – ~50% performance improvement achieved by using fixed-point math.

Summary of perf test details

Results:

Prior code (floating point) resulted in 112 loops/second.
Fixed-point version resulted in 142 loops/sec.

Setup:

The perf test simulated the correction of 1000 LEDs, by manually inserting 1000 calls to correctColor() in the main loop. The perf test was run on an ESP32 (WT32-ETH01). The ESP32 was chosen as a “worst case” improvement scenario, as it has a hardware FPU (but only for multiplication). Neither the ESP8266 nor ESP32-S2 have any hardware FPU. The test was run on a silent network (no ethernet traffic, no wifi traffic, no other services), to get stable numbers across loops.

For performance-sensitive code (such as that which updates each and every LED), this appears worth the code complexity.

but not ready for PR...

It’s has a nasty hack at the moment, since Espressif didn’t include to_string() functions, but the perf increase shows that use of fixed-point maths is well worth the effort. Thus, I declared those functions just to get this to compile… a better solution is obviously needed for the to_string().

15

In a perfect world, the WLED smartphone app could be used to automate color correction. With a tap of a button, step-by-step it would activate the phone’s camera, send red, green, and then blue color commands with SOLID effect, then step through the intensity values to map the actual color response from camera input.

After calculating adjustment mapping, send to ESP, and then run through a validation test where the color mapping values are tested to verify proper performance.

Kind of like Pantone color mapping a desktop publishing monitor and printer to get exact color output, but for inexpensive LED strips (where color mapping is likely to help the most).

Is there already an app for that?

16

Using floating point is justified when you need high precision otherwise it may be good to accept rounding/truncating errors and only use integer math by multiplying original values by 1000 (or 10000) and then returning result divided by 1000 (or 10000).
This is assuming HSV values are between 0 and 1 and we can accept rounding to 3rd fractional digit.

What do you think?

(Thinking inspired by lack of knowledge of C++ namespaces.)

17

Thank you for your kind words and support reviewing this :slight_smile:

I actually did not know about this feature, this would make a lot of sense, fully agree to move it as User Module, I’m also happy to support and maintain this module down the road if it gains traction.

If you mean the customizable Value and Saturation values, then no. It is up to the user to customize and tweak up to their needs and specific taste.

That said, we should actually adhere to a specific range of these values, right now it feels arbitrary (1-30?)

Are these settings applied to all LEDs in the setup? For example if I have 2 LED strips connected in series, one from manufacturer A and other B, would these settings apply to both equally? If so, then yes, we do need to split since the values would differ across different LED strips.

Ah yes, I misunderstood your comment earlier, the reason why the config values are int was to make it easier for the user to try out different values (whole number is easier to remember).

Given your demo here, how difficult would it be to implement a basic to_string()? Can we somehow use a header library that provides such extension?

Camera input can be very tricky and inaccurate, since it can be affected by external light sources, it would not be accurate. I like the automation bit though, we can definitely improve this by having some sort of color matching palette where, for example: the user would get to specify the low/high for each of the R/G/B colors on the spectrum. Think of it as “calibrate your input against your LED”.

I agree with this, high precision is not needed in this feature, colors can be rounded or approximated as long as they achieve similar levels of “saturation boosts”. This was mainly my next step in improving this (turning it into completely int-based).

2 Likes
18

OK, user module seems like a good choice for initial development, and having it as such helps with long-term stability, esp. as it’s initially floating-point heavy.

  1. I have provided a (draft) PR that reverts PR 1920.
  2. Aircookie and Blazoncek would likely want input in how user modules hook into adjusting real-time data. I have a simple way (weak references), that enforces no more than a single user module hook at compilation time, but maybe they’d want to allow chaining?
  3. @ahmed-shehata, you’d need to create the PR with this as a usermod.

Sound good?
If so, I’ll convert the draft PR to non-draft as the first step…

BTW, I think it’s great to have this functionality eventually integrated. It sounds like this UM could even grow to support arbitrary adjustment curves. Very exciting!

19

FYI, my unit tests show that the individual R, G, B values returned differed by no more than ‘1’. So, precision from a single RGB → HSV → adjustment → RGB cycle is sufficiently accurate when using the fixed-point version. :partying_face:

20

Having this functionality as a usermod would make perfect sense! I myself want to pull some functionality from the core and make them available as user module, first and foremost Cronixie, but maybe also Blynk in the future.

Giving usermod(s) the ability to hook into realtime data would be an excellent addition.
I have performance reservations about a for loop iterating over every usermod in every call to setRealtimePixel(), but allowing a usermod setting a callback function could be an easy way to solve it, albeit it would take some extra looking into to support multiple usermod chaining.

A similar method could allow usermods to access handleOverlayDraw() to change arbitrary pixels in every frame. This is not as performance critical though as it is only called once per frame, not per pixel.

2 Likes
21

The PR was reverted. I am now working on moving that code back into a usermod, which I’m placing under the following directory:

usermods/usermod_v2_adjust_realtime_data

After I get it compiling (without hooking the realtime data), I will work to get the necessary hooks into Blazoncek’s fork’s dev branch.

Once that’s done, then will start the discussion of what hooks are needed to make this work well, and limit future re-architecting.