cpu/esp32: lwIP netdev

[gschorcht]

Contribution description

The changes in this PR allow to use an esp_wifi network device of ESP32 with lwIP.

Testing procedure

Compile tests/lwip and flash an ESP32 node with:

USEMODULE=esp_wifi CFLAGS='-DESP_WIFI_SSID=\"<your_ssid>\" -DESP_WIFI_PASS=\"<your_passphrase>\"' make BOARD=esp32-wroom-32 -C tests/lwip flash

Start netcat on a Linux machine in same LAN:

nc -6l 12345

Use a terminal to connect to the ESP32 node and use the following commands

> tcp connect fe80::c73e:f07d:d95d:ebf9 12345
> tcp send 00
Success: send 1 byte over TCP to server
> tcp disconnect

where the address should be replaced with the address of your Linux machine.

Issues/PRs references

Fixes #11910
Depends on PR #11964
Depends on PR #11967

[miri64]

Code-wise it looks good, however, I don't have the hardware to test. @MichelRottleuthner can you have a look?

[MichelRottleuthner]

Tried it here and compared to master it works better - but something seems to be broken.
I.e. instead of (master):

tcp connect <pc_lladdr> <port>
Error: unable to connect

I can do the following:

tcp connect <pc_lladdr> <port>
tcp send 313233
Success: send 3 byte over TCP to server

On the PC I do the following:

socat -dd TCP6-LISTEN:12345 stdout
2019/08/01 15:27:24 socat[30856] N listening on AF=10 [0000:0000:0000:0000:0000:0000:0000:0000]:12345
2019/08/01 15:31:37 socat[30856] N accepting connection from AF=10 [fe80:0000:0000:0000:32ae:a4ff:fed3:35ac]:49153 on AF=10 [fe80:0000:0000:0000:xxxx:xxxx:xxxx:xxxx]:12345
2019/08/01 15:31:37 socat[30856] N using stdout for reading and writing
2019/08/01 15:31:37 socat[30856] N starting data transfer loop with FDs [6,6] and [1,1]
1232019/08/01 15:32:33 socat[30856] W read(6, 0x55c562462680, 8192): Connection reset by peer
2019/08/01 15:32:33 socat[30856] N socket 1 to socket 2 is in error
2019/08/01 15:32:33 socat[30856] N socket 1 (fd 6) is at EOF
2019/08/01 15:32:33 socat[30856] N exiting with status 0

With wireshark I can see that there are a lot of (5000) TCP Dup ACK and Spurious Retransmissions before socat reports the error. UDP seems to work ok.

(I used socat instead of netcat didn't work at all for me, but that could be because of the many different netcat versions out there)

[miri64]

This seems to be rather an lwIP problem than a problem with the device integration. However if you see the packets that means the device integration works, no?

[miri64]

Tried it here and compared to master it works better - but something seems to be broken.
I.e. instead of (master):

No surprise there ^^ device support is always better than no device support ;-)

[MichelRottleuthner]

@miri64 I agree. Just wanted to point out what my results were. And I prefer to say if something was different to it just works(TM). Also I lack the expertise on our lwip integration to judge if this actually comes from lwip or the device integration^^

[miri64]

ACK

[miri64]

Please squash @gschorcht

[gschorcht]

I can observe these duplicate ACKs too. Strange, the ACK repeated a thousand times is not the ACK for the data packet, but the last ACK from the three-way handshake that originated from the initiating RIOT node.

[gschorcht]

Hm, I tried it with native and a tap device. It does seem to have the problem with the duplicate ACKs. It works exactly as exepcted. Maybe, the problem is even related to the esp_wifi netdev.

[gschorcht]

@miri64 @MichelRottleuthner After investigating the problem of duplicate ACKs for two days, I finally found the cause. In fact, the problem was the esp_wifi network device driver. Function _esp_wifi_send did not return the right value. The problem was so obvious 😎 and easy to fix.

I fixed it with commit 7941149. Now, lwIP is working with esp_wifi as expected. Only one ACK, no packet retransmission and so on.

If you prefer, I could provide the fix within a separate PR and could rebase this PR to it.

BTW, when I was investigating the problem, I realized that there are still other problems in esp_wifi, e.g., in NETDEV_EVENT_ISR handling that might lead to complete blocking under very special conditions. Therefore, I've already refactored the implementation of esp_wifi in the meantime. The new version will be very similar to the ESP8266 implementation and would allow further code deduplication. I will open a PR in the next few days.

[gschorcht]

@MichelRottleuthner Thank you for figuring out the problem with the duplicate ACKs. It was a very useful. May I ask you to test it again with the fix?

[MichelRottleuthner]

I tested again. No more duplicate acks and now a graceful disconnect after sending data and executing tcp disconnect.

If you prefer, I could provide the fix within a separate PR and could rebase this PR to it.

I don't mind but I'd prefer to have the fix in a separate commit so it is not completely mixed with the lwip related netdev stuff.

[gschorcht]

@miri64 After fixing and cleaning up cpu/esp32/Makefile.include (Thanks to @MichelRottleuthner for testing), the light for the rebased and squashed version of this PR green now.

[miri64]

What is this about?

[miri64]

If it is required, then this shouldn't be hidden in the platform code.

[gschorcht]

It is platform specific. By default the lwIP's tcpip thread has a priority of 1. However, in ESP32, the thread that handles hardware interrupts from the WiFi interface has also a priority of 1. From my point of view, the hardware driver should have a higher priority than the TCP/IP thread to be sure that hardware is always handled in time.

I figured it out and changed it when I was investigating the problem with the duplicate ACKs. This change was already covered by the test by @MichelRottleuthner.

[miri64]

I have no doubts that it works, I just fear that it might cause problems later on, with people not expecting that something related to an external package is configured within a cpu Makefile. How about flipping it around and putting it in pkg/lwip/Makefile.include? Including some explanation why it is required.

[miri64]

I have no doubts that it works, I just fear that it might cause problems later on, with people not expecting that something related to an external package is configured within a cpu Makefile. How about flipping it around and putting it in pkg/lwip/Makefile.include? Including some explanation why it is required.

[gschorcht]

Do you think it is better to place ESP32 specific make code in pkg/lwip/Makefile.include? Hm, I do not feel very comfortable with that.

[miri64]

Do you think it is better to place ESP32 specific make code in pkg/lwip/Makefile.include? Hm, I do not feel very comfortable with that.

This is not unheard of: nimble e.g. pulls in architecture dependent modules in its Makefile:

RIOT/pkg/nimble/Makefile.dep

Lines 32 to 34 in 653c23e

	ifeq (nrf52,$(CPU_FAM))
	USEMODULE += nimble_drivers_nrf52
	endif

Both approaches have an ugly side for sure, but I feel far more comfortable if we add a platform-specific config to an external package's configuration than configuring a platform for an external package.

[gschorcht]

Maybe, a better place could be here

RIOT/pkg/lwip/include/lwipopts.h

Line 145 in 653c23e

where the overridden default definitions reside in lwIP

+#ifdef CPU_ESP32
+#define TCPIP_THREAD_PRIO   2
+#endif

[miri64]

Maybe, a better place could be here

RIOT/pkg/lwip/include/lwipopts.h

Line 151 in 653c23e

where the overridden default definitions reside in lwIP

+#ifdef CPU_ESP32
+#define TCPIP_THREAD_PRIO   2
+#endif

Right. With lwip we have this ability. It still follows my argument about config semantics, so you may decide which of the two approaches (pkg/lwip/Makefile or pkg/lwip/include/lwipopts.h) you prefer.

[miri64]

In either case, don't forget documenting the "why" ;-)

[miri64]

Thanks! Please squash 5610802, b40a119, and d27dfb3 into one commit.

[gschorcht]

Squashed. Unfortunately, Travis failed due to connection errors to github.com. How can we trigger Travis restart the static tests?

[miri64]

Squashed. Unfortunately, Travis failed due to connection errors to github.com. How can we trigger Travis restart the static tests?

Travis sems fine so did you restart yourself? You can do that by logging in with your GitHub account via OAuth. Maintainership should grant you permission to hit the restart button then.

[miri64]

ACK. Code looks fine and was tested by @MichelRottleuthner.

[gschorcht]

@miri64 @MichelRottleuthner Thanks for reviewing and testing it.