Strange timer LED behavior

Strange timer LED behavior

[Bollinger, Seth]

Hello All,

We use the sysfs timer trigger of the led subsystem to control our leds (turn on, turn off, and blink).  While doing this we found a strange behavior.  If the values of delay_on and delay_off are ever both zero, the led subsystem will make the decision to blink at a 500ms on and 500ms off rate.

# cd /sys/class/leds/g01
# cat delay_on
0
# cat delay_off
86400
# echo 0 > delay_off
# cat delay_on
500
# cat delay_off
500

drivers/led/led-core.c:184
    /* blink with 1 Hz as default if nothing specified */
    if (!*delay_on && !*delay_off)
        *delay_on = *delay_off = 500;

Is there a reason that the kernel would decide to blink at this default rate instead of leaving the timer disabled?

Thanks,

Seth

Re: Strange timer LED behavior

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

Hi!

> We use the sysfs timer trigger of the led subsystem to control our leds (turn on, turn off, and blink).  While doing this we found a strange behavior.  If the values of delay_on and delay_off are ever both zero, the led subsystem will make the decision to blink at a 500ms on and 500ms off rate.
> 
> # cd /sys/class/leds/g01
> # cat delay_on
> 0
> # cat delay_off
> 86400
> # echo 0 > delay_off
> # cat delay_on
> 500
> # cat delay_off
> 500
> 
> drivers/led/led-core.c:184
>     /* blink with 1 Hz as default if nothing specified */
>     if (!*delay_on && !*delay_off)
>         *delay_on = *delay_off = 500;
> 
> Is there a reason that the kernel would decide to blink at this default rate instead of leaving the timer disabled?
>

Yep. It allows hardware to provide different values if it can only
blink at fixed rate.
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Strange timer LED behavior

[Bollinger, Seth]

> 
> Hi!
> 
>> We use the sysfs timer trigger of the led subsystem to control our leds (turn on, turn off, and blink).  While doing this we found a strange behavior.  If the values of delay_on and delay_off are ever both zero, the led subsystem will make the decision to blink at a 500ms on and 500ms off rate.
>> 
>> # cd /sys/class/leds/g01
>> # cat delay_on
>> 0
>> # cat delay_off
>> 86400
>> # echo 0 > delay_off
>> # cat delay_on
>> 500
>> # cat delay_off
>> 500
>> 
>> drivers/led/led-core.c:184
>>    /* blink with 1 Hz as default if nothing specified */
>>    if (!*delay_on && !*delay_off)
>>        *delay_on = *delay_off = 500;
>> 
>> Is there a reason that the kernel would decide to blink at this default rate instead of leaving the timer disabled?
>> 
> 
> Yep. It allows hardware to provide different values if it can only
> blink at fixed rate.

Ok, I understand how hardware might make a different choice, but in this scenario we’re using software to blink, so everything is handled by the kernel.  Also, if the interface is hardware enabled, isn’t the path at line 177 used (which returns before the default is set)?

Seth

Re: Strange timer LED behavior

[Bollinger, Seth]

> On Oct 19, 2020, at 9:16 AM, Bollinger, Seth <Seth.Bollinger@digi.com> wrote:
> 
>> 
>> Hi!
>> 
>>> We use the sysfs timer trigger of the led subsystem to control our leds (turn on, turn off, and blink).  While doing this we found a strange behavior.  If the values of delay_on and delay_off are ever both zero, the led subsystem will make the decision to blink at a 500ms on and 500ms off rate.
>>> 
>>> # cd /sys/class/leds/g01
>>> # cat delay_on
>>> 0
>>> # cat delay_off
>>> 86400
>>> # echo 0 > delay_off
>>> # cat delay_on
>>> 500
>>> # cat delay_off
>>> 500
>>> 
>>> drivers/led/led-core.c:184
>>>   /* blink with 1 Hz as default if nothing specified */
>>>   if (!*delay_on && !*delay_off)
>>>       *delay_on = *delay_off = 500;
>>> 
>>> Is there a reason that the kernel would decide to blink at this default rate instead of leaving the timer disabled?
>>> 
>> 
>> Yep. It allows hardware to provide different values if it can only
>> blink at fixed rate.
> 
> Ok, I understand how hardware might make a different choice, but in this scenario we’re using software to blink, so everything is handled by the kernel.  Also, if the interface is hardware enabled, isn’t the path at line 177 used (which returns before the default is set)?

Why should software blink need to select a default rate since it should be able to handle all requests (even 0, 0)?

> Seth
> 
> 

Re: Strange timer LED behavior

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]


> >> Yep. It allows hardware to provide different values if it can only
> >> blink at fixed rate.
> > 
> > Ok, I understand how hardware might make a different choice, but in this scenario we’re using software to blink, so everything is handled by the kernel.  Also, if the interface is hardware enabled, isn’t the path at line 177 used (which returns before the default is set)?
> 
> Why should software blink need to select a default rate since it should be able to handle all requests (even 0, 0)?
> 

How can software blink at "0, 0"?

0, 0 simply means "use defaults", for hardware and software. Its old
API, so you'll have to live with it.

								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Strange timer LED behavior

[Bollinger, Seth]

>>>> Yep. It allows hardware to provide different values if it can only
>>>> blink at fixed rate.
>>> 
>>> Ok, I understand how hardware might make a different choice, but in this scenario we’re using software to blink, so everything is handled by the kernel.  Also, if the interface is hardware enabled, isn’t the path at line 177 used (which returns before the default is set)?
>> 
>> Why should software blink need to select a default rate since it should be able to handle all requests (even 0, 0)?
>> 
> 
> How can software blink at "0, 0"?

This would mean, don’t blink.  :).

Since we’re using sysfs, we need to update those values in 2 FS writes.  It appears that the sysfs is reordering the writes, so we can’t control what value is written when, so it glitches for us.  We change off first, then on, but the order of the calls in the kernel is on first (with off already 0), the kernel changes values to default, then the off is written, leaving on with a value of 500 (we set it to 0) and off with the value we wanted.

Is there a way to avoid this?  Even if we write the on value twice, it will flash for 500ms at least once…

I tried setting the trigger to none, but of course this disabled the display settings and setting the trigger to timer ends up flashing at the default rate, which is also what we don’t want.

> 
> 0, 0 simply means "use defaults", for hardware and software. Its old
> API, so you'll have to live with it.

Ok.

> 
> 								Pavel
> -- 
> https://linkprotect.cudasvc.com/url?a=http%3a%2f%2fwww.livejournal.com%2f~pavelmachek&c=E,1,LHg8yP6R6j23ZCidNyis2lOGhVeQZPy-0zT0-KT2rQ7lS3g3GWhmUMpYB72mDnw_4DaULTMkW0db0ibj1BSW1MR466DO5DZqcHbZwzQ6&typo=1
> 
> 

Re: Strange timer LED behavior

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

Hi!

> >>>> Yep. It allows hardware to provide different values if it can only
> >>>> blink at fixed rate.
> >>> 
> >>> Ok, I understand how hardware might make a different choice, but in this scenario we’re using software to blink, so everything is handled by the kernel.  Also, if the interface is hardware enabled, isn’t the path at line 177 used (which returns before the default is set)?
> >> 
> >> Why should software blink need to select a default rate since it should be able to handle all requests (even 0, 0)?
> >> 
> > 
> > How can software blink at "0, 0"?
> 
> This would mean, don’t blink.  :).

Can't change that now.

> Since we’re using sysfs, we need to update those values in 2 FS writes.  It appears that the sysfs is reordering the writes, so we can’t control what value is written when, so it glitches for us.  We change off first, then on, but the order of the calls in the kernel is on first (with off already 0), the kernel changes values to default, then the off is written, leaving on with a value of 500 (we set it to 0) and off with the value we wanted.
>

Aha, okay, so races are bad and patch to fix them would be probably accepted.

> Is there a way to avoid this?  Even if we write the on value twice,
>  it will flash for 500ms at least once…

There should be. Can you simply write new values to on and off,
without writing zeros there?

If not, can you use 1 instead of zero?

> 
> I tried setting the trigger to none, but of course this disabled the display settings and setting the trigger to timer ends up flashing at the default rate, which is also what we don’t want.
>

If there's no reasonable value to change the frequency, we'll need to
fix that.

Best regards,
							Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Strange timer LED behavior

[Bollinger, Seth]

> 
>> Is there a way to avoid this?  Even if we write the on value twice,
>> it will flash for 500ms at least once…
> 
> There should be. Can you simply write new values to on and off,
> without writing zeros there?

In this case we’re switching from on:86400, off:0 -> on:0, off:86400.  We write off first, then on using fwrite() to a file.  From the trace, it looks like on is always stored first.  So, the first store is on:0, off:0 for a brief period (enough to trigger the default), and then off is stored.  We’re already trying to not have them not both be zero at the same time.  I tried putting fflush() between them, but that didn’t help.  I suppose we could try to sleep some period of time, but this seems bad as well.  Maybe write and read to verify the value is correctly stored?

> 
> If not, can you use 1 instead of zero?

I have not measured the flash, but if on is 1ms, it’s on for a pretty good chunk time (our LED controller is over i2c).  It’s a very noticeable flash.

> 
>> 
>> I tried setting the trigger to none, but of course this disabled the display settings and setting the trigger to timer ends up flashing at the default rate, which is also what we don’t want.
>> 
> 
> If there's no reasonable value to change the frequency, we'll need to
> fix that.

I think if we could control the delays with trigger set to none, then we could set the delays and enable the timer trigger for a no glitch operation.  I suppose if there was a timer-off trigger, that would work as well, but seems kind of silly.


Seth

Re: Strange timer LED behavior

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On Tue 2020-10-20 18:28:33, Bollinger, Seth wrote:
> 
> > 
> >> Is there a way to avoid this?  Even if we write the on value twice,
> >> it will flash for 500ms at least once…
> > 
> > There should be. Can you simply write new values to on and off,
> > without writing zeros there?
> 
> In this case we’re switching from on:86400, off:0 -> on:0, off:86400.  We write off first, then on using fwrite() to a file.  From the trace, it looks like on is always stored first.  So, the first store is on:0, off:0 for a brief period (enough to trigger the default), and then off is stored.  We’re already trying to not have them not both be zero at the same time.  I tried putting fflush() between them, but that didn’t help.  I suppose we could try to sleep some period of time, but this seems bad as well.  Maybe write and read to verify the value is correctly stored?
>

I don't understand. Why would you use blink with on:86400, off:0?

> > If there's no reasonable value to change the frequency, we'll need to
> > fix that.
> 
> I think if we could control the delays with trigger set to none, then we could set the delays and enable the timer trigger for a no glitch operation.  I suppose if there was a timer-off trigger, that would work as well, but seems kind of silly.
>

You may want to look at ledtrig-transient and ledtrig-oneshot...?

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Strange timer LED behavior

[Bollinger, Seth]

>> 
>> In this case we’re switching from on:86400, off:0 -> on:0, off:86400.  We write off first, then on using fwrite() to a file.  From the trace, it looks like on is always stored first.  So, the first store is on:0, off:0 for a brief period (enough to trigger the default), and then off is stored.  We’re already trying to not have them not both be zero at the same time.  I tried putting fflush() between them, but that didn’t help.  I suppose we could try to sleep some period of time, but this seems bad as well.  Maybe write and read to verify the value is correctly stored?
>> 
> 
> I don't understand. Why would you use blink with on:86400, off:0?

In this case we’re using the api to turn the LED on and off, without blinking (in this case only).

> 
>>> If there's no reasonable value to change the frequency, we'll need to
>>> fix that.
>> 
>> I think if we could control the delays with trigger set to none, then we could set the delays and enable the timer trigger for a no glitch operation.  I suppose if there was a timer-off trigger, that would work as well, but seems kind of silly.
>> 
> 
> You may want to look at ledtrig-transient and ledtrig-oneshot...?

Ok, I’ll take a look.  Thanks!

Seth

Re: Strange timer LED behavior

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Tue 2020-10-20 18:35:13, Bollinger, Seth wrote:
> 
> >> 
> >> In this case we’re switching from on:86400, off:0 -> on:0, off:86400.  We write off first, then on using fwrite() to a file.  From the trace, it looks like on is always stored first.  So, the first store is on:0, off:0 for a brief period (enough to trigger the default), and then off is stored.  We’re already trying to not have them not both be zero at the same time.  I tried putting fflush() between them, but that didn’t help.  I suppose we could try to sleep some period of time, but this seems bad as well.  Maybe write and read to verify the value is correctly stored?
> >> 
> > 
> > I don't understand. Why would you use blink with on:86400, off:0?
> 
> In this case we’re using the api to turn the LED on and off, without blinking (in this case only).
>

Yeah. Don't do that! :-). For solid on/solid off, just turn the
trigger off. For blinking, use timer trigger.

Best regards,

								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Strange timer LED behavior

[Bollinger, Seth]

> 
> Yeah. Don't do that! :-). For solid on/solid off, just turn the
> trigger off. For blinking, use timer trigger.

If I want to go from on or off to flashing, I need to set the timer trigger, which results in a 500 ms flash rate until the real rates are set.  This results in a noticeably different first blink.  I guess we’re looking for smooth control of the LED.

Seth

Re: Strange timer LED behavior

[Jacek Anaszewski]

Hi Seth,

On 10/20/20 8:35 PM, Bollinger, Seth wrote:
> 
>>>
>>> In this case we’re switching from on:86400, off:0 -> on:0, off:86400.  We write off first, then on using fwrite() to a file.  From the trace, it looks like on is always stored first.  So, the first store is on:0, off:0 for a brief period (enough to trigger the default), and then off is stored.  We’re already trying to not have them not both be zero at the same time.  I tried putting fflush() between them, but that didn’t help.  I suppose we could try to sleep some period of time, but this seems bad as well.  Maybe write and read to verify the value is correctly stored?
>>>
>>
>> I don't understand. Why would you use blink with on:86400, off:0?
> 
> In this case we’re using the api to turn the LED on and off, without blinking (in this case only).
> 
>>
>>>> If there's no reasonable value to change the frequency, we'll need to
>>>> fix that.
>>>
>>> I think if we could control the delays with trigger set to none, then we could set the delays and enable the timer trigger for a no glitch operation.  I suppose if there was a timer-off trigger, that would work as well, but seems kind of silly.
>>>
>>
>> You may want to look at ledtrig-transient and ledtrig-oneshot...?
> 
> Ok, I’ll take a look.  Thanks!

You might want to check also ledtrig-pattern and paragraph 2.
of its documentation [0].

[0] Documentation/devicetree/bindings/leds/leds-trigger-pattern.txt

-- 
Best regards,
Jacek Anaszewski

Re: Strange timer LED behavior

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

Hi!

> > Yeah. Don't do that! :-). For solid on/solid off, just turn the
> > trigger off. For blinking, use timer trigger.
> 
> If I want to go from on or off to flashing, I need to set the timer trigger, which results in a 500 ms flash rate until the real rates are set.  This results in a noticeably different first blink.  I guess we’re looking for smooth control of the LED.
>

Okay, that's fair requirement, I guess.

If you have proposals how to solve this in a nice way, go ahead.

As a hack...

I believe you can go from on to off like this:

on:9999,off:0 -> on:9999,off:1 -> on:1,off:9999 -> on:0,off:9999.

(And I believe we should document this somewhere).

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Strange timer LED behavior

[Bollinger, Seth]

>>> Yeah. Don't do that! :-). For solid on/solid off, just turn the
>>> trigger off. For blinking, use timer trigger.
>> 
>> If I want to go from on or off to flashing, I need to set the timer trigger, which results in a 500 ms flash rate until the real rates are set.  This results in a noticeably different first blink.  I guess we’re looking for smooth control of the LED.
>> 
> 
> Okay, that's fair requirement, I guess.
> 
> If you have proposals how to solve this in a nice way, go ahead.
> 
> As a hack...
> 
> I believe you can go from on to off like this:
> 
> on:9999,off:0 -> on:9999,off:1 -> on:1,off:9999 -> on:0,off:9999.
> 
> (And I believe we should document this somewhere).


I hate to respond to this, because it makes me look stupid, but…  :)

The problem turned out to be that the original author was using buffered IO to do the sysfs writes (which I didn’t notice until yesterday).  When I switched to a different method, the writes were ordered correctly and didn’t glitch.

Apologies for bothering you all, and great thanks for your assistance!

Thanks again!

Seth

Re: Strange timer LED behavior

[Bollinger, Seth]

> 
> You might want to check also ledtrig-pattern and paragraph 2.
> of its documentation [0].
> 
> [0] Documentation/devicetree/bindings/leds/leds-trigger-pattern.txt

Thank you!  I will check this out.

Seth

Re: Strange timer LED behavior

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

On Thu 2020-10-22 11:34:34, Bollinger, Seth wrote:
> 
> >>> Yeah. Don't do that! :-). For solid on/solid off, just turn the
> >>> trigger off. For blinking, use timer trigger.
> >> 
> >> If I want to go from on or off to flashing, I need to set the timer trigger, which results in a 500 ms flash rate until the real rates are set.  This results in a noticeably different first blink.  I guess we’re looking for smooth control of the LED.
> >> 
> > 
> > Okay, that's fair requirement, I guess.
> > 
> > If you have proposals how to solve this in a nice way, go ahead.
> > 
> > As a hack...
> > 
> > I believe you can go from on to off like this:
> > 
> > on:9999,off:0 -> on:9999,off:1 -> on:1,off:9999 -> on:0,off:9999.
> > 
> > (And I believe we should document this somewhere).
> 
> 
> I hate to respond to this, because it makes me look stupid, but…  :)
> 
> The problem turned out to be that the original author was using buffered IO to do the sysfs writes (which I didn’t notice until yesterday).  When I switched to a different method, the writes were ordered correctly and didn’t glitch.
> 
> Apologies for bothering you all, and great thanks for your assistance!

Hehe, okay. Good that it is solved.

(And yes, such bugs happen, forgetting about buffered i/o is quite
common).

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]