I asked @kaspar030 to test this as he as both an stm32 with on-board Ethernet and a self-administered router in his office.

@kaspar030 you can activate the DHCP client by using the lwip_dhcp pseudo-module.

I'm just wondering - is this something that should be only done for stm32_eth?

I'm just wondering - is this something that should be only done for stm32_eth?

@benpicco Good point! However, this is done in a separate preprocessor conditional for all supported devices, and thus this follows the same design choice. You could argue that we could move the netif_add of all interfaces outside of their conditionals and simply always run the netif_add, because indeed... the called functions are identical. But I think doing so would cause 3 other effects:

1. no more support for devices that have multiple netifs (e.g MRF24J40 or even TAP);
2. the passed state parameter has a unique type specified for each hardware type. We'd have to create a generic netdev_t and use it for all of them. Also the variable name would need to be generic (currently the variable name contains the a name related to the hardware e.g. stm32_eth);
3. The DEBUG message on fail would also be generic, and not contain the name of the hardware we try to initialize anymore

So to be honest, I think the design choice wasn't that bad.
It is possible to do it the way you propose it, but it would require a full rewrite of the lwip-bootstrap.

How about adding it for esp_wifi then?
So it doesn't look like this is something special for stm32 - also easier to test since they should be more common than stm32s with ethernet.

So to be honest, I think the design choice wasn't that bad.
It is possible to do it the way you propose it, but it would require a full rewrite of the lwip-bootstrap.

The design choice was based on my laziness, so I wouldn't say it is good either. If you want to take care of it go ahead. Just know that @jia200x's netif rework will include some generalized device initialization, so your rework might not stick for long.

@benpicco So you mean like this?

#if esp:
--> esp setup
#endif
#if stm32
--> setup stm32
#endif
#if esp || stm32
--> netif_add
#endif

It's entirely possible, but I'm not sure if I agree that this will be an improvement. It would remove a clear seperation between the different hardware families.
Point 2 of my previous comment also still applies in this case: We'd have to use a generic variable now instead of the esp_wifi_netdev_t and the netdev_t (or use a generic variable name and cast the variable to the proper type).

@miri64 there's always room for improvement ofcourse, but at the moment there's a clear seperation between all hardware devices (whether that's a good thing or not leaves room is still open for debate, but not all that relevant here), breaking this clear seperation for only two of all the devices will create confusion and is not really an improvement imo.

@wosym I thought more of adding the same chunk of code to the esp_wifi block so when someone comes around to refactor this they note the similarity and see a general solution is needed 😉

No need to do something clever.

@benpicco Oooh, I entirely misunderstood you! I thought you meant to move the esp and stm code out of their preprocessor-blocks :)
Yes, what you ask is perfectly possible! However, I will not be able to test this. I don't have the required hardware.

I work for a company that sells devices to end users, so we have no control over their networking infrastructure. Like it or not, we have to support IPv4. So, I agree that lwip IPv4 support for other devices is worthwhile. I (and a number of people) have an esp_wifi device, and would be happy to test.

@kb2ma Go ahead! The esp-code is already pushed! :)

I tried to run tests/lwip_sock_udp. I was unable to compile because arch_esp8266 is blacklisted in pkg/lwip/Makefile.dep.

Not one to let a little thing like a blacklist get in the way, I found two issues:

The test Makefile was setting a CFLAG for SO_REUSE. The esp8266 SDK required use of another parameter to set this value, CONFIG_LWIP_SO_REUSE. So I set that value to 1 in CFLAGS.

The test Makefile also set a macro value ETHARP_SUPPORT_STATIC_ENTRIES=1. However, the esp8266 SDK hard codes this flag to 0. I changed the value to 1, and the test compiled.

However, when running the test, I see the failure below. Maybe @gschorcht or @miri64 has some insight. Perhaps it's not feasible to use with sock.

Assuming there is not some serious incompatibility with esp8266, @wosym I see you have used tests/lwip in #12859. If you can let me know the Makefile magic for that to use IPv4, I could try running the shell for it. I have not used lwip before, so need some hand holding.

main(): This is RIOT! (Version: 2020.01-devel-1311-gf37ed-lwip_stm32_ipv4)
Help: Press s to start test, r to print it is ready
READY
s
START
code 0x11
mode : sta(bc:dd:c2:14:63:ab) + softAP(be:dd:c2:14:63:ab)
add if0
add if1
bcn 100
system_event_sta_start_handle_default
system_event_ap_start_handle_default
scandone
state: 0 -> 2 (b0)
state: 2 -> 0 (200)
system_event_sta_disconnected_handle_default
disconnected from ssid xxxx, reason 2 (AUTH_EXPIRE)
Calling test_sock_udp_create4__EADDRINUSE()
Calling test_sock_udp_create4__EAFNOSUPPORT()
Calling test_sock_udp_create4__EINVAL_addr()
Calling test_sock_udp_create4__EINVAL_netif()
Calling test_sock_udp_create4__no_endpoints()
Calling test_sock_udp_create4__only_local()
Calling test_sock_udp_create4__only_local_port0()
Calling test_sock_udp_create4__only_local_reuse_ep()
*** RIOT kernel panic:
FAILED ASSERTION.

	pid | name                 | state    Q | pri | stack  ( used) | base addr  | current     
	  - | isr_stack            | -        - |   - |   2048 (  792) | 0x3ffe8410 | 0x3ffe8c10
	  1 | ppT                  | bl rx    _ |   2 |   3636 (  564) | 0x3fff6d10 | 0x3fff79e0 
	  2 | pmT                  | bl rx    _ |   4 |   1076 (  324) | 0x3fff7b90 | 0x3fff7e90 
	  3 | rtT                  | bl rx    _ |   3 |   2100 ( 1372) | 0x3fff7fd0 | 0x3fff86d0 
	  4 | esp_events           | bl rx    _ |   5 |   2100 (  876) | 0x3fff8e40 | 0x3fff9510 
	  5 | idle                 | pending  Q |  31 |   1024 (  232) | 0x3fff0e08 | 0x3fff1130 
	  6 | main                 | running  Q |  15 |   3072 ( 1576) | 0x3fff1208 | 0x3fff1950 
	  7 | lwip_netdev_mux      | bl rx    _ |  11 |   1024 (  296) | 0x3fff4668 | 0x3fff4950 
	  8 | tcpip_thread         | bl mbox  _ |   1 |   1024 (  416) | 0x3fff1e50 | 0x3fff20c0 
 ets Jan  8 2013,rst cause:2, boot mode:(3,6)

load 0x40100000, len 7160, room 16 
tail 8
chksum 0xf8
load 0x3ffe8408, len 24, room 0 
tail 8
chksum 0x41
load 0x3ffe8420, len 3500, room 0 
tail 12
chksum 0x68

@kb2ma Yes, I also used lwip in that PR, but because it's an entirely different issue that is being fixed in that patch, I didn't apply any of the unmerged IPv4 changes there yet. It's entirely tested on IPv6.

However, I did use LWIP IPv4 in the code of an unrelated project for a customer. All I had to do is add the following modules:
USEMODULE += ipv4_addr
USEMODULE += lwip_arp
USEMODULE += lwip_dhcp
USEMODULE += lwip_ipv4
USEMODULE += lwip lwip_sock_ip lwip_netdev
USEMODULE += lwip_udp lwip_sock_udp
USEMODULE += lwip_tcp lwip_sock_tcp

You might also want to enable DHCP (unless you want to assign the IP's static, which is also possible):
CFLAGS += -DUSE_DHCP

@wysman #12950 is about esp32 ethernet, this just adds the configuration for the wifi.
And it seems like it uncovered a bunch of bugs while doing so - good we did it

@benpicco It should be @wosym 😉 Ineed, the PR #12950 only covers the ESP32 ethernet driver esp_eth, not ESP WiFi.

@benpicco FYI, We have start to create a minimal IPv4 stack using Riot GNRC. The branch is not in an active development currently but we will finish it as soon as possible. We have ARP requests support, IPv4 (send broadcast, send unicast, and receive demux). We have test it on an ESP32 board with UDP client & server tests packages. We need to finish the TCP part (seem broken in RIOT when we try it). The branch is not very clean, we rush that on few days.

https://github.com/Power-Lan/RIOT/tree/gnrc_ipv4

Reverted to the original state by removing the ESP-code, and added a comment after the else preprocessor directive.

@wosym Why did you remove the ESP-code?

@gschorcht: Whoops, looks like I misinterpreted the previous comments. I added it again.

@gschorcht: Whoops, looks like I misinterpreted the previous comments. I added it again.

@wosym Thanks.

ACK from my side. Tested on ESP32.

What should we do with STM32 test? I don't have the hardware but I would guess that it also works on STM32. It's a very small change that has been proven with ESP32 so that a successful compilation for STM32 should be suffiecient.

What STM32 test do you mean? Test if IPv4 works?
I've been using it for 2-3 weeks now, seems to work without a hitch.

I did not however add it to tests/lwip. Should that be done?

How did you test this change in pkg/lwip/contrib/lwip.c for STM32?

@wosym Do you have a version of tests/lwip for IPv4 you could share?

How did you test this change in pkg/lwip/contrib/lwip.c for STM32?

I'm currently working on an implementation of the DoIP protocol for a customer. I used it on IPv6 first, but needed IPv4 support as well. after applying the changes of this PR, and changing the used modules in the DoIP example (and the used ip-addresses) everything worked like a charm. Wireshark confirmed this.

Do you have a version of tests/lwip for IPv4 you could share?

Not at the moment, no. But I can alter the existing code with some preprocessor conditionals to allow toggling between IPv4 and IPv6 if you want.

Do you have a version of tests/lwip for IPv4 you could share?

Not at the moment, no. But I can alter the existing code with some preprocessor conditionals to allow toggling between IPv4 and IPv6 if you want.

I had planned to create a PR because it makes sense to have tests/lwip also for IPv4. So I just wanted to know if you already have something ready that you could provide before I start. If not, no problem, I'll do it.

Nope, currently don't have any code for that written. Go ahead :)

@benpicco FYI, We have start to create a minimal IPv4 stack using Riot GNRC.

https://github.com/Power-Lan/RIOT/tree/gnrc_ipv4

Excellent! Please consider posting this work as a WIP PR. For fundamental work like this, it's important that it fits well in the stack. It might save everyone some time to get an early view of the implementation approach.

For ESP32 I can confirm, that the changes for IPv4 work for esp_wifi:

* `tests/lwip_sock_ip` succeed

* `tests/lwip_sock_udp` succeed

* `tests/lwip_sock_tcp` failed in `test_tcp_connect4__EINVAL_netif`. But this might be ok since `sock_tcp_connect` returns 0 (success), probably because there is a real `_TEST_NETIF + 1` netif.

Did not read the whole discussion, but I'm not surprised that a test does not run 100% as intended, if you add modules it doesn't need (the lwip_sock tests use a mock device so adding another devlce can generate side-effects).

so adding another devlce can generate side-effects.

That's what I thought too. The only test that has to work with real netif is probably tests/lwip.

That's what I thought too. The only test that has to work with real netif is probably tests/lwip.

yepp

@wosym How can I test this? When I ran the test (for a nucleo-f207zg) under tests/lwip, the IPv4 address remained empty when I ran ifconfig. It just printed:

ET_00: inet 0.0.0.0

@JannesVolkens It's been a while since I used this, so my memory might be clouded. But is it possible that you need to set up a DHCP server in order for it to work?

This PR doesn't really change tests/lwip though. @gschorcht said he was going to open a PR to adapt tests/lwip to work with these changes. I assume he did, but I never tested those changes myself. (At least not that I remember)