Hi and thanks for the PR. We're just starting to work on the backlog of contributions, and I just wanted to let you know that I was taking a look at this now. I really appreciate the help.

Thank you a lot, man.

Fixed my jumping red led with:

            if( r ) r = scale8_LEAVING_R1_DIRTY( r, val) + 1;
            asm volatile("");
            if( g ) g = scale8_LEAVING_R1_DIRTY( g, val) + 1;
            asm volatile("");
            if( b ) b = scale8_LEAVING_R1_DIRTY( b, val) + 1;
            asm volatile("");

in all places in hsv2rgb_rainbow() where the scale8_LEAVING_R1_DIRTY appeared...

Haven't tested it in other cases but mine, don't exactly understand the issue and don't know if it changes the execution time (on AVRs, it shouldn't, right?). But I do understand the lower saturation in fill_rainbow() was there for a purpose and I do like the uniform brightness it enables.

I'm not in favor of changing the saturation of the rainbow at this time. Too risky.

The asm noop fix is very interesting. But the AVR chipsets are becoming less relevant each year. If someone has a new PR with the ASM changes (the comment above is not definitive) then I'll accept it.

Hi there, I'm looking at this again and would like to get this old PR fix in. Overall it looks good and I have absolutely noticed bad color with the hue algorithm in many tests.

What I'm noticing here is that you missed a few places. Wanted to give you a chance to review before I proceed.

In src/colorutils.cpp I notice that there are four rainbow functions that each set the set hsv.sat = 240

Here is what I currently see with ripgrep:

1

void fill_rainbow( struct CRGB * targetArray, int numToFill,
                  uint8_t initialhue,
                  uint8_t deltahue )
{
    CHSV hsv;
    hsv.hue = initialhue;
    hsv.val = 255;
    hsv.sat = 240;
    for( int i = 0; i < numToFill; ++i) {
        targetArray[i] = hsv;
        hsv.hue += deltahue;
    }
}

2

void fill_rainbow( struct CHSV * targetArray, int numToFill,
                  uint8_t initialhue,
                  uint8_t deltahue )
{
    CHSV hsv;
    hsv.hue = initialhue;
    hsv.val = 255;
    hsv.sat = 240;
    for( int i = 0; i < numToFill; ++i) {
        targetArray[i] = hsv;
        hsv.hue += deltahue;
    }
}

3

void fill_rainbow_circular(struct CRGB* targetArray, int numToFill, uint8_t initialhue, bool reversed)
{
    if (numToFill == 0) return;  // avoiding div/0

    CHSV hsv;
    hsv.hue = initialhue;
    hsv.val = 255;
    hsv.sat = 240;

    const uint16_t hueChange = 65535 / (uint16_t)numToFill;  // hue change for each LED, * 256 for precision (256 * 256 - 1)
    uint16_t hueOffset = 0;  // offset for hue value, with precision (*256)

    for (int i = 0; i < numToFill; ++i) {
        targetArray[i] = hsv;
        if (reversed) hueOffset -= hueChange;
        else hueOffset += hueChange;
        hsv.hue = initialhue + (uint8_t)(hueOffset >> 8);  // assign new hue with precise offset (as 8-bit)
    }
}

4

void fill_rainbow_circular(struct CHSV* targetArray, int numToFill, uint8_t initialhue, bool reversed)
{
    if (numToFill == 0) return;  // avoiding div/0

    CHSV hsv;
    hsv.hue = initialhue;
    hsv.val = 255;
    hsv.sat = 240;

    const uint16_t hueChange = 65535 / (uint16_t) numToFill;  // hue change for each LED, * 256 for precision (256 * 256 - 1)
    uint16_t hueOffset = 0;  // offset for hue value, with precision (*256)

    for (int i = 0; i < numToFill; ++i) {
        targetArray[i] = hsv;
        if (reversed) hueOffset -= hueChange;
        else hueOffset += hueChange;
        hsv.hue = initialhue + (uint8_t)(hueOffset >> 8);  // assign new hue with precise offset (as 8-bit)
    }
}

@zackees can we please get the suggested changes for AVR at the same time. Iirc, this value of 240 for saturation was reduced from 255 to work around a very old, and previously misunderstood issue on AVR. Since then, I've seen it manifest in my own uses of scale8_LEAVING_R1_DIRTY. The proposed volatile asm(""); additions should fix the original problem and allow saturations up to 255 to work correctly on AVR too. They serve as sequence points, so the optimiser cannot reorder the asm()s.

Fixed my jumping red led with:

            if( r ) r = scale8_LEAVING_R1_DIRTY( r, val) + 1;
            asm volatile("");
            if( g ) g = scale8_LEAVING_R1_DIRTY( g, val) + 1;
            asm volatile("");
            if( b ) b = scale8_LEAVING_R1_DIRTY( b, val) + 1;
            asm volatile("");

in all places in hsv2rgb_rainbow() where the scale8_LEAVING_R1_DIRTY appeared...

Haven't tested it in other cases but mine, don't exactly understand the issue and don't know if it changes the execution time (on AVRs, it shouldn't, right?). But I do understand the lower saturation in fill_rainbow() was there for a purpose and I do like the uniform brightness it enables.

Thanks @sutaburosu

Here is the new pr that fixes this: #1907

I'll leave feedback open for 48 hours, unless someone gives me a LGTM before.

@sutaburosu okay the asm("") fix is in. Am I correct that this means this issue is now fixed and can be closed?

I'm sorry, I haven't had time in the last couple of days to put together the visual test case that I wanted to provide. I tried briefly, but this problem can be difficult to reproduce when you actually want to see it.

After skimming the linked issues, I notice this commentor reached the same conclusion: prevent the compiler re-ordering anything in this stanza.

My vote is to close this and the others.

ok it's fixed. Will reopen if not fixed