SuperHouse / esp-open-rtos

Open source FreeRTOS-based ESP8266 software framework
BSD 3-Clause "New" or "Revised" License
1.53k stars 491 forks source link

lwip update #554

Closed ourairquality closed 6 years ago

ourairquality commented 6 years ago
ourairquality commented 6 years ago

The default lwip stack has been lowered from 768 words to 480 words, due to the reduced stack usage by the mdns responder, recovering almost 1k. Hope this does not impact other apps, but memory can be tight.

jeffsf commented 6 years ago

While extras/sntp appears to extract src/apps/sntp/sntp.*, having the current version "in tree" will resolve some of the problems with the present SNTP implementation, such as its failing to measure and take into account RTT. While I'm not sure why the code was replicated in extras/sntp (especially as you need to have LWIP to use it), updating sntp.* is quite welcome (as is updating LWIP, in general!)

_Edit: For the "new" SNTP, this will be required to provide LWIP_ASSERT_CORE_LOCKED()_

jeffsf commented 6 years ago

When I check this out or merge this onto master I get many compile-time warnings about redefinition of [UN}LOCK_TCP_CORE() such as

In file included from /Volumes/esp-build/esp-open-rtos/open_esplibs/libnet80211/ieee80211_hostap.c:8:0:
../../lwip/lwip/src/include/lwip/tcpip.h:56:0: warning: "LOCK_TCPIP_CORE" redefined [enabled by default]
 #define LOCK_TCPIP_CORE()     sys_mutex_lock(&lock_tcpip_core)
 ^
In file included from ../../lwip/lwip/src/include/lwip/opt.h:51:0,
                 from ../../lwip/lwip/src/include/lwip/tcpip.h:40,
                 from /Volumes/esp-build/esp-open-rtos/open_esplibs/libnet80211/ieee80211_hostap.c:8:
../../lwip/include/lwipopts.h:129:0: note: this is the location of the previous definition
 #define LOCK_TCPIP_CORE()          sys_lock_tcpip_core()
 ^

Is there something that I need to do to remove the cause of these warnings that you know of?

(examples/blink used for a relatively "clean" build of LWIP)

ourairquality commented 6 years ago

There are no warnings here when building examples/blink. The patch has been rebased to master and updated to the latest lwip too, and the build check above did not complain, so perhaps you will need to explore the issue locally.

jeffsf commented 6 years ago

Confirm no compiler warnings building blink after git submodule update (not sync -- too much time on Android, apparently)

Tracking down

Warning: ELF binary has undefined symbol sys_mbox_trypost_fromisr

Thanks for the effort on this merge and the response.

SaimenSays commented 6 years ago

After latest update I also get the

Warning: ELF binary has undefined symbol sys_mbox_trypost_fromisr

If it is not related to this update, what can I check to find what cause it has? This symbol is not in my code, so there is something included which uses it. For the blink example this is optimized away and won't generate the warning, or I am wrong?

flannelhead commented 6 years ago

It seems that lwIP now requires a function sys_mbox_trypost_fromisr along with sys_mbox_trypost in sys_arch.c and this implementation is missing somehow.

ourairquality commented 6 years ago

The function sys_mbox_trypost_fromisr is not required for esp-open-rtos and it does not use that path. Missed that this had been added to lwip, and here's a quick patch to quieten linking https://github.com/SuperHouse/esp-open-rtos/pull/567

flannelhead commented 6 years ago

@ourairquality, thank you! I was even getting linker failures for some reason, which seem pretty illogical:

./build/lwip.a(tcpip.o): In function `tcpip_send_msg_wait_sem':
/home/sakari/projektit/espway/esp-open-rtos/lwip/lwip/src/api/tcpip.c:436: undefined reference to `sys_mbox_trypost_fromisr'
/home/sakari/projektit/espway/esp-open-rtos/lwip/lwip/src/api/tcpip.c:436: undefined reference to `sys_mbox_trypost_fromisr'

What baffles me is that tcpip_send_msg_wait_sem doesn't refer to sys_mbox_trypost_fromisr. It is called from tcpip_callbackmsg_trycallback_fromisr though, and probably the linker is just confusing something with the line numbers.

Adding the stub certainly removes the errors.

SaimenSays commented 6 years ago

@ourairquality: Thanks for all your work!

For my project I get multiple "Function called without core lock". After some inspection I need to add multiple LOCK_TCP_IP core to tcp_new(), tcp_bind(), tcp_accept(), ... Now everything seems to be fine, but I think this not the original intention. Are these tcp_ not intended for user code and should I use higher level functions instead? Some time ago I tried to use netconn API, but I find this a bit strange and dropped it.

ourairquality commented 6 years ago

@SaimenSays This was a new check for lwip, and for now it just prints a warning for esp-open-rtos. The intention is not to add LOCK_TCP_IP to these lwip functions rather the caller needs to ensure this lock is held by wrapping the call in LOCK_TCP_IP() and UNLOCK_TCPIP_CORE(). On inspection it might be that the lock also needs to be help between some of these calls.