[BUG] RGB LED switch off when print starts -SKR 1.1 pro, 2nd try

[arminth]

Bug Description

I am using an RGB-strip and a Neopixel ring on my Delta with SKR 1.1 pro.
When I start a print (from Octoprint), LED start showing the correct status from blue to magenta until bed is heated and then from magenta to red until nozzletemp is reached.
When printing starts, neopixel switches to white, RGB strip switches off! I can still switch it on manually.
Another effect is in LED-Control via menu, I can manually switch to R, G, B and all mixed colours but when I choose white, it switches the RGB-lights of. Not the neopixels! To me it looks as if some code was mixed from RGBW-LEDs into RGB but I couldn't really trace it down

My Configurations

configuration.zip

Steps to Reproduce

Start a print.

Expected behavior: All LED switch to white when print starts.

Actual behavior: Neopixel goes white, RGB go out.

Switch to white in LED Menu

Expected behavior: All LED switch to white.

Actual behavior: Neopixel goes white, RGB go out.

Additional Information

This is the second try to report the issue. In my first try #16300, the additional information led to the issue having been taken over into a PIN definition / Feature request thing without even asking if this was what I intended.
I'd kindly ask @boelle, to take care it's not happening again.
And just for good practice: Yes, I have just tried it on 2.0.3. with the new pins assigned as indicated in the old report. Same effect so has nothing to do with pin definitions imho.

Hope somone with knowledge on RGB/LED programming can have a look.

Cheers
Armin

[boelle]

i'll keep my fingers off, i'm only the "janitor" so i dont do code etc
the only reasons for me to close an issue is when its solved and people forget to close
or if we ask the original poster a question and they dont get back to us

[arminth]

@boelle
No offense my friend! It was going into the wrong direction last time because I mentioned the PIN thing... All good! Nice weekend to Denmark!
Cheers
Armin

[boelle]

No offense taken :-) i'm to old for that

[thinkyhead]

The mix of NEOPIXEL and RGB confuses the current LED code because of this:

#define HAS_WHITE_LED EITHER(RGBW_LED, NEOPIXEL_LED)
. . .
#if HAS_WHITE_LED
  #define LEDColorWhite()         LEDColor(  0,   0,   0, 255)
#else
  #define LEDColorWhite()         LEDColor(255, 255, 255)
#endif

To get around the issue we will need to apply the RGBW/NEOPIXEL version of white only to those devices, and the RGB version of white to the other devices.

[thinkyhead]

With your configuration, here's a fix that will work. Change line 36 in leds.h to this:

#define HAS_WHITE_LED ENABLED(RGBW_LED) || (ENABLED(NEOPIXEL_LED) && !NEOPIXEL_IS_RGB)

[arminth]

@thinkyhead
I tried (and changed both files according to your commit).
I get a compiler error:

If I revert, it compiles just fine.
Not quite there yet! :(

[arminth]

I have never learned C++ so beware! But I have found a solution, that works in my configuration. I only don't know, if it also works in a Neopixel GRBW situation.
As far as I understood the Typedef-Struct I have altered it in leds.h as follows:

typedef struct LEDColor {

uint8_t r, g, b, w
// #if HAS_WHITE_LED
// , w

//#endif
#if ENABLED(NEOPIXEL_LED)
    , i
  #endif

;

LEDColor() : r(255), g(255), b(255)
#if HAS_WHITE_LED
  , w(255)
  
#endif 
#if ENABLED(NEOPIXEL_LED)
    , i(NEOPIXEL_BRIGHTNESS)
  #endif

{}

LEDColor(uint8_t r, uint8_t g, uint8_t b, uint8_t w=0

  #if ENABLED(NEOPIXEL_LED)
    , uint8_t i=NEOPIXEL_BRIGHTNESS
  #endif

) : r(r), g(g), b(b)
#if HAS_WHITE_LED
  , w(w)
#endif
#if ENABLED(NEOPIXEL_LED)
   , i(i)
#endif

{}

LEDColor(const uint8_t (&rgbw)[4]) : r(rgbw[0]), g(rgbw[1]), b(rgbw[2])
#if HAS_WHITE_LED
, w(rgbw[3])
#if ENABLED(NEOPIXEL_LED)
, i(NEOPIXEL_BRIGHTNESS)
#endif
#endif
{}

LEDColor& operator=(const uint8_t (&rgbw)[4]) {
r = rgbw[0]; g = rgbw[1]; b = rgbw[2];
#if HAS_WHITE_LED
w = rgbw[3];
#endif
return *this;
}

LEDColor& operator=(const LEDColor &right) {
if (this != &right) memcpy(this, &right, sizeof(LEDColor));
return *this;
}

bool operator==(const LEDColor &right) {
if (this == &right) return true;
return 0 == memcmp(this, &right, sizeof(LEDColor));
}

bool operator!=(const LEDColor &right) { return !operator==(right); }

bool is_off() const {
return 3 > r + g + b
#if HAS_WHITE_LED
+ w
#endif
;
}
} LEDColor;

Main changes:
Always give the struct a w-component.
Pull the #if ENABLED(NEOPIXEL_LED) conditions out of the #if HAS_WHITE_LED conditions to get the intensity portion.

I kindly ask an expert (@thinkyhead ) to doublecheck. Maybe, it helps to get issue fully sorted.

[thinkyhead]

I’ve patched Marlin to deal with it more smartly. When Neopixel is enabled the white channel comes into play. For the moment it will just use RGB white in all cases when colors are being mixed. I’ll audit the code soon to ensure that it’s using the best white color option for each device.

[arminth]

@thinkyhead
Thanks a lot Scott!
I downloaded the latest bugfix with your commit and ran my configuration.
I can confirm, your more smartly dealing is working pefect! :D
Once again, many thanks for so quckly solving this (minor) issue!

Cheers
Armin

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.