I've been using ChibiOS+LWIP for a couple of years, and I've been making some changes which I think might be useful in general, so here they are for your consideration to integrate them in ChibiOS.
The main feature that is missing for me from the current LWIP bindings is some way to reconfigure things dynamically. The use case would be to have an interface to switch the device on the fly between the various modes (AUTOIP/DHCP/static IP) without requiring to build a different firmware. To do so, one solution I've been using is to add another method:
Code: Select all
--- a/os/various/lwip_bindings/lwipthread.h
+++ b/os/various/lwip_bindings/lwipthread.h
@@ -234,10 +234,18 @@ typedef struct lwipthread_opts {
#endif
} lwipthread_opts_t;
+typedef struct lwipreconf_opts {
+ uint32_t address;
+ uint32_t netmask;
+ uint32_t gateway;
+ net_addr_mode_t addrMode;
+} lwipreconf_opts_t;
#ifdef __cplusplus
extern "C" {
#endif
void lwipInit(const lwipthread_opts_t *opts);
+ void lwipReconfigure(const lwipreconf_opts_t *opts);
#ifdef __cplusplus
}
#endif
And then restructure a bit things in lwipthread.c. First of all, bring the relevant variables out at the file scope from the thread function, and add linkup/down callbacks to enable/disable autoip/dhcp:
Code: Select all
@@ -266,6 +266,38 @@ static err_t ethernetif_init(struct netif *netif) {
return ERR_OK;
}
+static net_addr_mode_t addressMode;
+static ip4_addr_t ip, gateway, netmask;
+static struct netif thisif = { 0 };
+
+static void linkup_cb(void *p)
+{
+ struct netif *ifc = (struct netif*) p;
+ (void) ifc;
+#if LWIP_AUTOIP
+ if (addressMode == NET_ADDRESS_AUTO)
+ autoip_start(ifc);
+#endif
+#if LWIP_DHCP
+ if (addressMode == NET_ADDRESS_DHCP)
+ dhcp_start(ifc);
+#endif
+}
+
+static void linkdown_cb(void *p)
+{
+ struct netif *ifc = (struct netif*) p;
+ (void) ifc;
+#if LWIP_AUTOIP
+ if (addressMode == NET_ADDRESS_AUTO)
+ autoip_stop(ifc);
+#endif
+#if LWIP_DHCP
+ if (addressMode == NET_ADDRESS_DHCP)
+ dhcp_stop(ifc);
+#endif
+}
+
/**
* @brief LWIP handling thread.
*
I also changed the addesses type from ip_addr_t to ip4_addr_t, which allows one to build this code with both IPv4 and IPv4+IPv6.
I'm pretty sure dhcp_start/stop must be called indirectly via a callback in the tcpip_thread, because it makes use of methods that are not thread safe.
I'm also pretty sure autoip should be stopped/restarted in case the link goes down, because in the meantime another device could have been connected and picked the same IP address.
Second, the above changes need to be integrated into the thread function, with a couple more fixes:
Code: Select all
@@ -275,10 +307,7 @@ static err_t ethernetif_init(struct netif *netif) {
static THD_FUNCTION(lwip_thread, p) {
event_timer_t evt;
event_listener_t el0, el1;
- ip_addr_t ip, gateway, netmask;
- static struct netif thisif = { 0 };
static const MACConfig mac_config = {thisif.hwaddr};
- net_addr_mode_t addressMode;
err_t result;
chRegSetThreadName(LWIP_THREAD_NAME);
@@ -332,20 +361,8 @@ static THD_FUNCTION(lwip_thread, p) {
osalSysHalt("netif_add error"); // Not sure what else we can do if an error occurs here.
};
- netif_set_default(&thisif);
-
- switch (addressMode)
- {
-#if LWIP_AUTOIP
- case NET_ADDRESS_AUTO:
- autoip_start(&thisif);
- break;
-#endif
-
- default:
- netif_set_up(&thisif);
- break;
- }
+ netifapi_netif_set_default(&thisif);
+ netifapi_netif_set_up(&thisif);
/* Setup event sources.*/
evtObjectInit(&evt, LWIP_LINK_POLL_INTERVAL);
@@ -366,18 +383,12 @@ static THD_FUNCTION(lwip_thread, p) {
if (current_link_status) {
tcpip_callback_with_block((tcpip_callback_fn) netif_set_link_up,
&thisif, 0);
-#if LWIP_DHCP
- if (addressMode == NET_ADDRESS_DHCP)
- dhcp_start(&thisif);
-#endif
+ tcpip_callback_with_block(linkup_cb, &thisif, 0);
}
else {
tcpip_callback_with_block((tcpip_callback_fn) netif_set_link_down,
&thisif, 0);
-#if LWIP_DHCP
- if (addressMode == NET_ADDRESS_DHCP)
- dhcp_stop(&thisif);
-#endif
+ tcpip_callback_with_block(linkdown_cb, &thisif, 0);
}
}
}
If LWIP_AUTOIP is enabled, autoip_start was called on an interface instead of netif_set_up. This always fails. There seems to be a misleading comment in the LWIP source that suggests to call autoip_start after netif_add, but in fact autoip_start checks that the interface is up:
Code: Select all
LWIP_ERROR("netif is not up, old style port?", netif_is_up(netif), return ERR_ARG;);
so I couldn't get the current code to start up properly for AUTOIP. The change above should fix that, moving autoip start/stop at the same level as DHCP.
Another issue I think is that one shouldn't call netif_ methods outside the tcpip thread. To do that safely there are the netifapi_ methods, which I switched to (note: netifapi was already used for netif_add a few lines above).
And then as mentioned above, I think also the dhcp_ methods need to be called from the tcpip thread, so now they are called from the callbacks.
In principle, if DHCP and AUTOIP are both disabled, the whole code for the linkup/linkdown callbacks could be removed via preprocessor directives, although it doesn't add much overhead.
Finally, there is the implementation of the reconfiguration method:
Code: Select all
+typedef struct lwip_reconf_params {
+ const lwipreconf_opts_t *opts;
+ semaphore_t completion;
+} lwip_reconf_params_t;
+
+static void do_reconfigure(void *p)
+{
+ lwip_reconf_params_t *reconf = (lwip_reconf_params_t*) p;
+
+ /* Here we could be smart about only changing what needed, but just
+ change out everything every time because it should not happen often
+ and if something is not working it might be useful to reset all state. */
+
+ switch (addressMode) {
+#if LWIP_DHCP
+ case NET_ADDRESS_DHCP: {
+ if (netif_is_up(&thisif))
+ dhcp_stop(&thisif);
+ break;
+ }
+#endif
+ case NET_ADDRESS_STATIC: {
+ ip4_addr_t zero = { 0 };
+ netif_set_ipaddr(&thisif, &zero);
+ netif_set_netmask(&thisif, &zero);
+ netif_set_gw(&thisif, &zero);
+ break;
+ }
+#if LWIP_AUTOIP
+ case NET_ADDRESS_AUTO: {
+ if (netif_is_up(&thisif))
+ autoip_stop(&thisif);
+ break;
+ }
+#endif
+ }
+
+ ip.addr = reconf->opts->address;
+ gateway.addr = reconf->opts->gateway;
+ netmask.addr = reconf->opts->netmask;
+ addressMode = reconf->opts->addrMode;
+
+ switch (addressMode) {
+#if LWIP_DHCP
+ case NET_ADDRESS_DHCP: {
+ if (netif_is_up(&thisif))
+ dhcp_start(&thisif);
+ break;
+ }
+#endif
+ case NET_ADDRESS_STATIC: {
+ netif_set_ipaddr(&thisif, &ip);
+ netif_set_netmask(&thisif, &netmask);
+ netif_set_gw(&thisif, &gateway);
+ break;
+ }
+#if LWIP_AUTOIP
+ case NET_ADDRESS_AUTO: {
+ if (netif_is_up(&thisif))
+ autoip_start(&thisif);
+ break;
+ }
+#endif
+ }
+
+ chSemSignal(&reconf->completion);
+}
+
+void lwipReconfigure(const lwipreconf_opts_t *opts)
+{
+ lwip_reconf_params_t params;
+ params.opts = opts;
+ chSemObjectInit(¶ms.completion, 0);
+ tcpip_callback_with_block(do_reconfigure, ¶ms, 0);
+ chSemWait(¶ms.completion);
+}
+
/** @} */
The method itself just stops whatever needs to be stopped depending on the current mode, and then starts whatever needs to be started depending on the new mode, wrapped in a callback to be run in the tcpip thread.
One complication with this is to ensure the validity of the reconfiguration options, since callbacks via tcpip_callback_with_block can only take a pointer to void*, and blocks only waiting to post the callback to the thread queue, not until the callback is processed. Here I just opted to use a semaphore to block the calling thread until the request is processed since my projects always use semaphores, but there are other ways of doing that if that's not ok to add this dependency.
Then I have a few more small changes:
1) Especially after the improved make system was introduced (r11049), pulling in lwip.mk will make some hard decisions about what files to build. In my case, I do not need HTTP, but lwip.mk will force me to provide an httpd_opts.h to build a firmware, so I have to duplicate the whole lwip.mk with modifications.
There is currently no way to override the settings, because
Code: Select all
LWSRC = ...
...
ALLCSRC += $(LWSRC)
is equivalent to
Code: Select all
LWSRC = ...
...
ALLCSRC := $(ALLCSRC) $(LWSRC)
so LWSRC gets assigned in lwip.mk and gets immediately evaluated after and assigned to ALLCSRC, leaving no way to override it before or after.
My suggestion would be to make LWSRC a combination of required + optional modules, with the latter defaulting to HTTPDFILES if not overridden, so no changes are needed for currently working httpd projects:
Code: Select all
LWSRC_REQUIRED = $(COREFILES) $(CORE4FILES) $(APIFILES) $(LWBINDSRC) $(NETIFFILES)
LWSRC_EXTRAS ?= $(HTTPDFILES)
...
ALLCSRC += $(LWSRC_REQUIRED) $(LWSRC_EXTRAS)
This way, an existing project will still work pulling in lwip.mk without modifications, but if somebody needs different files to be built, e.g. no httpd and mdns, they can do this:
Code: Select all
LWSRC_EXTRAS = $(MDNSFILES)
include $(CHIBIOS)/os/various/lwip_bindings/lwip.mk
and just use the same lwip.mk file.
2) Would it be worth it to automatically pull in evttimer for lwip builds? I.e. change the above to
Code: Select all
ALLCSRC += $(LWSRC_REQUIRED) $(LWSRC_EXTRAS) $(CHIBIOS)/os/various/evtimer.c
As far as I can tell, lwip is the only user of evttimer within ChibiOS. Although this change would require to modify all the makefiles that explicitly pull it, so maybe it's not worth the hassle.
3) In os/various/lwip_bindings/static_lwipopts.h, it might be a good idea to set SYS_LIGHTWEIGHT_PROT by default:
Code: Select all
-#define SYS_LIGHTWEIGHT_PROT 0
+#define SYS_LIGHTWEIGHT_PROT 1
The reason to use it is that without it, some of the thread safe APIs are still unsafe. For example, anything using netconn (directly, or via using sockets) will allocate and free netbufs both in the thread being used and the tcpip_thread via memp_malloc() and memp_free(), which themselves are not safe to use without SYS_LIGHTWEIGHT_PROT enabled.
Note that not having protection works fine with the httpd examples because they rely on the tcp callback API, which runs the callbacks in the tcpip_thread.
This issue was discussed previously viewtopic.php?t=3855 and seemed to be solved in that context, but I find that the simple use case of using sockets in a user thread eventually (given enough traffic and time) triggers the race condition that user thread and tcpip_thread end up working on the netbuf pool at the same time with consequent corruption and hard to diagnose crashes.
In the context of the previous bug report, I believe the reason it does not trigger is because the lwip_thread runs at LOWPRIO, and since it's the only source of packets to the tcpip_thread (at higher priority), it is guaranteed that it won't allocate a pbuf until after the previous pbuf has been freed so that there is no corruption of the mempool.
The protection is really fast (implemented in os/various/lwip_bindings/arch/sys_arch.c via chSysGetStatusAndLockX() and chSysRestoreStatusX()), so it seems that it would be a better default to err on the side of safety, and let advanced users that need to gain a fraction of a percentage in performance to disable it if needed.
4) Again in os/various/lwip_bindings/static_lwipopts.h, LWIP_COMPAT_MUTEX_ALLOWED is only checked for definition in lwip, so the current code is a bit deceiving in that if the value is changed to 0, the option is still enabled. I suggest changing to
Code: Select all
-#define LWIP_COMPAT_MUTEX_ALLOWED 1
+#define LWIP_COMPAT_MUTEX_ALLOWED
I hope they can be useful,
GB