Hi Sven,
> -----Ursprüngliche Nachricht-----
> Von: Sven Schwermer <sven@xxxxxxxxxxxxxxxx>
> Gesendet: Dienstag, 3. Mai 2022 14:17
> An: linux-leds@xxxxxxxxxxxxxxx
> Cc: pavel@xxxxxx
> Betreff: Setting multi-color intensities stops software blink
>
> Hi,
>
> I'm experiencing an issue with multi-color LEDs when setting the
> intensities while software blinking is active (e.g. trigger=timer). This
> manifests itself by delay_on and delay_off being set to 0 when writing
> multi_intensities while the LED is off. If doing this while the LED is
> on, everything works as expected.
At least I can see the same thing. Setup a led like this:
echo 255 0 0 | sudo tee /sys/class/leds/rgb\:fbx-led-1/multi_intensity
echo 100 | sudo tee /sys/class/leds/rgb\:fbx-led-1/brightness
echo "timer" | sudo tee /sys/class/leds/rgb\:fbx-led-1/trigger
echo 5000 | sudo tee /sys/class/leds/rgb\:fbx-led-1/delay_on
echo 5000 | sudo tee /sys/class/leds/rgb\:fbx-led-1/delay_off
Change the color afterwards while the LED is in the "on" cycle
echo 0 255 0 | sudo tee /sys/class/leds/rgb\:fbx-led-1/multi_intensity
no problem, on the next "on" cycle the led has the new intensity
Change the color afterwards while the LED is in the "off" cycle
echo 0 255 0 | sudo tee /sys/class/leds/rgb\:fbx-led-1/multi_intensity
the led stays off because delay_on and delay_off is 0.
This also looks like incorrect behavior to me.
> I suspect that this happens because multi_intensity_store() calls
> led_set_brightness(led_cdev, led_cdev->brightness) at the end. It seems
> like the software blinking modifies led_cdev->brightness directly, so if
> the LED is in its off-phase, we're effectively switching the LED off
> because we're setting its brightness to 0 which clears delay_on and
> delay_off to 0:
>
> led_set_brightness(brightness=0): sets LED_BLINK_DISABLE
> -> set_brightness_delayed()
> -> led_stop_software_blink(): clears blink delays
>
> How would one fix this properly? Should multi_intensity_store() call
> led_set_brightness() with brightness=led_cdev->blink_brightness if
> blinking is active? That feels like an unclean solution.
>
> I'm hoping for some input. Thanks :)
To me this looks wrong:
void led_set_brightness(struct led_classdev *led_cdev, unsigned int brightness)
{
/*
* If software blink is active, delay brightness setting
* until the next timer tick.
*/
if (test_bit(LED_BLINK_SW, &led_cdev->work_flags)) {
/*
* If we need to disable soft blinking delegate this to the
* work queue task to avoid problems in case we are called
* from hard irq context.
*/
if (!brightness) {
set_bit(LED_BLINK_DISABLE, &led_cdev->work_flags);
I think it is wrong to ask for the brightness over here when we know
that the led is blinking and could be currently in it's off cycle.
schedule_work(&led_cdev->set_brightness_work);
} else {
set_bit(LED_BLINK_BRIGHTNESS_CHANGE,
&led_cdev->work_flags);
led_cdev->new_blink_brightness = brightness;
I think the else path could always be done regardless of the brightness?
}
return;
}
Regards,
Sven
