LWIP improvements Topic is solved

Use this forum for requesting small changes in ChibiOS. Large changes should be discussed in the development forum. This forum is NOT for support.
geebee
Posts: 8
Joined: Thu Dec 06, 2018 10:22 pm
Has thanked: 2 times
Been thanked: 11 times

LWIP improvements

Postby geebee » Thu Dec 05, 2019 4:40 pm

Hi,

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(&params.completion, 0);
+  tcpip_callback_with_block(do_reconfigure, &params, 0);
+  chSemWait(&params.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

User avatar
Giovanni
Site Admin
Posts: 12749
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 696 times
Been thanked: 573 times
Contact:

Re: LWIP improvements

Postby Giovanni » Tue Dec 10, 2019 9:37 am

Any comment about those changes by stakeholders?

Giovanni

steved
Posts: 671
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 8 times
Been thanked: 96 times

Re: LWIP improvements

Postby steved » Tue Dec 10, 2019 11:58 am

I've not looked at the detail of the code, but generally supportive of the changes, and have a few of my own.

1. Not personally bothered about the dynamic reconfiguration at present (although could be useful in the future), but I have found a need to do link up/down from external code (which is partly a similar problem). Is there any merit in having an ioctl-style interface, rather than individual calls? Possibly there aren't enough potential options to make it worthwhile. My version of this triggers an event in the processing thread, to avoid any possibility of inter-thread conflicts (although lwIp's synchronisation function probably avoids that anyway).

2.

Code: Select all

I also changed the addesses type from ip_addr_t to ip4_addr_t
- definitely

3. autoip - not really dug into this, but your comments make sense. I'm unclear whether autoip can be active at the same time as static or DHCP; another system I work on suggests it can, but my brief play with lwIp suggests it can't.

4. Make file - agree it needs better control. Personally I would prefer to see optional features specifically included, rather than excluding unwanted features. However I accept that this would break BC. (what features do people use?)
On the same subject, I prefer to reference a specific version of a library, to avoid uncontrolled changes to a project. I think this just affects the definition of LWIP_DIR

5.

Code: Select all

2) Would it be worth it to automatically pull in evttimer for lwip builds?
Don't think so - could cause confusion elsewhere.

6.

Code: Select all

it might be a good idea to set SYS_LIGHTWEIGHT_PROT by default
Yes

From my own driver:
7. Support for multicast in low_level_init (needs mods elsewhere as well):

Code: Select all

static void low_level_init(struct netif *netif) {
  /* set MAC hardware address length */
  netif->hwaddr_len = ETHARP_HWADDR_LEN;

  /* maximum transfer unit */
  netif->mtu = LWIP_NETIF_MTU;

  /* device capabilities */
  /* don't set NETIF_FLAG_ETHARP if this device is not an Ethernet one */
  netif->flags = NETIF_FLAG_BROADCAST | NETIF_FLAG_ETHARP | NETIF_FLAG_LINK_UP;
  #if LWIP_IGMP > 0
  /* Note: STM32_MAC_IGMP_ENABLE needs to be set to a value 1..3 in mcuconf.h */
  /* If we set the next flag, lwIp looks after everything else. The low-level
   * implementation may adopt varying degrees of cleverness in handling
   * multicast, according to the value of STM32_MAC_IGMP_ENABLE
   */
  netif->flags |= NETIF_FLAG_IGMP;
  #endif


#if LWIP_IPV6 && LWIP_IPV6_MLD
  /*
   * For hardware/netifs that implement MAC filtering.
   * All-nodes link-local is handled by default, so we must let the hardware know
   * to allow multicast packets in.
   * Should set mld_mac_filter previously. */
  if (netif->mld_mac_filter != NULL) {
    ip6_addr_t ip6_allnodes_ll;
    ip6_addr_set_allnodes_linklocal(&ip6_allnodes_ll);
    netif->mld_mac_filter(netif, &ip6_allnodes_ll, NETIF_ADD_MAC_FILTER);
  }
#endif /* LWIP_IPV6 && LWIP_IPV6_MLD */
  /* Do whatever else is needed to initialise interface. */
}


8. There is merit in setting ETH_PAD_SIZE true (possibly as a default) since it aligns the data buffers on 4-byte boundaries. Doing so highlights a couple of typing bugs in low_level_input():

Code: Select all

#if ETH_PAD_SIZE
    pbuf_header(*pbuf, -ETH_PAD_SIZE); /* drop the padding word */
#endif
(and another a few lines on)

9. Also in low_level_input(), brackets are needed round a pointer reference:

Code: Select all

    MIB2_STATS_NETIF_ADD(netif, ifinoctets, (*pbuf)->tot_len);


10. In the thread code, I've added:

Code: Select all

  MIB2_INIT_NETIF(&thisif, snmp_ifType_ethernet_csmacd, 100000);     // Assume fixed 100kbit/sec on the interface
immediately after the macStart() line

11. Just a comment; historically it was necessary to modify one of the lock operations in the lwIp code, because it was made from an interrupt context. Starting with lwIp 2.1, this doesn't appear to be necessary.

I've attached my full lwipthread.c
Attachments
lwipthread.7z
(5.67 KiB) Downloaded 17 times

geebee
Posts: 8
Joined: Thu Dec 06, 2018 10:22 pm
Has thanked: 2 times
Been thanked: 11 times

Re: LWIP improvements

Postby geebee » Wed Dec 11, 2019 5:48 am

Thank you for looking over the changes! Some replies/clarifications, numbers matching your points:

1. Just to clarify what I mean by dynamic reconfiguration, I'm working on firmwares for devices that get shipped out to customers which need to configure them for their network, which basically results in having the equivalent of this type of dialog https://cdnssl.ubergizmo.com/wp-content ... ip-add.jpg. I could not find a way to do it with the current interface, short of saving the settings to eeprom and resetting. If some way exists sorry for missing it, can you briefly explain how one can do that?

Re: callbacks, that's actually a pretty good idea, allowing people to experiment with other parts of LWIP that need actions on link up/down (e.g. v6 equivalent of what's already there).
In fact that's how I originally implemented this, with a small change to ChibiOS to allow callbacks, and the rest of the code outside of it. Since the callback did not really change much after the initial development, I integrated it in ChibiOS and submitted that since it was a bit simpler.

Re-integrating the way I had it, basically I optionally pass the callbacks to the initial configuration, and use some defaults otherwise. In lwipthread.h:

Code: Select all

@@ -232,6 +232,9 @@ typedef struct lwipthread_opts {
 #if LWIP_NETIF_HOSTNAME || defined(__DOXYGEN__)
   const char              *ourHostName;
 #endif
+
+  void (*linkUpCB)(void*);
+  void (*linkDownCB)(void*);
 } lwipthread_opts_t;
 
 typedef struct lwipreconf_opts {
@@ -244,6 +247,8 @@ typedef struct lwipreconf_opts {
 #ifdef __cplusplus
 extern "C" {
 #endif
+  void lwipDefaultLinkUpCB(void *p);
+  void lwipDefaultLinkDownCB(void *p);
   void lwipInit(const lwipthread_opts_t *opts);
   void lwipReconfigure(const lwipreconf_opts_t *opts);
 #ifdef __cplusplus


and then in lwipthread.c:

Code: Select all

@@ -270,7 +270,7 @@ static net_addr_mode_t addressMode;
 static ip4_addr_t ip, gateway, netmask;
 static struct netif thisif = { 0 };
 
-static void linkup_cb(void *p)
+void lwipDefaultLinkUpCB(void *p)
 {
   struct netif *ifc = (struct netif*) p;
   (void) ifc;
@@ -284,7 +284,7 @@ static void linkup_cb(void *p)
 #endif
 }
 
-static void linkdown_cb(void *p)
+void lwipDefaultLinkDownCB(void *p)
 {
   struct netif *ifc = (struct netif*) p;
   (void) ifc;
@@ -309,6 +309,8 @@ static THD_FUNCTION(lwip_thread, p) {
   event_listener_t el0, el1;
   static const MACConfig mac_config = {thisif.hwaddr};
   err_t result;
+  tcpip_callback_fn linkupCB = NULL;
+  tcpip_callback_fn linkdownCB = NULL;
 
   chRegSetThreadName(LWIP_THREAD_NAME);
 
@@ -329,6 +331,8 @@ static THD_FUNCTION(lwip_thread, p) {
 #if LWIP_NETIF_HOSTNAME
     thisif.hostname = opts->ourHostName;
 #endif
+    linkupCB = opts->linkUpCB;
+    linkdownCB = opts->linkDownCB;
   }
   else {
     thisif.hwaddr[0] = LWIP_ETHADDR_0;
@@ -346,6 +350,11 @@ static THD_FUNCTION(lwip_thread, p) {
 #endif
   }
 
+  if (!linkupCB)
+    linkupCB = lwipDefaultLinkUpCB;
+  if (!linkdownCB)
+    linkdownCB = lwipDefaultLinkDownCB;
+
 #if LWIP_NETIF_HOSTNAME
   if (thisif.hostname == NULL)
     thisif.hostname = LWIP_NETIF_HOSTNAME_STRING;
@@ -383,12 +392,12 @@ static THD_FUNCTION(lwip_thread, p) {
         if (current_link_status) {
           tcpip_callback_with_block((tcpip_callback_fn) netif_set_link_up,
                                      &thisif, 0);
-          tcpip_callback_with_block(linkup_cb, &thisif, 0);
+          tcpip_callback_with_block(linkupCB, &thisif, 0);
         }
         else {
           tcpip_callback_with_block((tcpip_callback_fn) netif_set_link_down,
                                      &thisif, 0);
-          tcpip_callback_with_block(linkdown_cb, &thisif, 0);
+          tcpip_callback_with_block(linkdownCB, &thisif, 0);
         }
       }
     }


The idea here is that if somebody already memsets the lwipthread_opts_t to zero before setting what needs to be set, the user code will still work without modifications after this change. The up/down callbacks are public so that it's easy to extend them in case someone wants to add some functionality, by calling the defaults and then performing the additional operations.
To get a ioctl-like interface, you could move linkupCB and linkdownCB to file scope, and provide the interface to change them out (making sure it's safe, and also modify the code to deal with setting to NULL).

3. From what I can tell, there is nothing that is specifically "static" in LWIP, just API methods to change address, gateway, netmask. When you run AUTOIP or DHCP, that code calls the same API methods to set those addresses as needed, so it would override any setting that you previously made, and vice-versa if you call those methods after DHCP configures the addresses, you would override its settings. So really static is just calling those APIs manually and ensuring that nothing else is calling them with different addresses behind your back.

In LWIP, AUTOIP and DHCP can coexist "at the same time" by enabling both LWIP_AUTOIP and LWIP_DHCP, and then defining LWIP_DHCP_AUTOIP_COOP to 1. If you do that, I believe you can still use AUTOIP by itself as we do here, but dhcp_start() and dhcp_stop() will basically use AUTOIP if DHCP cannot successfully get an address (so the changes I proposed would still work, except that DHCP mode will get AUTOIP fallback). So that's why I say "at the same time": it's really only one or the other, but LWIP's code will automatically handle which one.

Not sure if it's possible in LWIP to have DHCP falling back to static, my guess would be no (without changing anything that is). And finally, I would think static and AUTOIP together doesn't make too much sense in practice, because the only way you would fall back to static would be if the entire AUTOIP address space is exhausted, so I'd be surprised if a lightweight stack cares about that extremely unlikely edge case.

4. Absolutely. To be clear my proposed change does not require you to take things away, it just provides a default for LWSRC_EXTRAS if not specified so that a project that builds against the current ChibiOS will still build after picking up the proposed change without needing any modification. If breaking that is fine for you guys, I'm all for removing the default. Removed from my changes.

5. Ok, I'm fine with that. I don't really know how that is used in people's projects, I just noticed that within ChibiOS evtimer is only used for LWIP. Removed from my changes.

7. That will be very nice, can't wait! In particular the MAC filtering done right. So far I have been simply disabling all filtering with (on STM32F7)

Code: Select all

  ETH->MACFFR |= ETH_MACFFR_PAM;

but actually using the hardware filters would be great!


Finally, I didn't realize I should attach the changes in a zipped file. Here they are, with the things proposed in the first post and the changes from your feedback. Feel free to make change as you see fit.

Thanks again for looking into this,

GB
Attachments
lwip_proposals.7z
(6.8 KiB) Downloaded 9 times

steved
Posts: 671
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 8 times
Been thanked: 96 times

Re: LWIP improvements - Multicasting

Postby steved » Fri Dec 13, 2019 2:50 pm

Suspect there are going to be some long responses in this thread! Anyway, to pick up on multicasting first,
geebee wrote:7. That will be very nice, can't wait! In particular the MAC filtering done right. So far I have been simply disabling all filtering with (on STM32F7)

Code: Select all

  ETH->MACFFR |= ETH_MACFFR_PAM;

but actually using the hardware filters would be great!

At present I've only implemented the 'All' mode of multicasting, but allowed to specify the other options (basically by enabling IGMP):

Code: Select all

#define STM32_MAC_IGMP_ENABLE               1             /* added flag   [0 = NONE| 1 = ALL| 2 = FILT | 3 = HASH] */
I've attached my modified MAC driver, which makes provision for all those options.

I've found what looks to be a very competent implementation of hash-based filtering here, but not got around to porting it yet. Two things to consider:
1. Where should the code go? In the short term it could go in the hal_mac-lld.c; in the medium term quite possibly it should be split between the LLD and the higher level driver code. This is likely to be triggered by the need to support another MAC.
2. I already have my own crc32 library routine outside ChibiOs, so would want to use that. (And I also want to have the option of using the CRC generator peripheral that's on the more powerful ST device).


geebee wrote:Finally, I didn't realize I should attach the changes in a zipped file.

Certainly not compulsory; more a personal preference. Partly that I find it easier to actually see the changes (Winmerge on old and new files); partly that I don't think I've ever successfully applied a patch file! (Other than with a little manual decoding)
Attachments
hal_mac_lld.zip
(6.31 KiB) Downloaded 8 times

User avatar
Giovanni
Site Admin
Posts: 12749
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 696 times
Been thanked: 573 times
Contact:

Re: LWIP improvements

Postby Giovanni » Fri Dec 13, 2019 2:59 pm

Just a note, HAL MAC driver is not lwIP-specific, it can be used with other stacks, putting lwIP-specific code in there is not a good idea IMHO.

Anyway, there potential for a lot of enhancements, we need a synthesis and start introducing changes one by one so everybody can be synchronized going ahead, so far we have several mutually exclusive proposals.

Giovanni

steved
Posts: 671
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 8 times
Been thanked: 96 times

Re: LWIP improvements

Postby steved » Fri Dec 13, 2019 7:13 pm

Giovanni wrote:Just a note, HAL MAC driver is not lwIP-specific, it can be used with other stacks, putting lwIP-specific code in there is not a good idea IMHO.

Understood (and agree). Specifically on multicast, IIRC much of the code is generic, with some hardware-specific. Any lwip-specific code I'd be inclined to add to the lwip_bindings directory. Presumably hal_mac.c would be the place for generic code in modest amounts?

Giovanni wrote:Anyway, there potential for a lot of enhancements, we need a synthesis and start introducing changes one by one so everybody can be synchronized going ahead, so far we have several mutually exclusive proposals.
Working on it! There are others here who clearly use lwIp - would be nice to have their input, even if its to say that the current implementation does all they need.

User avatar
Giovanni
Site Admin
Posts: 12749
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 696 times
Been thanked: 573 times
Contact:

Re: LWIP improvements

Postby Giovanni » Fri Dec 13, 2019 9:15 pm

A set of incremental patches would be great, lets discuss step by step. I am not a believer in big changes, got burned too often in the past with that.

Giovanni

geebee
Posts: 8
Joined: Thu Dec 06, 2018 10:22 pm
Has thanked: 2 times
Been thanked: 11 times

Re: LWIP improvements

Postby geebee » Wed Dec 18, 2019 7:52 pm

I looked over the code submitted by steved, and I think it's almost entirely orthogonal to what I have submitted, so I don't think it makes sense to merge them into a single commit. Furthermore, unless there is more code/more recent versions of hal_mac and lwipthread, it seems that those changes are not very close to being done.

Some notes on the ideas from that code:

The natural place to put MAC filtering IMO is indeed in hal_mac. What you need to do is to add an API to add and remove addresses (so basically two methods, say macAddAddress and macRemoveAddress), and then use said API in the lwip_bindings, where you would implement and add to the interface a mac filter which uses the HAL api to add/remove filters.

That way, you can start off just with empty methods and uing PAM in MACFFR, and later add the rest. I would structure the abstraction differently though. First of all, at this level of abstraction this is just MAC filtering, using IGMP in the constants (STM32_MAC_IGMP_ENABLE) is misleading because you would do mac filtering for IPv6 MLD as well, and in general for whatever else you might need (unlikely, but you could roll your own link layer protocol too). But most of all, I'm not sure there is any usefulness in exposing the register configutation like that. At most, I can see a distinction between forcing to use PAM, and doing real filtering, for cases of desperation where nothing is working and you just want to pass through all the packets to at least make sure the issue it's not there when debugging. Otherwise, I think it should be the job of the driver in hal_mac_lld to select the most appropriate form of filtering, i.e. perfect filtering until you have more than 3 addresses, then switch to hashing, and revert back to perfect if enough filters are removed to go back to 3 addresses or less.

The code does not seem to have anything regarding the mentioned ioctl interface, but I feel that callbacks are a more consistent interface with the rest of ChibiOS, while ioctl would seem more in line with a certain other embedded OS (which starts in N and ends in X).


One possible way to break things into smaller chunks would be to break out the small changes from my original post (points 1-4) and the small changes from steved post, and commit those first:
- Makefile changes: the original change I proposed requires no changes in current projects, whereas the one implemented after feedback would require adding HTTPD. I don't particularly care one way or another, or even a third way, as long as there is a way to specify which additional LWIP modules to use. Fine to leave evtimer as it is now.
- changes to static options
- steved bugfixes for ETH_PAD_SIZE and MIB2_STATS_NETIF_ADD
- probably steved point #10, although I would add a method to hal_mac to return the link speed (via MACCR FES bit on STM32), initialize it to zero, and update it in linkup/linkdown using said method, so maybe this makes sense to do after the linkup/linkdown changes.

After those, initialization fixes and reconfiguration (which are ready to go, modulo more comments you guys might have), and then you guys can work on MAC filtering at your leisure (and I don't mind reviewing when it's done if it's helpful).

Thank you,

GB

steved
Posts: 671
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 8 times
Been thanked: 96 times

Re: LWIP improvements

Postby steved » Thu Dec 19, 2019 1:37 pm

geebee, You're right that there's a lot to be done to my code - I've been talking intentions rather than the actualite! I'm also no networking expert. You've probably deduced that currently I've just done what's needed to get my projects working.

I like your suggestion for handling multicast. If it has a flag to turn it on, this should set all the other required flags for lwIp and so on. Not sure that its necessary to be able to disable it though; maybe see how much extra code is involved before making a final decision. For ST at least, devices with network ports have relatively generous amounts of code space, so might not be an issue.

I've got no strong feelings about an ioctl-style interface or individual routines; I find the ioctl style useful where there are lots of infrequent things you might want to do - and it helps debugging because potentially you only need set on breakpoint at the entrance to the handler, rather than multiple breakpoints on individual routines. In the specific case of multicast, I don't see any problem in having a couple of entry points for adding and removing an address - although that could possibly come down to a single entry with an "add/remove" flag.

Happy with your list of "Phase 1" items.

On point #10, no idea why that was added - must have got it from somewhere! Maybe it was necessary for the SNMP MIB-II Stuff to work properly.


On a related topic, has anyone looked at alternatives to lwIP at all? A couple of the recently reported bugs have concerned me, especially as one might be responsible for an issue I'm struggling to pin down. lwIP is quite a mature product, at least at the core, so I wouldn't have expected this type of bug to be lingering. And the dev team is clearly very resource-limited, so bug fixing is usually a very slow process. FreeRTOS have done their own equivalent to lwIP - they used to support lwIP, so maybe they didn't like what they saw! It's an MIT licence, and has some commercial input, so might be worth looking at.


Return to “Small Change Requests”

Who is online

Users browsing this forum: No registered users and 4 guests