Re: [lwip-users] Sys_Timeout(); Calling Handler Immediately?

[chrysn]

hello list,

On Sat, Oct 19, 2013 at 09:49:30PM -0700, Ishmeet wrote:
> The problem--> sys_timeout() was calling handler function immediately. Is
> fixed now.
> 
> I have now fixed the problem using sys_restart_timeouts() before
> sys_timeout(), sys_restart_timeouts() sets back the timestamp of the last
> call to sys_check_timeouts(). Thus after 1000 milliseconds, I get called
> the handler function (Just what I needed).

i finally found the cause of ishmeet's problem. (not by actively looking
for it, tbh, but because it bit me too).

i'll explain a little context as sys_timeout seems not to be widely
used with NO_SYS (even though it's a great feature):

* all values we're talking about here are sys_now() style, ie. uint32
  that give milliseconds since startup or differences thereof. (wrapping
  is not an issue because we only deal with deltas, and as timeouts are
  always positive, we're fine to just assume they are and everything
  works modulo 2^32).

* static u32_t timeouts_last_time is set to the last time a timeout was
  handled (or system startup)

* (static sys_timeo *next_timeout, ->next) is a linked list, that's
  always sorted, and whose ->time members give the timeout relative to
  the entry's predecessor (if one exists) or to timeouts_last_time.

* at some point in the main loop, one calls sys_check_timeouts(), which
  works off `sys_now() - timeouts_last_time` from the linked list, frees
  the entry and calls the associated callback. then, it sets
  timeouts_last_time to when it started.

now the issue is that `sys_timeout(msecs, handler, arg)`, which
establishes entries in that linked list, takes msec (which are implied
to be measured from now), but by inserting them into a list that is
relative to timeouts_last_time, it can happen that entries are fired too
early.

that's not an issue if one calls `sys_timeout` from right after the
timing system was started, or from inside a hook, because then, the
delta between now and timeouts_last_time is zero anyways. but it does
matter when one starts from an arbitrary point in time, because then,
events fire far too early.


my patch fixes this. it also fixes another error that doesn't show up
"usually": time that has elapsed between the timeout was due and the
invocation of sys_check_timeouts got ignored by setting
timeouts_last_time to now, thus pushing back timeouts that got ignored
due to long delays in invoking sys_check_timeouts, and pulling ahead
timeouts that were scheduled from the handler. instead now, only the
time accounted for (by popping the first element from the list, and by
subtracting its time from the delta) is now added to timeouts_last_time.

it is ok that by that, timeouts_last_time does not always get updated to
"now" -- as long as we have a consistent time base for the list. (to be
precise: timeouts_last_time equals sys_now() iff by the time compared,
the last timeout that has been popped has been due for exactly 0ms).


note that this does subtly change timings -- while before, when a
timeout has been scheduled again inside itself, it would trigger in
regular intervals, independent of whether the routine took ages or the
check routine was delayed; now, it will trigger as many ms later as
specified at earliest. this allows reliably setting timers from outside
timers too, and i'd argue that if a timeout requests to be called in
5000ms after having taken some time to process itself, it should be ok
with that event realy taking those 5000ms.

also, it does incur the overhead of calling sys_now one more time.
should this become really critical, some optimizations could be done,
but sys_now() should really not take long. (if that became critical, i'd
much rather introduce a sys_now_compare() macro that is defined by
whoever defines sys_now, and allows for <32bit implementations, eg.
24bit RTCs commonly found in microcontrollers).


my patch has been submitted to the patch tracker[1]. committers, please
review (especially with respect to interaction with NO_SYS=0) and apply.

best regards
chrysn

[1] https://savannah.nongnu.org/patch/index.php?8244

-- 
To use raw power is to make yourself infinitely vulnerable to greater powers.
  -- Bene Gesserit axiom

signature.asc
Description: Digital signature