Add nic.ipconfig for other ports (like esp32, stm32 and others) #14199

felixdoerre · 2024-03-27T22:03:08Z

In order to make network configuration extensible and consistent across ports, we implemented ipconfig for the other implemented nics:

extmod/network_ninaw10.c -> nina_ifconfig
ports/cc3200/mods/modwlan.c -> sl_NetCfgSet
ports/esp32/network_lan.c --> esp-netif
ports/esp32/network_ppp.c --> (old?) lwip, can only be used to set dns.
ports/esp32/network_wlan.c --> esp-netif (setting the IP in CIDR notation requries more work)
ports/esp8266/network_wlan.c -> esp-netif, copy of the esp32 code(?)
ports/mimxrt/network_lan.c -> lwip
ports/stm32/network_lan.c -> lwip

robert-hh · 2024-03-28T07:00:53Z

@felixdoerre Thank you for your work. I had started yesterday as well with changes. See https://github.com/robert-hh/micropython/tree/ports_ipconfig
There, only STM32 and MIMXRT are included, but the changes are the same as your's. I've tested these and they work fine. I will test the ESP32 implementation. The one I'm currently working on is NINAW10. Since you did not touch that yet, I can continue. I scratched my head a little bit to understand your code's logic.
About the NINAW10: As far as I can tell, the NINAW10 support cannot change the DHCP mode. DHCP is always enabled. I'll finish the NINAW10 version today.
b.t.w.: your comment (setting the IP in CIDR notation requires more work) applies at all ports.

robert-hh · 2024-03-28T07:17:54Z

Tested your changes for MIMXRT (MIMXRT1020_EVK) and STM32 (LAN: PYBD_SF6 with DP83848, WiFi: Built-in CYW43) and they work, as expected. I did not test other boards because the changes do not touch board-specific features.

robert-hh · 2024-03-28T07:47:15Z

Tested a little bit more.
ESP32:
The ESP32 port builds and I can query parameters.

Setting e.g. addr4 failed.

>>> lan.ipconfig(addr4="10.0.0.144/16")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can't specify pos and kw args

and

>>> lan.ipconfig(addr4=("10.0.0.144", "255.255.255.0"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can't specify pos and kw args

network.ipconfig() is missing.

MIMXRT & STM32:

Setting addr4 using CIDR notation failed:

>>> lan.ipconfig(addr4="10.0.0.216/24")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: invalid arguments

ports/esp32/network_common.c

robert-hh · 2024-03-28T11:11:34Z

I have updated my branch with the implementation for NINAW10. It includes code for network.ipconfig(), even if the effect with NINAW10 is unclear, which may allow to set the DNS, but it cannot be read back from the module.

ports/esp32/network_common.c

robert-hh · 2024-03-28T12:19:09Z

The following change fixes the bug in using the CIDR notation for lwip based stacks:

--- a/extmod/network_lwip.c
+++ b/extmod/network_lwip.c
@@ -303,6 +303,7 @@ mp_obj_t mod_network_nic_ipconfig(struct netif *netif, size_t n_args, const mp_o
                                     to_copy = split - addr_str;
                                 }
                                 memcpy(plain_ip, addr_str, to_copy);
+                                plain_ip[to_copy] = '\0';
                                 mp_obj_t prefix_obj = mp_parse_num_integer(split + 1, strlen(split + 1), 10, NULL);
                                 prefix_bits = mp_obj_get_int(prefix_obj);
                             }
@@ -310,7 +311,7 @@ mp_obj_t mod_network_nic_ipconfig(struct netif *netif, size_t n_args, const mp_o
                                 uint32_t mask = -(1u << (32 - prefix_bits));
                                 ip_addr_set_ip4_u32_val(netmask, ((mask & 0xFF) << 24) | ((mask & 0xFF00) << 8) | ((mask >> 8) & 0xFF00) | ((mask >> 24) & 0xFF));
                             }
-                            if (!ipaddr_aton(addr_str, &ip_addr)) {
+                            if (!ipaddr_aton(plain_ip, &ip_addr)) {
                                 mp_raise_ValueError(MP_ERROR_TEXT("invalid arguments"));
                             }
                             if ((mp_obj_str_get_qstr(args[0]) == MP_QSTR_addr6) != IP_IS_V6(&ip_addr)

codecov · 2024-03-28T13:10:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (35e8d18) to head (64a24f4).
Report is 25 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #14199   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21200    21200           
=======================================
  Hits        20860    20860           
  Misses        340      340

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-03-28T13:14:11Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:   +20 +0.005% PYBV10
     mimxrt:   +24 +0.007% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

robert-hh · 2024-03-28T13:20:02Z

I updated the NINAW10 commit a few minutes ago. So you might not have taken the right version. Better wait a while, usually until the next day.
Edit: Sorry, you seem to have the last version.

robert-hh · 2024-03-28T13:23:16Z

OK. What's next. I could either continue with ESP8266 or CC3200.

felixdoerre · 2024-03-28T13:19:43Z

extmod/network_ninaw10.c

+
+        switch (mp_obj_str_get_qstr(args[1])) {
+            case MP_QSTR_dhcp4: {
+                return mp_const_true;


If I'm reading this (https://github.com/adafruit/nina-fw/blob/47875b775aee74c5f948ef2b955c9786b023a89e/arduino/libraries/WiFi/src/WiFi.cpp#L297) correctly, the ninaw10 has a dhcp client enabled, but disables it once you set a static IP (i.e. once you call NINA_CMD_SET_IF_CONFIG). And then it would need to be re-initialized to activate the dhcp client again. Maybe this behavior is something we should reflect here?

Otherwise I would argue that we should remove this keyword to indicate that the nic-driver "does not know".

I can do that. @iabdalkader since you made that driver, do you have more information?

And then it would need to be re-initialized to activate the dhcp client again. Maybe this behavior is something we should reflect here?

That would require some static flag, that NINA_CMD_SET_IF_CONFIG was called. That's something I want to avoid.

I can do that. @iabdalkader since you made that driver, do you have more information?

Hi, information about what specifically ? Can you provide more context ?

The discussion was about enabling and disabling DHCP in the NINAW10 module and whether there is a dedicated command/function to control it, or if it happens implicitly. The assumption now is: DHCP is enabled, when the NINAW10 module is intialized. And on any manual setting of the IP configuration it's disabled.

Yes that seems to be the case. Although I can't find an explicit call to tcpip_adapter_dhcpc_start in the nina firmware, but it seems to be enabled by default and disabled on static IP config.

extmod/network_ninaw10.c

ports/esp32/network_common.c

extmod/network_ninaw10.c

felixdoerre · 2024-03-28T13:36:10Z

I've incorporated the suggestions that were in the comments, so the esp32 code could work now. You can re-test that one. And/or start working on esp8266. I am not sure if esp8266 needs different code, or if that can re-use code from esp32, and if we can re-use code, should we try to extract the common logic somewhere?

robert-hh · 2024-03-28T13:51:31Z

OK. I'll re-test the ESP32. If you want to work at the ESP8266 code, I can go for the CC3200.
Or do you want to go for the esp32_ppp first?

robert-hh · 2024-03-28T14:06:21Z

The ESP32 code works fine. One little hiccup.
After setting DHCP first True, then False, the dns was still set, which seems wrong. Then, after setting addr4 and gw, dns was shown as 0.0.0.0. Nothing that needs a fix.

felixdoerre · 2024-03-28T14:38:23Z

OK. I'll re-test the ESP32. If you want to work at the ESP8266 code, I can go for the CC3200.
Or do you want to go for the esp32_ppp first?

I can go for esp32_ppp first, but that one also seems wired to me. It can only be used to set dns and not ip addresses, and retreive the whole 4-tuple. Also it does not set dns with something interface-specific, but with the lwip-global dns_setserver. Do you know if that would work as well instead of esp_netif_set_dns_info? And if so, maybe the best way is:

make dns network-global, and only use dns_setserver().
for ppp implement a method to get addr+gw
remove the dns set/get logic from network_common.

If that sounds sane to you, I'd continue with those changes, and you can pick esp8266 or cc3200 whatever you prefer.

robert-hh · 2024-03-28T14:58:27Z

I added a commit to my branch for NINAW10 dealing with dhcp4. For better visibility, I kept that separate. You can pick that commit and squash it to the existing NINAW10 commit.

robert-hh · 2024-03-28T15:02:32Z

I have no knowledge and no position about esp32_ppp. About dns setting per interface or global: that has to be tested. I can do that, if you have the code. dns setting was otherwise a network option. Although having a rather large function and some tweaky code in modnetwork.c seems not appropriate.

robert-hh · 2024-03-28T17:49:17Z

A few build errors with the last commit for ESP32_PPP. The last one is just a typo.

/home/robert/Downloads/MicroPython/micropython/ports/esp32/network_common.c: In function 'esp_network_ipconfig':
/home/robert/Downloads/MicroPython/micropython/ports/esp32/network_common.c:168:31: error: implicit declaration of function 'dns_getserver' [-Werror=implicit-function-declaration]
  168 |                 ipaddr_ntoa_r(dns_getserver(0), addr_str, sizeof(addr_str));
      |                               ^~~~~~~~~~~~~
/home/robert/Downloads/MicroPython/micropython/ports/esp32/network_common.c:168:31: warning: passing argument 1 of 'ipaddr_ntoa_r' makes pointer from integer without a cast [-Wint-conversion]
  168 |                 ipaddr_ntoa_r(dns_getserver(0), addr_str, sizeof(addr_str));
      |                               ^~~~~~~~~~~~~~~~
      |                               |
      |                               int
In file included from /home/robert/Downloads/esp-idf/components/lwip/lwip/src/include/lwip/sockets.h:46,
                 from /home/robert/Downloads/esp-idf/components/lwip/include/lwip/sockets.h:8,
                 from /home/robert/Downloads/MicroPython/micropython/ports/esp32/network_common.c:45:
/home/robert/Downloads/esp-idf/components/lwip/lwip/src/include/lwip/ip_addr.h:245:38: note: expected 'const ip_addr_t *' {aka 'const struct ip_addr *'} but argument is of type 'int'
  245 | char *ipaddr_ntoa_r(const ip_addr_t *addr, char *buf, int buflen);
      |                     ~~~~~~~~~~~~~~~~~^~~~
/home/robert/Downloads/MicroPython/micropython/ports/esp32/network_common.c:193:25: error: implicit declaration of function 'dns_setserver' [-Werror=implicit-function-declaration]
  193 |                         dns_setserver(0, &dns);
      |                         ^~~~~~~~~~~~~
In file included from /home/robert/Downloads/MicroPython/micropython/py/mpstate.h:35,
                 from /home/robert/Downloads/MicroPython/micropython/py/runtime.h:29,
                 from /home/robert/Downloads/MicroPython/micropython/ports/esp32/network_common.c:36:
/home/robert/Downloads/MicroPython/micropython/ports/esp32/network_common.c: At top level:
/home/robert/Downloads/MicroPython/micropython/ports/esp32/network_common.c:206:57: error: 'network_ipconfig' undeclared here (not in a function); did you mean 'esp_network_ipconfig'?
  206 | MP_DEFINE_CONST_FUN_OBJ_KW(esp_network_ipconfig_obj, 1, network_ipconfig);
      |                                                         ^~~~~~~~~~~~~~~~
/home/robert/Downloads/MicroPython/micropython/py/obj.h:391:104: note: in definition of macro 'MP_DEFINE_CONST_FUN_OBJ_KW'
  391 |     {{&mp_type_fun_builtin_var}, MP_OBJ_FUN_MAKE_SIG(n_args_min, MP_OBJ_FUN_ARGS_MAX, true), .fun.kw = fun_name}
      |                                                                                                        ^~~~~~~~
/home/robert/Downloads/MicroPython/micropython/ports/esp32/network_common.c:158:17: warning: 'esp_network_ipconfig' defined but not used [-Wunused-function]
  158 | static mp_obj_t esp_network_ipconfig(size_t n_args, const mp_obj_t *args, mp_map_t *kwargs) {
      |                 ^~~~~~~~~~~~~~~~~~~~

robert-hh · 2024-03-28T17:56:59Z

Just needed to uncomment #include "lwip/dns.h" and adding an esp_ at the method declaration. Now it builds. More later.

robert-hh · 2024-03-29T16:20:20Z

@felixdoerre The support for the CC3200 is now in my branch. Tested with a WiPy board and STA mode.
Just take the last commit with CC3200 code and NOT the commits for NINAW10.

robert-hh · 2024-03-29T17:43:26Z

If you think it's ready, you can change the state from draft to open.

robert-hh · 2024-03-30T08:12:35Z

@felixdoerre There was still something to fix in extmod/network_lwip.c, when using ipconfig(addr="a.b.c.d") without the number for the netmask. The other ports would then just set the IP addr, but for LWIP ports the value for the address was undefined.
You should merge that change with the other change for extmod/network_lwip.c

felixdoerre · 2024-03-30T12:38:54Z

Hmm, when you now set the address without netmask, it assumes /32. Is that reasonable? Maybe we should assume /24 as a sane default.

robert-hh · 2024-03-30T12:58:22Z

Right & thanks. The intention was to not touch the netmask at all, like the ESPxx, NINAW10 and CC3200 does at the moment.

robert-hh · 2024-03-30T13:09:51Z

Updated. I was caught by the deferred setting of ip_addr and netmask. Please cross-check.

robert-hh · 2024-03-30T21:37:25Z

@felixdoerre While adding ipconfig() to another port which I support I noticed, that the code setting addr4 can be simplified. addr_str and to_copy are in most ports not needed. I pushed already the changes for C3200 and NINAW10 to the usual place. ESP32 and ESP8266 can be simplified similarly and had the behavior, that when skipping the /nnn in CIDR notation the netmask was 255.255.255.255. Updated files to ESP32 and ESP8266:
network_common.zip for the ESP32 port
network_wlan.zip for the ESP8266 port.

Signed-off-by: Felix Dörre <felix@dogcraft.de>

Due to a omission in the NINAW10 driver stack, setting the DNS address has no effect. But the interface is kept here just in case it's fixed eventually. dhcp4 and has_dhcp4 are dummy arguments. NINAW10 seems to use always DHCP. Signed-off-by: robert-hh <robert@hammelrath.com>

There was a little omisssion in the code. Signed-off-by: robert-hh <robert@hammelrath.com>

Signed-off-by: Felix Dörre <felix@dogcraft.de>

Co-authored-by: robert-hh <robert@hammelrath.com> Signed-off-by: Felix Dörre <felix@dogcraft.de>

Signed-off-by: robert-hh <robert@hammelrath.com>

felixdoerre · 2024-03-30T22:47:38Z

Thx for cleaning up. Yes. This looks reasonable. I've incorporated all those changes.

robert-hh · 2024-03-31T10:56:24Z

Thanks. One thing that I not really comfortable with is the dns setting in network.ipconfig(). Even if I understand the reason, it's kind of bumpy, because one cannot set all parameters with one call. It requires network.ipconfig() for dns and nic.ipconfig() for the other parameters.
Is there a common practice of reference for ipconfig() and it's parameters?

felixdoerre · 2024-03-31T15:14:49Z

We discussed options in #12600. I am not aware of any other ipconfig method (like e.g. from cpython) that we modeled this after. Maybe the thing that fits the closest is the low-level interface to configure networking on linux. Do you know how the esp interfaces behave, if you have multiple and set/receive via dhcp different DNS servers on both? From what I have seen in the lwip library, the two dns servers should override each other, which can be a bit irritating, when you configure two different nics.

When you have a more complete complex linux system with resolved, that service manages the per-link dns servers and tries to intelligently switch between them, but given we don't have such a component, I'd feel that per-link dns servers are irritating.

Initially, I also envisioned to set the default gw globally, so one can actively choose where packages are routed per default, but it seems we left that out.

(and additionally, prefer is also global and only makes sense globally. That would be even more irritating if set per link, so if we have ipv6 we need that global method anyways).

robert-hh · 2024-03-31T15:49:17Z

Interestingly the last comment of Damien in #12600 uses a DNS setting in the nic.ipconfig() method.

felixdoerre · 2024-03-31T23:53:38Z

Yes, and when I implemented the first round for lwip, I had dns in the nic method, but Damien hinted to keep the split between global and nic-config: #13689 (comment)

robert-hh · 2024-04-01T07:00:08Z

Maybe that can be re-considered. I most cases just one NIC is used. And most ports allow just one NIC well at a time. The ESP32 port for instance allows to use both WiFi and Ethernet at the same time in a good fashion, but for other ports this either does not work or is somewhat unresponsive. At least it's that what I have seen.

felixdoerre · 2024-04-01T11:11:51Z

A similar thing, is that activating dhcp is now non-blocking, meaning that if you want to make sure, you have an IP before you continue, requires additional calls. Maybe the easiest way for "common usage" would be to have one "network-config" method, that can accept one dict, and then also deal with wifi-phy setup and similar? Because the goal, to configure networking in one call, is already not possible in most cases.

projectgus · 2024-05-14T02:08:27Z

These code changes LGTM, although I think at some point (not this PR) we should look at refactoring out some of the common parts - for example splitting an IPV4/NETMASK string is implemented at least four times and they're not all the same.

There might even be a way to do this like the recent extmod/machine changes where the port supplies some static functions for each action, and there's one common argument parsing and verification routine.

But not for this PR, this LGTM.

Maybe the easiest way for "common usage" would be to have one "network-config" method, that can accept one dict, and then also deal with wifi-phy setup and similar? Because the goal, to configure networking in one call, is already not possible in most cases.

How difficult do you think this would be to implement on top of what's here? Agree it'd be a nice helper method for the common use case, although I worry a bit about the complexity of all the smaller details.

felixdoerre force-pushed the more_ipconfg_nic branch 5 times, most recently from 8f32d70 to 1b81f03 Compare March 27, 2024 22:25

felixdoerre mentioned this pull request Mar 27, 2024

docs: Update docs to replace ifconfig with ipconfig. #14140

Open

robert-hh reviewed Mar 28, 2024

View reviewed changes

ports/esp32/network_common.c Outdated Show resolved Hide resolved

robert-hh reviewed Mar 28, 2024

View reviewed changes

ports/esp32/network_common.c Show resolved Hide resolved

felixdoerre force-pushed the more_ipconfg_nic branch from 1b81f03 to 08758f3 Compare March 28, 2024 13:06

felixdoerre commented Mar 28, 2024

View reviewed changes

felixdoerre force-pushed the more_ipconfg_nic branch 3 times, most recently from 92f35c0 to cdf1713 Compare March 28, 2024 17:22

robert-hh added the ports label Mar 29, 2024

felixdoerre marked this pull request as ready for review March 29, 2024 17:54

felixdoerre force-pushed the more_ipconfg_nic branch from b53000f to a053d0c Compare March 29, 2024 18:00

felixdoerre force-pushed the more_ipconfg_nic branch from a053d0c to c259bef Compare March 30, 2024 09:09

felixdoerre force-pushed the more_ipconfg_nic branch from c259bef to ff6c34d Compare March 30, 2024 13:28

felixdoerre and others added 6 commits March 30, 2024 22:41

ports/{esp32,...}: Implement nic.ipconfig for more network interfaces.

7e1fc9e

Signed-off-by: Felix Dörre <felix@dogcraft.de>

extmod/network_lwip.c: Allow using the CIDR notation for addr4.

329ecf9

There was a little omisssion in the code. Signed-off-by: robert-hh <robert@hammelrath.com>

ports/esp32/network_ppp: Get config of esp ppp interfaces via ipconfig.

6a9af21

Signed-off-by: Felix Dörre <felix@dogcraft.de>

ports/esp8266/network_wlan: Get/set config of esp8266 via ipconfig.

92b9b97

Co-authored-by: robert-hh <robert@hammelrath.com> Signed-off-by: Felix Dörre <felix@dogcraft.de>

cc3200/modflash.c: Add the ipconfig() methods.

64a24f4

Signed-off-by: robert-hh <robert@hammelrath.com>

felixdoerre force-pushed the more_ipconfg_nic branch from ff6c34d to 64a24f4 Compare March 30, 2024 22:47

dpgeorge added this to the release-1.23.0 milestone Apr 19, 2024

dpgeorge modified the milestones: release-1.23.0, release-1.24.0 Apr 30, 2024

Add nic.ipconfig for other ports (like esp32, stm32 and others) #14199

Are you sure you want to change the base?

Add nic.ipconfig for other ports (like esp32, stm32 and others) #14199

Conversation

felixdoerre commented Mar 27, 2024 • edited

robert-hh commented Mar 28, 2024

robert-hh commented Mar 28, 2024

robert-hh commented Mar 28, 2024

robert-hh commented Mar 28, 2024

robert-hh commented Mar 28, 2024

codecov bot commented Mar 28, 2024 • edited

Codecov Report

github-actions bot commented Mar 28, 2024

robert-hh commented Mar 28, 2024 • edited

robert-hh commented Mar 28, 2024

felixdoerre Mar 28, 2024 • edited

Choose a reason for hiding this comment

robert-hh Mar 28, 2024

Choose a reason for hiding this comment

robert-hh Mar 28, 2024 • edited

Choose a reason for hiding this comment

iabdalkader Mar 28, 2024

Choose a reason for hiding this comment

robert-hh Mar 28, 2024

Choose a reason for hiding this comment

iabdalkader Mar 29, 2024

Choose a reason for hiding this comment

robert-hh Mar 29, 2024

Choose a reason for hiding this comment

felixdoerre commented Mar 28, 2024

robert-hh commented Mar 28, 2024

robert-hh commented Mar 28, 2024

felixdoerre commented Mar 28, 2024

robert-hh commented Mar 28, 2024 • edited

robert-hh commented Mar 28, 2024

robert-hh commented Mar 28, 2024

robert-hh commented Mar 28, 2024 • edited

robert-hh commented Mar 29, 2024

robert-hh commented Mar 29, 2024

robert-hh commented Mar 30, 2024

felixdoerre commented Mar 30, 2024

robert-hh commented Mar 30, 2024

robert-hh commented Mar 30, 2024

robert-hh commented Mar 30, 2024

felixdoerre commented Mar 30, 2024

robert-hh commented Mar 31, 2024

felixdoerre commented Mar 31, 2024

robert-hh commented Mar 31, 2024

felixdoerre commented Mar 31, 2024

robert-hh commented Apr 1, 2024

felixdoerre commented Apr 1, 2024 • edited

projectgus commented May 14, 2024

felixdoerre commented Mar 27, 2024 •

edited

codecov bot commented Mar 28, 2024 •

edited

robert-hh commented Mar 28, 2024 •

edited

felixdoerre Mar 28, 2024 •

edited

robert-hh Mar 28, 2024 •

edited

robert-hh commented Mar 28, 2024 •

edited

robert-hh commented Mar 28, 2024 •

edited

felixdoerre commented Apr 1, 2024 •

edited