tasklwIP - A Lightweight TCP/IP stack - Tasks: task #7235, Create timeout framework for...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #7235: Create timeout framework for NO_SYS=1

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Thu 23 Aug 2007 06:16:45 AM UTC  
 
Category:  None Should Start On:  Thu 23 Aug 2007 12:00:00 AM UTC
Should be Finished on:  Thu 23 Aug 2007 12:00:00 AM UTC Priority:  3 - Low
Status:  Done Privacy:  Public
Percent Complete:  100% Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  1.4.0
Effort:  0.00

( Jump to the original submission)

Sun 10 Jan 2010 11:44:19 AM UTC, comment #22: 

Regarding this as done as we have task #7244 for the rest.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 31 Dec 2009 04:30:47 PM UTC, comment #21: 

...presses 'send' to early...:

> removed timeout processing from nearly all sem/mbox functions


This also means that sys_arch_timeouts() / storing a timeout list per thread is not required any more: there is only one global timeout list which is always processed from tcpip_thread or main-loop. This is both simpler and faster than before.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 31 Dec 2009 04:23:39 PM UTC, comment #20: 

To get this started (and to get PPP running with NO_SYS==!), I've separated the timeout implementation from the sem/mbox implementation (timeout code moved to new file). I didn't change much on the timer semantics though:
- added sys_check_timeouts() to check timeouts with NO_SYS==1 (from the main loop)
- removed timeout processing from nearly all sem/mbox functions except one explicitly named function that is only used from tcpip_thread: timeouts must not be processed from any other place or we risk reentrancy/threading problems!

I'll update the 2 example ports as soon as I find the time plus I'll check in changes to PPP for NO_SYS==1.

I guess work on this task is now nearly done, please comment if you want to have it solved differently (as I only used the NO_SYS==0 implementation for NO_SYS==1).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 08 Dec 2009 07:50:09 AM UTC, comment #19: 

I'm currently inclined to jsut keep with the current implementation of a linked list of timeouts, as we can us that with NO_SYS=0, too, and sys_mbox_wait is easier to implement. However, we could introduce periodic timers to prevent deallocating and directly re-allocating the timeout struct via memp.

I'd move the timers and timeout functions from tcpip.c and sys.c to a new file, timers.c. This way, it is better separated from mbox/sem in sys.c.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 03 Dec 2009 07:53:57 PM UTC, comment #18: 

I'd be happy to add this after releaseing 1.3.2. However, I'd like to merge this into sys.h/.c so that both configurations can share as much code as possible. Therefore, the attached files would only be a starter.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 13 Nov 2009 02:59:32 PM UTC, comment #17: 

It's been quite a long time since this
thread was started. So far the only
discussion was about how to implement the timers.
I can confirm that nosys.c and nosys.h, sent by
Frederic work perfect for me with only one
difference: Added

if( time_diff < 0 ) {
/* the '> 0' is an easy wrap-around check: the big gap at the wraparound step is simply ignored... */
time_diff = 0;
last_time = cur_time;
}

before the if (time_diff > 0) check.

Otherwise when the tick counter overflows the value
0xFFFF0000, the (time_diff > 0) check will fail
for the next 65.535 seconds! This happens, as you know,
every 49 days. Not a big deal but may be irritating.
This is all I can say about the timers.

Now the timeouts:
I implemented a nosys framework with a small array
of timeout structs (12 bytes):

struct nosys_timeout_s {
  u32_t time;
  nosys_timeout_handler h;
  void *arg;
};

struct nosys_timeout_s nosys_timeouts[NOSYS_TIMEOUTS_NUM];

The following functions are defined:
void nosys_timeouts_init(void);
void nosys_timeout(u32_t msecs, nosys_timeout_handler h, void *arg);
void nosys_untimeout(nosys_timeout_handler h, void *arg);
void nosys_tout_timers(void);
u8_t nosys_getfreetouts(void);

#if 1 /* Test nosys_timeouts implementation */
void test_nosys_timeouts(void);
#endif

They work exactly as the sys_* cousins.
The timers are one-shot, but there is no problem
to request the timeout inside itself. This is
how it can becomes periodic like the others in
timers_table[];

There is also a test_nosys_timeouts() function
that proofs everything works as expected.

If you accept this, the following should be added to opt.h:
#ifndef NOSYS_TIMEOUTS_NUM
#define NOSYS_TIMEOUTS_NUM 5 /* Define how many simultaneous timeout can be active*/
#endif

#ifndef NOSYSTO_DEBUG
#define NOSYSTO_DEBUG                      LWIP_DBG_OFF
#endif

This is the log when the test is started:
nosys_timeouts_init(): Ready

 starting test_nosys_timeouts...

 Reg TO @ idx 0, time 1000, h 0x00037538, arg 0x40006A0C
 Reg TO @ idx 1, time 2000, h 0x000375B4, arg 0x40006A10
 Reg TO @ idx 2, time 3000, h 0x00037658, arg 0x40006A14

 nosys_tout_timers: Starting TO @ idx 0, handler 0x00037538, arg 0x40006A0C

 my_timeout1 fired, arg = 0 <--

 nosys_tout_timers: Finished TO @ idx 0 <--

 nosys_tout_timers: Starting TO @ idx 1, handler 0x000375B4, arg 0x40006A10

 my_timeout2 fired, arg = 1 <--

 Reg TO @ idx 0, time 2000, h 0x000375B4, arg 0x40006A10

 nosys_tout_timers: Finished TO @ idx 1 <--

 nosys_tout_timers: Starting TO @ idx 2, handler 0x00037658, arg 0x40006A14

 my_tout3a fired, arg = 1 <--(free: 4)

 Reg TO @ idx 1, time 2000, h 0x00037708, arg 0x40006A14

 nosys_tout_timers: Finished TO @ idx 2 <--

 nosys_tout_timers: Starting TO @ idx 0, handler 0x000375B4, arg 0x40006A10

 my_timeout2 fired, arg = 2 <--

 Reg TO @ idx 0, time 2000, h 0x000375B4, arg 0x40006A10

 nosys_tout_timers: Finished TO @ idx 0 <--

(file #19043, file #19044, file #19045, file #19046)

Iordan Neshev <iordan_neshev>
Wed 28 Nov 2007 03:55:45 PM UTC, comment #16: 

I'd prefer to wait till after 1.3.0 before merging this

Kieran Mansley <kieranm>
Project Member
Mon 26 Nov 2007 11:44:05 AM UTC, comment #15: 

Even if I think that change current lwIP timers handling is a good idea, I thought it could be done in post-1.3.0, and provide the framework in pre-1.3.0.

Perhaps we could merge now nosys.h/.c in tcpip.h/.c to provide the framework for 1.3.0 (since it doesn't break any code, using the #if NO_SYS/#endif of course), and later (after 1.3.0 release), study how to use it in with NO_SYS=0 case ?

Frédéric Bernon <fbernon>
Project Member
Mon 26 Nov 2007 11:34:33 AM UTC, comment #14: 

Oh, and something about the implementation of nosys_timers:

when a timer elapsed, you do the following:

t.timer -= t.timer_interval;

I suppose you do it like that to minimize timer jitter. However, it might be better to set t.timer to zero (or at least have an upper limit for t.timer after it is substracted) to prevent timers of taking too much processor time...?

Also, I'm not sure the wraparound check really works like that ...?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 26 Nov 2007 11:29:13 AM UTC, comment #13: 

Yes why not? Instead of all the timer callback functions in tcpip.c that call sys_timeout() to re-enqueue the timer, just use your table approach and call your nosys_timers(cur_time) function at the smallest interval.

Calling at the right interval is a problem to be solved, of course. But I think that could solve all the discussion about inaccurate periodic timers we had in task #7244 (where we talked about using the exact current time to know whether a timer has elapsed instead of the time waiting for a message like it is now).

To get the interval at which to call nosys_timers(), we could either call sys_time() in every loop in tcpip_thread or let the user specify a function to call every x ms that puts a message into the tcpip_thread mbox...

Anyway, I think this would be even better thant the approach in task #7244!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 26 Nov 2007 10:30:12 AM UTC, comment #12: 

>So we could do it the other way round and use the timers table for the lwIP internal periodic timers? Integrating that into tcpip.c should not be that difficult


Do you mean you want to do it for NO_SYS=0 ?

Frédéric Bernon <fbernon>
Project Member
Mon 26 Nov 2007 08:42:01 AM UTC, comment #11: 

So we could do it the other way round and use the timers table for the lwIP internal periodic timers? Integrating that into tcpip.c should not be that difficult.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 26 Nov 2007 08:23:01 AM UTC, comment #10: 

I think there is a problem to use "timeouts" for lwIP's "timers". These last one are periodic, the first are "one shot". Due to the initial purpose, I thought it was the idea, but it seems I'm wrong.

Perhaps we could split "user timeouts" design from "timers" design, in another task? But I think than having the same name (sys_timeout) with two differents execution environment (NO_SYS=0 or 1) is not a so good idea (since other sys_ functions will not be available)...

Frédéric Bernon <fbernon>
Project Member
Mon 26 Nov 2007 08:04:52 AM UTC, comment #9: 

Why don't we try to resemble the current API? Implementing the sys_timeout list like it is now but don't check it inside sys_sem_wait or sys_mbox_fetch but check it in your function nosys_timers(cur_time)?

The downside would be that more code is required... but it would prevent us from having two totally different implementations, and user timeouts would also work.

I think having user timeouts is the biggest benefit of the current solution: you can receive one UDP packet and use sys_timeout to set a timeout. With the proposed nosys.c, this won't work.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 26 Nov 2007 12:23:12 AM UTC, comment #8: 

I attach nosys.c and nosys.h. I think we could add them here :

lwip\src\api\nosys.c
lwip\src\include\lwip\nosys.h

Thoughts?

(file #14482, file #14483)

Frédéric Bernon <fbernon>
Project Member
Sat 03 Nov 2007 06:06:48 PM UTC, comment #7: 

>Hmm, I thought we could somehow reuse the sys_timeout function for NO_SYS


But if NO_SYS=1, there is no implementation of sys_timeout, right?

>The downside is of course that we have two places in the code that deal with timeouts...


Yes, but there is not lot of common code. Except if we change tcpip_thread to use the timers_table...

I'm in favor to add a nosys.c file. Comments?

Frédéric Bernon <fbernon>
Project Member
Sat 03 Nov 2007 02:56:10 PM UTC, comment #6: 

Hmm, I thought we could somehow reuse the sys_timeout function for NO_SYS. But on the other hand, your implementation might be a lot smaller because it is not configurable at runtime...

The downside is of course that we have two places in the code that deal with timeouts...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 02 Nov 2007 06:31:57 PM UTC, comment #5: 

The only problem to use "contrib/ports/msvc6/test.c" like a timeout framework is the use of "sys_get_ms()". A simple change could be to replace...

static void
timers_update()
{ ... 
  int cur_time, time_diff, idxtimer;

  cur_time = sys_get_ms();
  ...
}

...by...

static void
timers_update( int cur_time)
{ ... 
  int time_diff, idxtimer;
  ...
}

Like this, the code can be in the lwIP core (in a nosys.c file? or in tcpip.c?), and the "cur_time" is provided by the port in its "main loop".

Frédéric Bernon <fbernon>
Project Member
Fri 02 Nov 2007 04:36:19 PM UTC, comment #4: 

(extract contrib/ports/msvc6/test.c)

#if NO_SYS
/* port-defined functions used for timer execution */
void sys_init_timing();
u32_t sys_get_ms();
#endif /* NO_SYS*/

#if NO_SYS
/* special functions used for NO_SYS=1 only */
typedef struct _timers_infos {
  int timer;
  int timer_interval;
  void (*timer_func)(void);
}timers_infos;

static timers_infos timers_table[] = {
#if LWIP_TCP
  { 0, TCP_FAST_INTERVAL,       tcp_fasttmr},
  { 0, TCP_SLOW_INTERVAL,       tcp_slowtmr},
#endif /* LWIP_TCP */
#if LWIP_ARP
  { 0, ARP_TMR_INTERVAL,        etharp_tmr},
#endif /* LWIP_ARP */
#if LWIP_DHCP
  { 0, DHCP_FINE_TIMER_MSECS,   dhcp_fine_tmr},
  { 0, DHCP_COARSE_TIMER_MSECS, dhcp_coarse_tmr},
#endif /* LWIP_DHCP */
#if IP_REASSEMBLY
  { 0, IP_TMR_INTERVAL,         ip_reass_tmr},
#endif /* IP_REASSEMBLY*/
#if LWIP_AUTOIP
  { 0, AUTOIP_TMR_INTERVAL,     autoip_tmr},
#endif /* LWIP_AUTOIP */
#if LWIP_IGMP
  { 0, IGMP_TMR_INTERVAL,       igmp_tmr},
#endif /* LWIP_IGMP */
};

/* initialize stack when NO_SYS=1 */
static void
nosys_init()
{
  sys_init_timing();
  lwip_init();
}

/* get the current time and see if any timer has expired */
static void
timers_update()
{
  /* static variables for timer execution, initialized to zero! */
  static int last_time;

  int cur_time, time_diff, idxtimer;

  cur_time = sys_get_ms();
  time_diff = cur_time - last_time;

  /* the '> 0' is an easy wrap-around check: the big gap at
   * the wraparound step is simply ignored... */
  if (time_diff > 0) {
    last_time = cur_time;
    for( idxtimer=0; idxtimer<(sizeof(timers_table)/sizeof(timers_infos)); idxtimer++) {
      timers_table[idxtimer].timer += time_diff;

      if (timers_table[idxtimer].timer > timers_table[idxtimer].timer_interval) {
        timers_table[idxtimer].timer_func();
        timers_table[idxtimer].timer -= timers_table[idxtimer].timer_interval;
      }
    }
  }
}
#endif /* NO_SYS */

...

  /* MAIN LOOP for driver update (and timers if NO_SYS) */
  while (!_kbhit()) {
#if NO_SYS
    /* handle timers (already done in tcpip.c when NO_SYS=0) */
    timers_update();
#endif /* NO_SYS */

    /* check for packets */
    update_adapter();
  }

...

Frédéric Bernon <fbernon>
Project Member
Wed 29 Aug 2007 09:10:23 AM UTC, comment #3: 

AUTOIP_TMR_INTERVAL could be easily change to 250 (Dominik comments in autoip.c: "I recommend a value of 100. The value must divide 1000 with a remainder almost 0. Possible values are 1000, 500, 333, 250, 200, 166, 142, 125, 111, 100...", same for IGMP_TMR_INTERVAL (IGMP querier give several secondes to answer to a "Query message" most of time).

They not really change performance. I don't know for others timers, but they can be divided by 250 from memory.

Frédéric Bernon <fbernon>
Project Member
Wed 29 Aug 2007 08:59:19 AM UTC, comment #2: 

I think we'd need 100ms as the common denominator of the timers, but this is a low priority task anyway.

Kieran Mansley <kieranm>
Project Member
Thu 23 Aug 2007 07:12:26 AM UTC, comment #1: 

It's even a good idea that could be used to replace current "timers" in tcpip_thread based on "sys_timeouts".

Frédéric Bernon <fbernon>
Project Member
Thu 23 Aug 2007 06:16:45 AM UTC, original submission:  

I think we should create a functionality for NO_SYS=1 users so that they don't have to explicitly call every timer function in their main loop but instead something like timer_tick() which checks all timers (maybe based on 250ms, which seems to be the lowest timer interval, so all other timers could be multiplied from that).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.

 

Attached Files
file #19046:  nosys_timeouts.h added by iordan_neshev (718B - application/octet-stream - NO_SYS timers and timeouts)
file #19045:  nosys_timeouts.c added by iordan_neshev (7KiB - application/octet-stream - NO_SYS timers and timeouts)
file #19043:  nosys.c added by iordan_neshev (5KiB - application/octet-stream - NO_SYS timers and timeouts)
file #19044:  nosys.h added by iordan_neshev (2KiB - application/octet-stream - NO_SYS timers and timeouts)
file #14482:  nosys.h added by fbernon (2KiB - text/x-chdr)
file #14483:  nosys.c added by fbernon (4KiB - text/x-csrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by iordan_neshev (Updated the item)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-01-10 goldsimon StatusNone => Done
        Percent Complete90% => 100%
        Open/ClosedOpen => Closed
    2009-12-31 goldsimon Percent Complete10% => 90%
        Assigned toNone => goldsimon
    2009-12-08 goldsimon Planned ReleaseNone => 1.4.0
    2009-11-13 iordan_neshev Attached File- => Added nosys.c, #19043
        Attached File- => Added nosys.h, #19044
        Attached File- => Added nosys_timeouts.c, #19045
        Attached File- => Added nosys_timeouts.h, #19046
    2007-11-26 fbernon Attached File- => Added nosys.h, #14482
        Attached File- => Added nosys.c, #14483
        Percent Complete0% => 10%
    2007-08-29 kieranm Priority5 - Normal => 3 - Low

    Back to the top


    Powered by Savane 3.5