Re: AW: Setting multi-color intensities stops software blink

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 5/17/22 12:29, Sven Schuchmann wrote:
> Hi Sven,
> 
>> -----Ursprüngliche Nachricht-----
>> Von: Sven Schwermer <sven@svenschwermer.de>
>> Gesendet: Dienstag, 3. Mai 2022 14:17
>> An: linux-leds@vger.kernel.org
>> Cc: pavel@ucw.cz
>> 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