Brightness "Wrapping" Turns Segment Off Instead Of Looping To Lowest Brightness

[mattpiper99]

What happened?

I created a preset to change the brightness of segment 1 and tied it to button 1 long press. The API command used in the preset below:

{"seg":{"id":1,"bri":"w~10"}}

When the max brightness is reached (255), the segment is turned OFF instead of wrapping around to the lowest brightness setting.

In json.cpp, it calls...

if (getVal(elem["bri"], &segbri)) {

...which then calls...

parseNumber(str=w~-10, val=255, vmin=0, vmax=255)

...in there it ends up setting "out = 0" (which overwrites segbri to 0 also)... vmin and vmax are default values since nothing is passed into getVal otherwise

Then back in json.cpp...

seg.setOption(SEG_OPTION_ON, segbri);

...gets called with segbri=0, which turns OFF the segment.

Looks like this code dates back to https://github.com/Aircoookie/WLED/commit/a7dbfc49544371a4c7bde0c0c5cb5e5dec243364

I was able to fix this by changing the getVal line to specify a min of 1:

getVal(elem["bri"], &segbri, 1, 255);

...this way it loops brightness back down to 1. I can hold the button and it will cycle 1 to 255, reset back to 1, increment up to 255, reset back to 1, for as long as the button is held. I believe this is the intended behavior of the "wrap" option - but I think this fix might have unintended consequences where a min value of 0 is actually desirable.

So as a different approach, I left getVal alone (to default to 0, 255) and instead threw in a line in parseNumber that if wrapping is enabled, minv gets reset to 1 which accomplishes the same thing as the solution above, hopefully without screwing anything else up that does expect a value of 0... I believe this one line fix is probably the most appropriate solution to the problem, but will defer to the WLED experts...

if (str[0] == 'w' && strlen(str) > 1) {
    str++; wrap = true;
    if(minv <=0 )minv=1;  // prevent 0 from being returned, it will shut off strip and prevent wrap around, set to 1 instead
}

To Reproduce Bug

Make a new segment. Make a new preset with the code: {"seg":{"id":1,"bri":"w~10"}}. Change the segment id in the json if your segment is anything other than #1. Tie the preset to any button long press (maybe not button 0, i think that has special long press stuff). Set the segment brightness to some low value, and then press the button and watch the brightness increase until it reaches max and then the segment shuts off.

After the fix proposed above (to set minv=1), I also tested:

Dim segment to zero (without wrapping), segment turns off as it previously did when it reaches 0. {"seg":{"id":1,"bri":"~-10"}}

Dim segment to min (1) (with wrapping), segment brightness loops to max (255) and allows continuous dimming as desired. {"seg":{"id":1,"bri":"w~-10"}}

Brighten segment to max (255) (with wrapping), segment brightness loops to min (1) and allows continuous dimming as desired. It no longer shuts off the segment in this case. {"seg":{"id":1,"bri":"w~10"}}

Expected Behavior

Per the example in https://kno.wled.ge/interfaces/json-api/

• Increase brightness by 40 wrapping when maximum reached {"bri":"w~40"}

When the segment brightness reaches its max value, it should "wrap around" to its lowest value and allow you to continue increasing the brightness, theoretically in an endless loop.

Install Method

Self-Compiled

What version of WLED?

WLED 0.15.0-b4 (build 2407070)

Which microcontroller/board are you seeing the problem on?

ESP32

Relevant log/trace output

No response

Anything else?

No response

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[blazoncek]

Will need to debug, but logic says:

// assume seg.bri == 250
...
  if (str[0] == 'w' && strlen(str) > 1) {str++; wrap = true;} // wrap -> true
  if (str[0] == '~') {
    int out = atoi(str +1);  // out -> 10
    if (out == 0) {
//      if (str[1] == '0') return;
//      if (str[1] == '-') {
//        *val = (int)(*val -1) < (int)minv ? maxv : min((int)maxv,(*val -1)); //-1, wrap around
//      } else {
//        *val = (int)(*val +1) > (int)maxv ? minv : max((int)minv,(*val +1)); //+1, wrap around
//      }
    } else {
//      if (wrap && *val == maxv && out > 0) out = minv;
//      else if (wrap && *val == minv && out < 0) out = maxv;
//      else {
        out += *val; // out -> 10 + 250 == 260
        if (out > maxv) out = maxv; // out == 255
        if (out < minv) out = minv;
      }
      *val = out;
    }
    return;
}...

with next iteration

  // *val == 255
  if (str[0] == 'w' && strlen(str) > 1) {str++; wrap = true;} // wrap -> true
  if (str[0] == '~') {
    int out = atoi(str +1); // out -> 10
    if (out == 0) {
//      if (str[1] == '0') return;
//      if (str[1] == '-') {
//        *val = (int)(*val -1) < (int)minv ? maxv : min((int)maxv,(*val -1)); //-1, wrap around
//      } else {
//        *val = (int)(*val +1) > (int)maxv ? minv : max((int)minv,(*val +1)); //+1, wrap around
//      }
    } else {
      if (wrap && *val == maxv && out > 0) out = minv; // out -> 0
//      else if (wrap && *val == minv && out < 0) out = maxv;
//      else {
//        out += *val;
//        if (out > maxv) out = maxv;
//        if (out < minv) out = minv;
//      }
      *val = out;
    }
    return;
}...

in next iteration

  // *val == 0
  if (str[0] == 'w' && strlen(str) > 1) {str++; wrap = true;} // wrap -> true
  if (str[0] == '~') {
    int out = atoi(str +1); // out -> 10
//    if (out == 0) {
//      if (str[1] == '0') return;
//      if (str[1] == '-') {
//        *val = (int)(*val -1) < (int)minv ? maxv : min((int)maxv,(*val -1)); //-1, wrap around
//      } else {
//        *val = (int)(*val +1) > (int)maxv ? minv : max((int)minv,(*val +1)); //+1, wrap around
//      }
    } else {
//      if (wrap && *val == maxv && out > 0) out = minv;
//      else if (wrap && *val == minv && out < 0) out = maxv;
      else {
        out += *val; // out -> 10 + 0 == 10
//        if (out > maxv) out = maxv;
//        if (out < minv) out = minv;
      }
      *val = out;
    }
    return;
}...

This seems to work OK. You can try w~ to see what happens when code takes different route. You can also add "on":true in segment option to force segment to on.

[mattpiper99]

Thanks for taking the time to look into this!

In your example above, in the second iteration, when it reaches "max" (255), *val gets set to the "min" (0). Then back in json.cpp it checks...

if (segbri > 0) seg.setOpacity(segbri);

...since segbri=0 (val got set to 0), it doesnt actually change the segment "opacity" value leaving it at 255, and then it proceeds to turn the segment OFF. This means that the third iteration val still gets passed in as 255 (not zero as in your simulation), and so it once again returns 0 and you get stuck in this loop where brightness is maxed out (255) but segment is OFF. It never loops.

By adding "on":true to the preset, when it reaches 255, and returns "0", the brightness handling code shuts off the segment, and then because the "on" handling comes below that in the code, the segment gets turned back on, but it is still at 255 (because it was never updated due to the line of code above). In this case, you still end up stuck at max brightness.

Consider this example:

-assume "w~40" - so incrementing by 40 with wrapping, and segment brightness initially set to "200"

iteration 1: seg.bri = val = 200 out = 40 out += val (=240) set brightness to 240

iteration 2: seg.bri = val = 240 out = 40 out += val (=280) hits: if (out > maxv) out = maxv; // 280 > 255 (max) set brightness to 255 (max)

iteration 3: seg.bri = val = 255 out = 40 hits: if (wrap && val == maxv && out > 0) out = minv; // true && 255 == 255 && 40 > 0 out = 0 (min) do not set brightness (opacity) turn off segment

Any iteration past that you are stuck repeating the same flow as iteration 3

This actually highlights whats potentially the real issue in how the wrapping is being calculated. Theoretically if it did continue on instead of getting stuck, brightness would go from: 200 -> 240 (+40) -> 255 (+15 / stop at max) -> 0 (wrap to min) -> 40 (+40)

But instead of iteration 2 setting the value to the max (255) which is only an increase of 15 (from 240)... Wouldnt it actually be desirable for the wrap to occur at that point, and skip past max? So brightness would go from: 200 -> 240 (+40) -> 25 (+15 to max, wrap to min, +25 = +40) -> 65 (+40)

Perhaps thats overthinking it, and it would require rewriting a bit more of the wrapping calculation code. In real world scenarios, I dont think there would be a noticeable difference in how it wraps between the current calculation and modifying it to skip past max. I'd be fine leaving that part as is, but something still needs to be done about the "0" being returned that prevents the wrap from even occurring.

[blazoncek]

Oh, yes!

if (segbri > 0) seg.setOpacity(segbri);

[blazoncek]

What if you just change the json.cpp to read:

  byte segbri = seg.opacity;
  if (getVal(elem["bri"], &segbri, 1)) {
    seg.setOpacity(segbri);
    seg.setOption(SEG_OPTION_ON, segbri); // use transition
  }

?

[blazoncek]

Feedback would be much appreciated, please.

[mattpiper99]

Changing that getVal call to pass in min=1 was the first "solution" I came up with, but as I noted in my original post I think that might have unintended consequences. If someone is using "bri":"~-1", decrement by 1 without wrap, they might expect that when it reaches 0 for the segment to actually be turned off? Stopping at 1 could result in LEDs still dimly lit when they should be "off".

Per the json api state object description, "Setting bri to 0 is supported but it is recommended to use the range 1-255 and use on: false to turn off". I think thats why when it reaches zero, it sets on=false. Messing with that behavior seems potentially problematic.

I believe probably the most appropriate solution is to leave getVal alone (to default to 0, 255) and instead add a line in parseNumber that minv gets reset to 1, only when wrapping is enabled and min is <=0. This accomplishes the same thing as the getVal solution but only for wrapping mode, and that shouldnt screw anything else up that does expect a value of 0 (like the non-wrapping example above "bri":"~-1").

if (str[0] == 'w' && strlen(str) > 1) {
    str++; wrap = true;
    if(minv <=0 )minv=1;  // prevent 0 from being returned, it will shut off strip and prevent wrap around, set to 1 instead
}

[blazoncek]

Sorry I missed that. Too many things to handle and some slip.

If you think you have the solution (that works in all cases) do open a PR please.

Still, IMO, it would be ok (and would simplify program logic) if API/documentation would be updated to read 1-255 as it is already implemented in such way for global brightness UI and then just limit getVal function. The current implementation for global brightness is still legacy based from versions prior to segments and segment opacity. Perhaps that is the reason for that complicated (and confusing) logic.