lwip2 include ip4_addr.h has wrong defines?

gitpeut commented 6 years ago

Basic Infos

[x] This issue complies with the issue POLICY doc.
[x] I have read the documentation at readthedocs and the issue is not addressed there.
[x] I have tested that the issue is present in current master branch (aka latest git).
[x] I have searched the issue tracker for a similar issue.
[x] If there is a stack dump, I have decoded it.
[x] I have filled out all fields below.

Platform

Hardware: Wemos mini
Core Version: Arduino release 2.4.1 - SDK
Development Env: [Arduino IDE]
Operating System: [Windows]

Settings in IDE
Module: [Wemos D1 mini r2]
Flash Mode: [qio|dio|other]
Flash Size: [4MB/1MB]
lwip Variant: [v1.4|v2 see text below]
Reset Method: [N/A]
Flash Frequency: [40Mhz]
CPU Frequency: [80Mhz]
Upload Using: [SERIAL]
Upload Speed: [115200] (serial upload only)

Problem Description

Attached reproducer using lwip2 ( both low memory and high bandwidth) will not compile. Code compiles using lwip 1.4. IP2STR was used in some code I was re-using. Reason seems to be wrong define of ip4_addr* in lwip2/include/ip4_addr.h. Replacing defines with the ones from lwip 1.4 successfully compiles and works.

I am not sure if these defines in lwip2 are wrong by design or if this is a bug. As the 1.4 lwip version seems to work I suspect the latter.

Reproducer:

ip4_addr_reproducer.ino.txt

Compile Errors:

compile_errors.txt

Fix? Below the changed lines in lwip2/include/ip4_addr.h. Commented out the original defines that would not compile and added the same define from the lwip/include/ip4_addr.h.

//* WRONG ** / Get one byte from the 4-byte address / //#define ip4_addr1(ipaddr) (((const u8_t)(&(ipaddr)->addr))[0]) //#define ip4_addr2(ipaddr) (((const u8_t)(&(ipaddr)->addr))[1]) //#define ip4_addr3(ipaddr) (((const u8_t)(&(ipaddr)->addr))[2]) //#define ip4_addr4(ipaddr) (((const u8_t)(&(ipaddr)->addr))[3]) //**END WRONG **** //*RIGHT****

define ip4_addr1(ipaddr) (((u8_t*)(ipaddr))[0])

define ip4_addr2(ipaddr) (((u8_t*)(ipaddr))[1])

define ip4_addr3(ipaddr) (((u8_t*)(ipaddr))[2])

define ip4_addr4(ipaddr) (((u8_t*)(ipaddr))[3])

//**END RIGHT**

MauiJerry commented 6 years ago

The given fix is incorrect on a couple points: 1) The RIGHT version should use (u8_t) - a pointer to u8_t rather than a (u8_t) itself. 2) There needs to be a Close Comment / at the end of END WRONG line

Adding these to the file $(HOME)/Library/Arduino15/packages/esp8266/hardware/esp8266/2.4.1/tools/sdk/lwip2/include/lwip/ip_addr.h allowed my application (using painlessmesh library) to compile.

gitpeut commented 6 years ago

Maybe. Not my application, though. Funny how this changed then between lwip versions and previous non-lwip esp8266 versions. Hopefully the gurus will decide wisely.
No. // at the start of the line make this whole line a comment, never mind the asterisks

d-a-v commented 6 years ago

ip4_addrN() macros are lwIP's source code and not to blame. IP2STR() is espressif's and the right way to use it is

ip_addr_t ip = { WiFi.localIP() };
...printf(..., IP2STR(&ip));

or

ip_addr_t ip;
ip.addr = WiFi.localIP();
...printf(..., IP2STR(&ip));

The only "official" use of IP2STR() I found is in examples (link). Although your code is working well with lwIP-v1.4, it does not with lwIP-v2 because the lwIP folks adapted their structures to be the best possible compatible whether IPv6 is enabled or not. Note that the type ip_addr_t is no longer officially existing in lwIP-v2, and is still provided with our core's lwip2 for backward compatibility.

edit: fixed the second example

esp8266 / Arduino