[BUG] RGB LED / LED Strip Control - Missing an resolution set flag + proposed solution

[happy77fr]

Bug Description

I am compiling Marlin for a STM32F7 board. I am trying to add a led strip for better lighting of the print. When I activate the feature, the led strip is, at best, at half brightness. After a bit of digging, I discovered that it is missing an option to set resolution such as: analogWriteResolution(8)

the bit of code is of the leds.cpp file:

 #if EITHER(RGB_LED, RGBW_LED)
    // This variant uses 3-4 separate pins for the RGB(W) components.
    // If the pins can do PWM then their intensity will be set.
    #define UPDATE_RGBW(C,c) do {                       \
      if (PWM_PIN(RGB_LED_##C##_PIN)) {                   \
        analogWrite(pin_t(RGB_LED_##C##_PIN), incol.c); \
      else                                              \
        WRITE(RGB_LED_##C##_PIN, incol.c ? HIGH : LOW); \
    }while(0)
    UPDATE_RGBW(R,r); UPDATE_RGBW(G,g); UPDATE_RGBW(B,b);
    #if ENABLED(RGBW_LED)
      UPDATE_RGBW(W,w);
    #endif
 #endif

As I don't fully understand this piece of code, I am not quite sure where to put "analogWriteResolution(8);" Any recommandation?

Would it be possible to set the resolution in the marlin FW for this feature?

My Configurations

Marlin.zip Required: Please include a ZIP file containing your Configuration.h and Configuration_adv.h files.

Steps to Reproduce

Run the software normally.

Additional Information

On solution would be to place somewhere analogWriteResolution(PWM_RESOLUTION); with PWM_RESOLUTION set in the configuration.h The only thing that I am worried it is the ability to have it different that the one for the motors.....

[happy77fr]

After some tests, I would like to propose the following solution:

in configuration.h

/**
 * RGB LED / LED Strip Control
 *
 * Enable support for an RGB LED connected to 5V digital pins, or
 * an RGB Strip connected to MOSFETs controlled by digital pins.
 *
 * Adds the M150 command to set the LED (or LED strip) color.
 * If pins are PWM capable (e.g., 4, 5, 6, 11) then a range of
 * luminance values can be set from 0 to 255.
 * For NeoPixel LED an overall brightness parameter is also available.
 *
 * *** CAUTION ***
 *  LED Strips require a MOSFET Chip between PWM lines and LEDs,
 *  as the Arduino cannot handle the current the LEDs will require.
 *  Failure to follow this precaution can destroy your Arduino!
 *  NOTE: A separate 5V power supply is required! The NeoPixel LED needs
 *  more current than the Arduino 5V linear regulator can produce.
 * *** CAUTION ***
 *
 * LED Type. Enable only one of the following two options.
 *
 */
//#define RGB_LED
#define RGBW_LED

#if EITHER(RGB_LED, RGBW_LED)
  // Set PWM to a resolution of 4, 8, 12, 16,....
  //#define RGBW_LED_PWM_RES 8

  #define RGB_LED_R_PIN 5
  #define RGB_LED_G_PIN 6
  #define RGB_LED_B_PIN 7
  #define RGB_LED_W_PIN 8
#endif

in leds.cpp

void LEDLights::setup() {
  #if EITHER(RGB_LED, RGBW_LED)
    if (PWM_PIN(RGB_LED_R_PIN)) SET_PWM(RGB_LED_R_PIN); else SET_OUTPUT(RGB_LED_R_PIN);
    if (PWM_PIN(RGB_LED_G_PIN)) SET_PWM(RGB_LED_G_PIN); else SET_OUTPUT(RGB_LED_G_PIN);
    if (PWM_PIN(RGB_LED_B_PIN)) SET_PWM(RGB_LED_B_PIN); else SET_OUTPUT(RGB_LED_B_PIN);
    #if ENABLED(RGBW_LED)
      if (PWM_PIN(RGB_LED_W_PIN)) SET_PWM(RGB_LED_W_PIN); else SET_OUTPUT(RGB_LED_W_PIN);
    #endif
    #ifdef RGBW_LED_PWM_RES
        analogWriteResolution(RGBW_LED_PWM_RES);
    #endif
  #endif
  TERN_(NEOPIXEL_LED, neo.init());
  TERN_(PCA9533, PCA9533_init());
  TERN_(LED_USER_PRESET_STARTUP, set_default());
}

What do you think?

[thisiskeithb]

If you've tested it and it works, putting in a PR will likely generate more conversation.

Edit: I also added the Feature Request label since it's not really a bug as the ability to set RGB LED PWM doesn't exist in Marlin.

[AnHardt]

What do you think?

analogWriteResolution() is not available on all platforms - for example not on the 8bit AVRs.

By default the default resolution is 8 bit on all platforms. If not, that's an error in the platforms implementation of analogWrite() and needs to be fixed there.

Using analogWrite() in Marlin is generally not a good idea - it occupies timers intransparently.

[happy77fr]

analogwrite is in use in the leds.cpp file in the setup function LEDLights::setup() as I mentioned in my initial post. @AnHardt Which function do you think would be better then than analogwrite()?

Alternatively, instead of changing the resolution, a scaling factor could be introduced to compensate any resolution issues.

[AnHardt]

What do you think?

Your question includes my opinion about your code changes, but is not exclusive to that.

My replies first paragraph was about your code change and why a 'analogWriteResolution()' needs to be restricted to the platforms it is valid for.

The second paragraph was a suggestion for where might be a better place for the fix. (Platform specific HAL or platform, where the platform restriction is done automatically by its location)

The third paragraph is a very general opinion about analogWrite(). Over the years, having seen dozens of issues caused by unawareness of timer usage by analogWrite() i'm frustrated by that, and wanted to tell. Always when i see a analogWrite() in Marlin i'm thinking about that. Just rarely the question is that way i can tell you about that. :-)

For the processors able to run on Arduino_Core_STM32 i like the Timer-API - at least it gives the total awareness of what timers are in use. It's a bit more complicated to work with, but that actually is the Arduino deal. "Make it simple" means - pay with flexibility, speed, code-size or any combination of that.