patchlwIP - A Lightweight TCP/IP stack - Patches: patch #5785, Integrate SNMP initialization in...

 
 

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

patch #5785: Integrate SNMP initialization in tcpip.c

Submitter:  Frédéric Bernon <fbernon>
Submitted:  Tue 06 Mar 2007 07:50:13 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Wont Do Privacy:  Public
Assigned to:  fbernon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Wed 23 May 2007 08:40:02 AM UTC, comment #17: 

Ok, I close it because :

- features like "ticks" & "jiffies" to replace snmp timers will be another task

- we don't get a consensus about a lwip_init function (see first comments)


Frédéric Bernon <fbernon>
Group Member
Mon 12 Mar 2007 12:01:03 PM UTC, comment #16: 

I don't like 64-bit in lwIP core, for the following reasons:
 - it introduces unnecessary overhead
 - there are still compilers that do not support 64-bit.

I believe it is important to decide what new function we want to add in ports, whether it is get_tickcount or time_now.

I prefer get_tickcount because it is easier to implement in any port without having any problem with overflow.

Dmitry Potapov <dpotapov>
Mon 12 Mar 2007 10:10:29 AM UTC, comment #15: 

I don't have enough understanding of the many timeout related issues that are going on at the moment to make a good decision I'm afraid. However, I don't want this to hold things up and I'm happy for you (collectively) to make a decision if some consensus can be reached.  I would prefer:
 - a complete solution, even if that breaks ports - we're breaking ports in the next release anyway, so makes sense to get all the breakage out in one go.
 - something that will work well for both the raw API and the sequential/sockets API
 - something that is efficient to execute and port.
 - something that doesn't bring too much detail of "other" protocols into the core of lwIP.

Your use of a branch to share big changes such as this until they have been tested is much appreciated.

Kieran Mansley <kieranm>
Group Member
Mon 12 Mar 2007 09:29:55 AM UTC, comment #14: 

Hm, 64 bit is what I would favour, also. But 64-bit calculating in on a 16-bit processor is not what you can call fast, I think! Plus we need that timestamp rather often...

But I would favour a portable solution (like a sys_time_t) which allows for 64bit or 32bit time (16bit is too short). I think Dmitrys patch can do that.

Regarding the SNMP sysUpTime value: if it is defined to be in 100th of seconds (not 1000th), we should implement it that way. Of course, (also with the current solution?) if you count the time in ms and the nconvert to 10ms, you get an overflow after 49,7 days instead of 497 days. I don't think that's a real problem. As you say, "not infinite"...

I would definitively not change SNMP time implementation until that timestamp thing is solved. (sysuptime+=inc would work but is a suboptimal solution, I think.)

This leads us to bugs #19167 and #1902:

Should we implement this / check it in to CVS HEAD or not???? It will break current ports, but I think it's worth it!

Kieran?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 12 Mar 2007 09:10:39 AM UTC, comment #13: 

Just an alternative idea: most of "timestamp" features are implemented as "unsigned int 64bits". With that, you can use it like "absolute" time (like file times, "number of 100-nanosecond intervals since January 1, 1601"), or relative time from boot time.

So, Solution #1, why not "u64_t sys_arch_get_time_now();" ? Is it a real problem? If a target can provide a u64_t tickcount, it can  always use the older system with timeout...

About SNMP problem. sysUpTime is defined like this:

http://www.faqs.org/rfcs/rfc1213.html "RFC 1213 - Management Information Base for Network Management of TCP/IP-based internets:MIB-II"

          sysUpTime OBJECT-TYPE
              SYNTAX  TimeTicks
              ACCESS  read-only
              STATUS  mandatory
              DESCRIPTION
                      "The time (in hundredths of a second) since the network management portion of the system was last                      re-initialized."
              ::= { system 3 }

http://www.faqs.org/rfcs/rfc1155.html "RFC 1155 - Structure and identification of management information for TCP/IP-based internets"

 TimeTicks ::=
                      [APPLICATION 3]
                          IMPLICIT INTEGER (0..4294967295)

All that indicate that sysUpTime will overflow each ~497 days. Long, but not infinite. So, Solution #2, if we use a u32_t tickcount (in milliseconds) to get sysUptime, ok, it will overflow each ~49.7 days, but is it a real problem? Most of time, SNMP supervisors only display this information, but there's no processing on it...

Last, Solution #3, is perhaps to keep the actual sys_timeout solution, but with another period. We could call snmp_inc_sysuptime only each 1000ms (by example), write something like that:

void snmp_inc_sysuptime( u32_t inc)
{ sysuptime+=inc;
}

Less accurate, but less overhead...

Frédéric Bernon <fbernon>
Group Member
Sun 11 Mar 2007 10:37:20 PM UTC, comment #12: 

I think there are two alternatives:
1. to add get_arch_time_now() as described in #19167:
2. to add sys_arch_get_tickcount() and two functions (or macros) for conversation ticks to or from milliseconds.

The problem with the first approach is implementation of get_arch_time_now(), which should wrap correctly over its maximum value (as I decribed in #19167).

In the second case, we store timestamps in ticks, so converation ticks to milliseconds is performed only for timeout values (which are relatively small), so there is no problem with overflow when you calculate milliseconds by formula (ticks*M/N). And the tick counter wraps naturally without any effort (you just increment it by one in the timer interrupt). So for those who makes a port, #2 is simpler and more straightforward.


Dmitry Potapov <dpotapov>
Sun 11 Mar 2007 07:54:25 PM UTC, comment #11: 


> It was Kieran's idead to not commit everything to HEAD


Yes, that just I wanted to say. But you're right. If you want some help to test and check some code about multithread, "call" me, I will be please to spend time on DEVEL_SOCKET branch (you'are also right about the name :) ).




Frédéric Bernon <fbernon>
Group Member
Sun 11 Mar 2007 07:36:16 PM UTC, comment #10: 

Just wanted to have a "testing" branch, so that we only merge changes into HEAD if we are sure they work... I know DEVEL_SOCKET is a bad name...

It was Kieran's idead to not commit everything to HEAD and I must say it's the right thing, since HEAD is still kind of what we 'release' to the public, isn't it?

I wanted to check in the timestamp stuff from Dmitry in that branch, also. (as I said, DEVEL_SOCKET is a bad name.)

Simon Goldschmidt <goldsimon>
Group administrator
Sun 11 Mar 2007 06:26:37 PM UTC, comment #9: 

I think your idea to use another CVS branch for features like timestamps is good, but multithread stability and netconn api are more features to add to HEAD. Except, of course, if you use timestamps for that (is it?). But this is my point of view....

Can you tell me more about what you want to do in this new branch? I have an idea, from the bug# 19167, but, what else? (to avoid to do the same job twice...). In fact, the idea is the same than Dmity's sys_arch_time_now, but I don't have read all the subject (I do it now)...

>Oh, regarding coding style: I would favor a define in upper case letters..


Right, but it was to be closer that sys_ functions names...


Frédéric Bernon <fbernon>
Group Member
Sun 11 Mar 2007 06:03:38 PM UTC, comment #8: 

That's a good idea, but I would like to postpone that from CVS HEAD. I've made a new branch DEVEL_SOCKET, where I'm working on socket stability (multithreading stability, so netconn_ API also).

(I'd taken the DEVEL branch but that was out of date and I don't know if I can use it or if someone else is 'working' with that.)

I also want to include the timestamp thing in that branch, to test it without releasing it to HEAD. Maybe you can check it in there?

Oh, regarding coding style: I would favor a define in upper case letters...

Simon Goldschmidt <goldsimon>
Group administrator
Sun 11 Mar 2007 05:51:12 PM UTC, comment #7: 

To reduce timer overhead for snmp_get_sysuptime, I propose to write something like that :

in snmp.h :

#if (sys_get_tickcount==0)
void snmp_get_sysuptime(u32_t *value);
#else
#define snmp_get_sysuptime(t) sys_get_tickcount(t)
#endif /* (sys_get_tickcount==0) */

in mib2.c :

#if (sys_get_tickcount==0)
/**

  • Must be called at regular 10 msec interval from a timer interrupt
  • or signal handler depending on your runtime environment.

 */
void snmp_inc_sysuptime(void)
{
  sysuptime++;
}

void snmp_get_sysuptime(u32_t *value)
{
  *value = sysuptime;
}
#endif /* (sys_get_tickcount==0) */

// There is also a little patch to do in system_get_value to use the function and not directly sysuptime variable/


in opt.h :

#ifndef sys_get_tickcount
#define sys_get_tickcount 0
#endif

So, if we already get a target "tickcount", we can write in lwipopts.h :

#define sys_get_tickcount(t) sys_arch_get_tickcount(t)

It would be better (no overhead, only call when SNMP are process, and more accurate, because not based on sys_timeout).

Your comments?

Frédéric Bernon <fbernon>
Group Member
Wed 07 Mar 2007 09:40:04 AM UTC, comment #6: 

Ok, right, documentation is important.

If I provide some documentation "drafts", is there people ready to correct them? In a first time, I propose :

- "Getting Started lwIP with multithread operating system"
- "Tunning lwIP options"

All that, based on previous threads in forum...


Frédéric Bernon <fbernon>
Group Member
Wed 07 Mar 2007 09:33:52 AM UTC, comment #5: 

I think this is a problem that would be best solved with documentation rather than adding an extra function.  That way the developers will be able to make the right decision about which modules to initialise, and the documentation could include your suggested function as a sensible default.

Kieran Mansley <kieranm>
Group Member
Wed 07 Mar 2007 09:06:57 AM UTC, comment #4: 

Agree with you Kieran about functions called by lwip_init. In the current options availables, we can't know is we are using the tcpip.c thread. I think that adding a new option (yes, I know, another one), it will be possible to remove any extra code or initialization which are not used per rawapi users...

But, if I don't do any mistake, using tcpip.c need to use all that:

#if LWIP_STATS
stats_init ();
#endif /* STATS */
#if (NO_SYS == 0)
sys_init ();
#endif /* (NO_SYS == 0) */
mem_init ();
memp_init ();
pbuf_init ();
etharp_init ();
netif_init ();
lwip_socket_init();

lwip_socket_init don't have to be used for rawapi, but it seems that all others are need. Wrong?


Frédéric Bernon <fbernon>
Group Member
Wed 07 Mar 2007 09:00:04 AM UTC, comment #3: 

I think adding the lwip_init function might cause more problems than it solves, as many people wont want or need to initialise all the modules - particularly if using the raw API.

Kieran Mansley <kieranm>
Group Member
Wed 07 Mar 2007 08:58:26 AM UTC, comment #2: 

Ye,s agree with you, timeout timer are not a perfect feature, and, in this case, I will prefer to be able to remove "snmp_inc_sysuptime()" and the variable "sysuptime" and to be able to define my "own" function "snmp_get_sysuptime" (based on platform timer). So, overhead will be reduce.

About lwip_init() function, in a lwip.c file? Perhaps it will be usefull to add a define to tell is tcpip.c is used, to don't increase footprint if not necessary (sockets.c, api_lib.c, api_msg.c, tcpip.c). With that, it will give a pseudo-code like for tcpip.c users:

main()
{ TODO: Target specific init

  lwip_init();

  TODO: Add here network inferfaces with net_add

#if LWIP_SNMP
  snmp_set_sysdesr(...);
  snmp_set_syscontact(...);
  snmp_set_sysname(...);
  snmp_set_syslocation(...);
  etc...
#endif


  tcpip_init();

  TODO: Application


For RAW API users, only tcpip_init will have to be replace...

Frédéric Bernon <fbernon>
Group Member
Wed 07 Mar 2007 08:19:34 AM UTC, comment #1: 

Hm, I don't like the way SNMP gets it's sys_uptime anyway (since it produces high system load calling snmp_inc_sysuptime even if you have no client which reads it). AND it's not very accurate if called by an lwIP timeout timer (as stated in discussion to bug #19167).

If we do the timeouts using a timestamp, we can get the sys_up_time when the client reads it, that would be more accurate.

I agree with you we need an lwip_init() init function. But I think it would be better not to integrate it in tcpip.c. I think this should also be available to raw-only users. We could make the right initialization for both configurations.
Having a function like lwip_init() would definitively help new users!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 06 Mar 2007 07:50:13 PM UTC, original submission:  

With the very useful Christiaan Simons's SNMP Agent, we can add this protocol to our products.

I propose to help SNMP integration for new users like this :

#if LWIP_SNMP
static void
snmp_timer(void *arg)
{ snmp_inc_sysuptime();
  sys_timeout( SNMP_TMR_INTERVAL, snmp_timer, arg);
}
#endif /* LWIP_SNMP */

static void
tcpip_thread(void *arg)
{
  struct tcpip_msg *msg;

#if LWIP_SNMP
  snmp_init();
#endif /* LWIP_SNMP */

#if IP_REASSEMBLY
  sys_timeout( IP_TMR_INTERVAL, ip_timer, NULL);
#endif
  sys_timeout( ARP_TMR_INTERVAL, arp_timer, NULL);

#if LWIP_SNMP
  sys_timeout( SNMP_TMR_INTERVAL, snmp_timer, NULL);
#endif // LWIP_SNMP

//...

So, before call tcpip_init(), new users just have to call :

#if LWIP_SNMP
snmp_set_sysdesr(sysdescr_default,&sysdescr_len_default);
snmp_set_syscontact(syscontact_default,&syscontact_len_default);
snmp_set_sysname(sysname_default,&sysname_len_default);
snmp_set_syslocation(syslocation_default,&syslocation_len_default);
etc...
#endif

More, why don't integrate in tcpip.c a new "tool" function (with a name like "lwip_init" or other) which do :

void lwip_init()
{ #if LWIP_STATS
  stats_init      ();
  #endif /* STATS */
  #if (NO_SYS == 0)
  sys_init        ();
  #endif /* (NO_SYS == 0) */
  mem_init        ();
  memp_init       ();
  pbuf_init       ();
  etharp_init     ();
  netif_init      ();
  lwip_socket_init();
}


Frédéric Bernon <fbernon>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-05-23 fbernon Open/ClosedOpen Closed
    2007-05-23 fbernon StatusIn Progress Wont Do

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code