Setting multi-color intensities stops software blink

Setting multi-color intensities stops software blink

[Sven Schwermer]

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.

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 :)

Best regards,
Sven

AW: Setting multi-color intensities stops software blink

[Sven Schuchmann]

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?

		}
		return;
	}


Regards,

   Sven

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

[Jacek Anaszewski]

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

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

[Sven Schwermer]

Hi Jacek,

On 5/17/22 23:43, Jacek Anaszewski wrote:
> 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.
> 

That sounds like a good and very simple fix. I'll try it out and if it 
works, I'll post a patch to this list shortly. Thank you!

Best regards,
Sven

[PATCH] led: multicolor: Don't set brightness when blinking

[Sven Schwermer]

From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

When writing to the multi_intensity file, don't unconditionally call
led_set_brightness. By only doing this if blinking is inactive we
prevent blinking from stopping if the blinking is in its off phase while
the file is written.

Instead, if blinking is active, the changed intensity values are applied
upon the next blink. This is consistent with changing the brightness on
monochrome LEDs with active blinking.

Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---
 drivers/leds/led-class-multicolor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
index e317408583df..5b1479b5d32c 100644
--- a/drivers/leds/led-class-multicolor.c
+++ b/drivers/leds/led-class-multicolor.c
@@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device *dev,
 	for (i = 0; i < mcled_cdev->num_colors; i++)
 		mcled_cdev->subled_info[i].intensity = intensity_value[i];
 
-	led_set_brightness(led_cdev, led_cdev->brightness);
+	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
+		led_set_brightness(led_cdev, led_cdev->brightness);
 	ret = size;
 err_out:
 	mutex_unlock(&led_cdev->led_access);

base-commit: 210e04ff768142b96452030c4c2627512b30ad95
-- 
2.36.1

AW: [PATCH] led: multicolor: Don't set brightness when blinking

[Sven Schuchmann]

Hi Sven,

For the patch: tested-by Sven Schuchmann <schuchmann@schleissheimer.de>

Changing the intensity or brightness is possible in on and off state.

Sven

> -----Ursprüngliche Nachricht-----
> Von: Sven Schwermer <sven@svenschwermer.de>
> Gesendet: Mittwoch, 18. Mai 2022 10:22
> An: linux-leds@vger.kernel.org
> Cc: Sven Schwermer <sven.schwermer@disruptive-technologies.com>;
> jacek.anaszewski@gmail.com; Sven Schuchmann <schuchmann@schleissheimer.de>; pavel@ucw.cz
> Betreff: [PATCH] led: multicolor: Don't set brightness when blinking
> 
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> When writing to the multi_intensity file, don't unconditionally call
> led_set_brightness. By only doing this if blinking is inactive we
> prevent blinking from stopping if the blinking is in its off phase while
> the file is written.
> 
> Instead, if blinking is active, the changed intensity values are applied
> upon the next blink. This is consistent with changing the brightness on
> monochrome LEDs with active blinking.
> 
> Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
>  drivers/leds/led-class-multicolor.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
> index e317408583df..5b1479b5d32c 100644
> --- a/drivers/leds/led-class-multicolor.c
> +++ b/drivers/leds/led-class-multicolor.c
> @@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device *dev,
>  	for (i = 0; i < mcled_cdev->num_colors; i++)
>  		mcled_cdev->subled_info[i].intensity = intensity_value[i];
> 
> -	led_set_brightness(led_cdev, led_cdev->brightness);
> +	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
> +		led_set_brightness(led_cdev, led_cdev->brightness);
>  	ret = size;
>  err_out:
>  	mutex_unlock(&led_cdev->led_access);
> 
> base-commit: 210e04ff768142b96452030c4c2627512b30ad95
> --
> 2.36.1

Re: [PATCH] led: multicolor: Don't set brightness when blinking

[Jacek Anaszewski]

Hi Sven,

On 5/18/22 10:22, Sven Schwermer wrote:
> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> 
> When writing to the multi_intensity file, don't unconditionally call
> led_set_brightness. By only doing this if blinking is inactive we
> prevent blinking from stopping if the blinking is in its off phase while
> the file is written.
> 
> Instead, if blinking is active, the changed intensity values are applied
> upon the next blink. This is consistent with changing the brightness on
> monochrome LEDs with active blinking.
> 
> Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
> ---
>   drivers/leds/led-class-multicolor.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
> index e317408583df..5b1479b5d32c 100644
> --- a/drivers/leds/led-class-multicolor.c
> +++ b/drivers/leds/led-class-multicolor.c
> @@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device *dev,
>   	for (i = 0; i < mcled_cdev->num_colors; i++)
>   		mcled_cdev->subled_info[i].intensity = intensity_value[i];
>   
> -	led_set_brightness(led_cdev, led_cdev->brightness);
> +	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
> +		led_set_brightness(led_cdev, led_cdev->brightness);
>   	ret = size;
>   err_out:
>   	mutex_unlock(&led_cdev->led_access);
> 
> base-commit: 210e04ff768142b96452030c4c2627512b30ad95

Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] led: multicolor: Don't set brightness when blinking

[Jacek Anaszewski]

On 5/18/22 21:29, Jacek Anaszewski wrote:
> Hi Sven,
> 
> On 5/18/22 10:22, Sven Schwermer wrote:
>> From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
>>
>> When writing to the multi_intensity file, don't unconditionally call
>> led_set_brightness. By only doing this if blinking is inactive we
>> prevent blinking from stopping if the blinking is in its off phase while
>> the file is written.
>>
>> Instead, if blinking is active, the changed intensity values are applied
>> upon the next blink. This is consistent with changing the brightness on
>> monochrome LEDs with active blinking.
>>
>> Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> Signed-off-by: Sven Schwermer 
>> <sven.schwermer@disruptive-technologies.com>
>> ---
>>   drivers/leds/led-class-multicolor.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/leds/led-class-multicolor.c 
>> b/drivers/leds/led-class-multicolor.c
>> index e317408583df..5b1479b5d32c 100644
>> --- a/drivers/leds/led-class-multicolor.c
>> +++ b/drivers/leds/led-class-multicolor.c
>> @@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device 
>> *dev,
>>       for (i = 0; i < mcled_cdev->num_colors; i++)
>>           mcled_cdev->subled_info[i].intensity = intensity_value[i];
>> -    led_set_brightness(led_cdev, led_cdev->brightness);
>> +    if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
>> +        led_set_brightness(led_cdev, led_cdev->brightness);
>>       ret = size;
>>   err_out:
>>       mutex_unlock(&led_cdev->led_access);
>>
>> base-commit: 210e04ff768142b96452030c4c2627512b30ad95
> 
> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> 

However I'd change patch title because now it is misleading.

How about

"led: multicolor: Fix intensity setting while SW blinking"

-- 
Best regards,
Jacek Anaszewski

[PATCH v2] led: multicolor: Fix intensity setting while SW blinking

[Sven Schwermer]

From: Sven Schwermer <sven.schwermer@disruptive-technologies.com>

When writing to the multi_intensity file, don't unconditionally call
led_set_brightness. By only doing this if blinking is inactive we
prevent blinking from stopping if the blinking is in its off phase while
the file is written.

Instead, if blinking is active, the changed intensity values are applied
upon the next blink. This is consistent with changing the brightness on
monochrome LEDs with active blinking.

Suggested-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Tested-by: Sven Schuchmann <schuchmann@schleissheimer.de>
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Signed-off-by: Sven Schwermer <sven.schwermer@disruptive-technologies.com>
---

Notes:
    V1->V2: Change title, add tags

 drivers/leds/led-class-multicolor.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
index e317408583df..5b1479b5d32c 100644
--- a/drivers/leds/led-class-multicolor.c
+++ b/drivers/leds/led-class-multicolor.c
@@ -59,7 +59,8 @@ static ssize_t multi_intensity_store(struct device *dev,
 	for (i = 0; i < mcled_cdev->num_colors; i++)
 		mcled_cdev->subled_info[i].intensity = intensity_value[i];
 
-	led_set_brightness(led_cdev, led_cdev->brightness);
+	if (!test_bit(LED_BLINK_SW, &led_cdev->work_flags))
+		led_set_brightness(led_cdev, led_cdev->brightness);
 	ret = size;
 err_out:
 	mutex_unlock(&led_cdev->led_access);

base-commit: 210e04ff768142b96452030c4c2627512b30ad95
-- 
2.36.1