Mon 26 Mar 2007 04:19:45 PM UTC, comment #12:
Ok, I commit the new LWIP_ARP option. Note for developers: I don't have disable directly code in etharp.h/.c because some modules call them, and I don't know what should be a good behavior for them :
- snmp/mib2.c for ip_ntomentry_get_value() and atentry_get_value(),
- netif.c for netif_set_ipaddr() (in fact, the actual code for "gratuitous ARP" is disable).
- dhcp.c for dhcp_check()
I close this item, but reopen it if there is something wrong for you...
|
Mon 26 Mar 2007 01:52:26 PM UTC, comment #11:
Ok, I will call it from tcpip_init. It's also because others init (ip_init, udp_init & tcp_init) were moved from tcpip_thread to tcpip_init (I thought it solve some others developers problems, and more, it help to see which init functions use timers, and which don't use them).
Good, I will do like this...
|
Mon 26 Mar 2007 01:45:47 PM UTC, comment #10:
Re LWIP_ARP: fair enough.
Re etharp_init: yes calling from tcpip_init is absolutely fine. In comment 5, you referred to the sample code in comment 1, which has etharp_init being called from the port-specific initialisation code, not from tcpip_init.
But if you're happy with calling it from tcpip_init, that's fine with me - it's essentially equivalent to calling it from the start of tcpip_thread anyway.
|
Mon 26 Mar 2007 01:35:15 PM UTC, comment #9:
Because ARP can also be used with Token Ring (and even if this protocol is not really supported now), I think that LWIP_ARP would be better. Ok to set it to 1 as default value.
About etharp_init(), I don't understand why you want to call it inside tcpip_thread? It's just a table initialisation, no timer is initialized inside this function... So, why not like in comment #1, or why not there :
void
tcpip_init(void (* initfunc)(void ), void arg)
{
#if LWIP_ARP
etharp_init();
#endif
ip_init();
#if LWIP_UDP
udp_init();
#endif
#if LWIP_TCP
tcp_init();
#endif
tcpip_init_done = initfunc;
tcpip_init_done_arg = arg;
mbox = sys_mbox_new();
sys_thread_new(tcpip_thread, NULL, TCPIP_THREAD_PRIO);
}
I just want to understand your point of view (perhaps I miss something)...
|
Mon 26 Mar 2007 01:19:21 PM UTC, comment #8:
I think we may as well have the default as enabled (1) as it is the common case, was sort of already the default (as long as the user built the etharp module) and I guess one of the underlying netif interfaces should be enabled by default, so it can be that one. Just my opinion.
Re comment 5: I think LWIP_ARP is fine, although LWIP_ETHARP might be clearer and more in line with the filename. Your choice :-).
If we have a LWIP_ARP define, then etharp_init() can be called from tcpip_thread() before the call to tcpip_init_done(), but only #if LWIP_ARP.
Note, this would need a documentation/Changelog update (and changes to the contributed ports) because the port will now no longer call etharp_init itself. But this is an improvement - simplifying ports is good.
|
Mon 26 Mar 2007 01:17:39 PM UTC, comment #7:
Yes, I think ARP should be optional code.
|
Mon 26 Mar 2007 01:06:58 PM UTC, comment #6:
Is it ok for others to add LWIP_ARP in opt.h to disable ARP code if you only use PPP&SLIP (and to set it to 0 par default)?
|
Fri 23 Mar 2007 08:18:33 AM UTC, comment #5:
For arp_timer, I will prefer a LWIP_ARP set to 0 in opts.h to match to other options (their names are LWIP_UDP, LWIP_TCP, ....).
etharp_init is another problem. It have to be call with other init function (like a sample is proposed in comment #1).
About idea from comment #1, where we have talk to initialize arp_timer from tcpip_init_done, to my point of view, if it's always possible, it is not a good solution to keep lwIP simple.
It's good for us that you use PPP & SLIP, because it doesn't seem that lot of active developpers can help us on this part.
Before change it with a LWIP_ARP option, I propose you to wait comments from other active developers. Agree for you?
|
Fri 23 Mar 2007 07:04:50 AM UTC, comment #4:
This patch is incorrect. PPP and SLIP users without ethernet interfaces should not have the ARP timer (and probably won't be building netif/etharp.c in the first place). That's why in the mail thread on lwip-users I was saying it would happen in the port's tcpip_init_done callback because the port knows whether there are any ethernet interfaces, and tcpip.c does not.
Either it should be left to the port, or there should be something like a new LWIP_ETH (or ETH_SUPPORT?) feature test macro in opt.h (overriden by the port's lwipopts.h), which can be used to make the ARP timer init conditional.
I think adding something like LWIP_ETH would make things simpler overall.
Separately, on the move of etharp_init to tcpip_init_done, I would think that if this change stays, it's important that this new requirement for ports is highlighted (and the example ports in contrib/ should be updated). But if we did have the ability to test for LWIP_ETH, then we could add etharp_init to tcpip_init.
|
Mon 05 Mar 2007 11:30:48 AM UTC, comment #3:
With these patchs, I think that ARP initialization and use is correct, except for driver. See some solutions in forum's thread "the ARP layer isnotprotectedagainstconcurrentaccess" (I will post a patch about that). If it's ok for others, I will integrate in CVS....
(file #12113, file #12114)
|
Sat 03 Mar 2007 11:51:27 AM UTC, comment #2:
I tend to agree with Frederic, and would remove the arp initialization from the ethernetif skeleton, in favor of sticking it in the tcpip_init_done() block.
|
Thu 01 Mar 2007 08:52:30 AM UTC, comment #1:
I think that etharp_init() would have to be call before any net_add, because ARP Table is a global resource. Initialization sequence should be :
#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();
//...
netif_add( ...);
//...
About the arp_timer, I proposed yesterday in the forum's thread "Timers & lwip tasks (a multithreadquestionwithLwIP)" to initialize it in the tcpip_init_done (if you use tcpip_thread).
|
Thu 01 Mar 2007 07:30:02 AM UTC, original submission:
The ethernetif_init skeleton executes the following code:
ethernetif->ethaddr = (struct eth_addr *)&(netif->hwaddr[0]);
low_level_init(netif);
etharp_init();
sys_timeout(ARP_TMR_INTERVAL, arp_timer, NULL);
return ERR_OK;
}
When multiple network interfaces are instantiated, usually via the netif_add routine, the etharp_init is executed, and the arp timer start for each instantiation. This is not desired behaviour, and results in multiple instances of the arp_timer timer, which is bad, and causes the arp_timer function to be run more frequently than expected.
I've attached a patch file that wraps the arp initialization such that it is executed at most, only one time.
|