lcd4linux-devel

[Lcd4linux-devel] Two new FIXMEs in timer code :)

[Martin Z. <co...@mz...>]

Hello everybody!

I finally had the time to beautify, streamline and fully comment both 
the timer and timer group code.  It's been quite a bit of work, but I 
can only encourage everyone to do it.  Commenting your code will not 
only enable others to help fixing bugs much more efficiently because the 
code is much easier to understand.  On "re-thinking" the code I and 
others wrote, I have also found three to four would-be problems or even 
bugs per C file.

That being said, here come the two things in "timer.c" I didn't want to 
change without asking in advance.

First, I think that CLOCK_SKEW_DETECT_TIME_IN_S should be set to a much 
higher value:

   /* delay in seconds between timer events that is considered as being
      induced by clock skew */
   #define CLOCK_SKEW_DETECT_TIME_IN_S 1

Imagine you only have timers that trigger every two seconds (think of a 
rather slow processor and/or LCD with data that rarely change).  Right 
now, the timers always trigger immediately instead of every two seconds 
because the code assumes the delay (two seconds) is being caused by 
clock skew instead of the timer.

Of course, increasing CLOCK_SKEW_DETECT_TIME_IN_S won't get rid of the 
problem completely.  But it will lower the chances that a user will 
stumble upon the "clock skew problem" (which might be totally obscure to 
him or her).  I propose a value of 61 seconds (just above one minute). 
This should be a sane value that is able to efficiently spot clock skew 
while not needlessly constraining the users' freedom. :)

But I'm open to comments -- that's why I'm writing all this.

The other "thing" seems like a real bug to me (and is of course related 
to clock skew).  In case a clock skew has been detected, the next 
upcoming timer (and *only* that) is updated to the new time.

Now imagine two timers of different intervals and a clock skew of one 
hour (think of the transitions between summer and winter time).  The 
next upcoming timer (let's say number 1) is updated and triggers on. 
But what about the second?  If I get that right, it will not trigger for 
at least a whole hour while the user awes in wonder!

So I think we should update *all* timers:

   for (timer = 0; timer < nTimers; timer++)
     Timers[timer].when = now;

instead of only the next upcoming one:

   Timers[next_timer].when = now;

Again, I'm open for discussion.  Maybe I even got something wrong as I 
tried to bend my brain around the PROBLEM OF TIME -- me not being a 
philosopher... :)

Have a nice Sunday,

Martin
-- 
www.mzuther.de
www.radix-musik.de

Re: [Lcd4linux-devel] Two new FIXMEs in timer code :)

[Michael R. <mi...@re...>]

Hi Martin,

sorry for being that late in answering your question, but I'm busy busy busy busy...

Thanks for your analysis, here are my comments:

> First, I think that CLOCK_SKEW_DETECT_TIME_IN_S should be set to a much
> higher value:
>
>     /* delay in seconds between timer events that is considered as being
>        induced by clock skew */
>     #define CLOCK_SKEW_DETECT_TIME_IN_S 1
>
> Imagine you only have timers that trigger every two seconds (think of a
> rather slow processor and/or LCD with data that rarely change).  Right
> now, the timers always trigger immediately instead of every two seconds
> because the code assumes the delay (two seconds) is being caused by
> clock skew instead of the timer.

clock skew detection was added in r1027 by michux, unfortunately I couldn'd find anything useful in my archives about 
the background. But I agree: 1 second may be too short, probably it never showed up because everybody is using timers 
shorter than one second.

A clean solution cold be to check if timestamp of next timer is later than "now + timer.delay"? (or probably "now + 
2*timer.delay") ?

I also think that the correction "set it to current" is wrong: It should be set to "now + timer+delay" or something similar.

> The other "thing" seems like a real bug to me (and is of course related
> to clock skew).  In case a clock skew has been detected, the next
> upcoming timer (and *only* that) is updated to the new time.
>
> Now imagine two timers of different intervals and a clock skew of one
> hour (think of the transitions between summer and winter time).  The
> next upcoming timer (let's say number 1) is updated and triggers on.
> But what about the second?  If I get that right, it will not trigger for
> at least a whole hour while the user awes in wonder!
>
> So I think we should update *all* timers:
>
>     for (timer = 0; timer<  nTimers; timer++)
>       Timers[timer].when = now;
>
> instead of only the next upcoming one:
>
>     Timers[next_timer].when = now;

I'm not sure about this: probably all the other timers would be corrected one by one as soon as they are processed 
(every timer should be the "next occuring timer" somethime, and then would be corrected)



regards, Michael

-- 
Michael Reinelt <mi...@re...>
http://home.pages.at/reinelt
GPG-Key 0xDF13BA50
ICQ #288386781

Re: [Lcd4linux-devel] Two new FIXMEs in timer code :)

[Martin Z. <co...@mz...>]

Hi Michael,

I'm quite busy as well, so never mind the delay... ;)

 > A clean solution cold be to check if timestamp of next timer is later
 > than "now + timer.delay"? (or probably "now + 2*timer.delay") ?

That sounds clever.  Though I think that checking for

   now + timer.delay + CLOCK_SKEW_DETECT_TIME_IN_S

would be best.  This way, time skew detection wouldn't depend on the 
actual timer intervals and users might still easily adapt the detection 
limit to their needs.

 > I also think that the correction "set it to current" is wrong: It should
 > be set to "now + timer+delay" or something similar.

You're right.  I think that adding the detected time skew to the timer 
triggers would be best.

 >> The other "thing" seems like a real bug to me (and is of course related
 >> to clock skew). In case a clock skew has been detected, the next
 >> upcoming timer (and *only* that) is updated to the new time.
 >>
 >> Now imagine two timers of different intervals and a clock skew of one
 >> hour (think of the transitions between summer and winter time). The
 >> next upcoming timer (let's say number 1) is updated and triggers on.
 >> But what about the second? If I get that right, it will not trigger for
 >> at least a whole hour while the user awes in wonder!
 >>
 >> So I think we should update *all* timers:
 >>
 >> for (timer = 0; timer< nTimers; timer++)
 >> Timers[timer].when = now;
 >>
 >> instead of only the next upcoming one:
 >>
 >> Timers[next_timer].when = now;
 >
 > I'm not sure about this: probably all the other timers would be
 > corrected one by one as soon as they are processed (every timer should
 > be the "next occuring timer" somethime, and then would be corrected)

Think of these (bogus) trigger time stamps (in seconds):

   current time:  3615
   timer #0:      3616
   timer #1:      3618

Now imagine that the system time is set backward by one hour (think of 
winter time):

   current time:    15
   timer #0:      3616
   timer #1:      3618

By the current code, timer #0 (the next upcoming one) would be corrected:

   current time:    15
   timer #0:        15
   timer #1:      3618

But timer #0 will *still* the next upcoming timer, so timer #1 will 
*not* be corrected (and thus not trigger) for an hour:

   current time:    16    17    18  ...  3618  3619
   timer #0:        16    17    18  ...  3618  3619
   timer #1:      3618  3618  3618  ...  3618  3621

Feel free to correct me, but I think that I'm correct. :)

Finally, I have updated the function "timer_inc()" (rev 1141).  The 
function now checks how many trigger intervals have passed since a given 
timer has been updated.  This might be due to "negative clock skew" 
(think of summer time) or the fact that some processing took too long 
(i.e. fetching of a web site).  These missed trigger intervals are then 
skipped and the user is notified so that he may adapt his timer settings.

This handling is essential, otherwise unprocessed timers might stack up 
and would trigger continuously while at the same time becoming 
notoriously late and unreliable.  As the "timer_inc()" is called right 
after calling a timer's callback, the code change also shouldn't do any bad.

Happy new year (somewhat late) from Germany,

Martin
-- 
www.mzuther.de
www.radix-musik.de

Re: [Lcd4linux-devel] Two new FIXMEs in timer code :)

[Martin Z. <co...@mz...>]

Hi Michael,

I wanted to get rid of this time skew business once and forever.  So I 
checked my assumptions with the "date --set" command.

I was completely right regarding the timers' behaviour on clock skews. 
So I updated the clock skew testing routine to check for

   now + timer.delay + CLOCK_SKEW_DETECT_TIME_IN_S

Moreover, I changed the clock skew correction routine to subtract the 
detected clock skew from *all* timers.

Finally, I checked this extensively using "date --set".  Although my 
system once got a little unstable, but "lcd4linux" simply kept on 
running.  So I guess we're done here.

I committed my changes to the repository.  Feel free to try it out 
yourself (but I advise you to close all applications before you start 
manipulating the Linux system time).

All the best,

Martin
-- 
www.mzuther.de
www.radix-musik.de