On 5/17/22 12:29, Sven Schuchmann wrote:
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?
This construct was introduced to let the new brightness be applied only
upon the next blink, so as to avoid an interference of the blink
sequence.
To fix the issue it could suffice to let led_set_brightness()
be called in multi_intensity_store() only if SW blinking is disabled.
When enabled the new color will be applied upon the next blink anyway,
and the behavior will match that of monochrome LEDs.
--
Best regards,
Jacek Anaszewski
