[Gratuitous ARP] Thread-safe LWIP API calls, fix condition for IF #2583
Conversation
As stated in LWIP documentation: https://www.nongnu.org/lwip/2_1_x/multithreading.html "When running in a multithreaded environment, raw API functions MUST only be called from the core thread since raw API functions are not protected from concurrent access (aside from pbuf- and memory management functions). Application threads using the sequential- or socket API communicate with this main thread through message passing." https://www.nongnu.org/lwip/2_1_x/netifapi_8h.html Is not present in current esp-idf / arduino-esp32 builds. Using existing "private" TCPIP API, create a minimal struct to pass a message to the LWIP thread instead of calling etharp_gratuitous directly.
See this comment and further discussion in the issue: xoseperez/espurna#1877 (comment) Use the same condition as esp-lwip.
|
Great! |
|
Might've been to fast on this.... there's a public api for this type of thing, somehow missed that reading through the docs.
Is it ok to just add new commit with callback() or should I use rebase to eb3cc6c message as well? |
|
Just add it to this PR, it has not yet been merged (due to build issues on some builds, we're approaching max sketch size) I prefer no have the Gratuitous ARPs to be non blocking, as long as the queue is being processed in order, or at least in order for the 'forced' ARP packets. For example after a detected failed request. |
|
FWIW, some brief testing shows that it takes around the same time for sendGratuitousARP to finish in either blocking or non-blocking mode: timings screen avg ~0.05s, 0.1s worst case Speaking of failed requests, I think I stumbled on another bug. Running c008 with wrong IP or PORT caused the node to have 99% load and webserver became really slow and completely unresponsive to settings changes. Serial terminal sort of working, but extremely slow as well. |
|
What is the status of this PR? |
|
This works as-is. In theory, iterating ifs should probably be inside of the tcp thread too (ref hwaddr check), but it should still work like this too. I have not tested this for very long though.
Yes, sorry, I forgot about it. |
https://github.com/espressif/esp-lwip/blob/61d840ff4778f4946c8743f7e412345abcd537f1/src/core/ipv4/etharp.c#L142-L154
i.e.
netif_is_up(garp_netif) && netif_is_link_up(garp_netif)https://www.nongnu.org/lwip/2_1_x/multithreading.html
Therefore, utilize the same methods as netifapi does, so that etharp method is called from within lwip/tcpip thread. netifapi itself is not included in arduino-esp32 idf sdkconfig, but required code is rather self-explanatory:
https://www.nongnu.org/lwip/2_1_x/netifapi_8h.html
https://github.com/espressif/esp-lwip/blob/2.1.2-esp/src/api/netifapi.c
As to why... Reading AsyncTCP issue tracker, using tcp/udp methods directly had caused a lot of issues in the past, until tcpip_api messaging replaced it. Even something as small as this should probably use safer methods.
Notes:
err_tis substituted viaint8_t, because platformio .ino converter will create .cpp as ESPEasy.ino + prototypes + everything else. As the result there's compilation error, becauseinclude <lwip/...>only happens aftererr_tis mentioned.Continuing from my comment:
xoseperez/espurna#1877 (comment)
I also noticed that current LWIP build for ESP32 already has gratuitous arp timer set up to trigger every 60 seconds after connection was established, but the value seems to be too high and current burst ... to 5 seconds implementation does a better job at notifying other devices about it's MAC.
platformio.ini contains some flags that try to change this behaviour:
1c451bb
But they are ineffective, because lwip is pre-built:
https://github.com/espressif/arduino-esp32/blob/master/tools/sdk/lib/liblwip.a
Perhaps, those CONFIG... flags should be removed?