Ipv6 support v2 #7520

nuclearcat · 2022-11-27T16:44:28Z

This patch partially depends on:
espressif/esp32-arduino-lib-builder#67
Without this patch we will get only Link local IPv6 (still useful for MDNS and etc).
With patch we will get also global IPv6 address by SLAAC.
By default IPv6 disabled, until it is properly tested.

Tested on BasicHttpClient by adding:
wifiMulti.IPv6(true);
before: wifiMulti.addAP() call
and fetching ipv6.google.com which is ipv6 only address.

Enabling Core Debug Level: verbose
If IP6 obtained, in logs will be visible:
[ 8028][V][WiFiGeneric.cpp:380] _arduino_event_cb(): IF[0] Got IPv6: IP Index: 0, Zone: 2, fe80:0000:0000:0000:xxxx:xxxx:xxxx:xxxx
[ 8028][D][WiFiGeneric.cpp:852] _eventCallback(): Arduino Event: 8 - STA_GOT_IP6
[ 11028][V][WiFiGeneric.cpp:380] _arduino_event_cb(): IF[0] Got IPv6: IP Index: 1, Zone: 0, 2a0d:yyyy:0000:4000:yyyy:yyyy:yyyy:yyyy
[ 11028][D][WiFiGeneric.cpp:852] _eventCallback(): Arduino Event: 8 - STA_GOT_IP6

This is linked to: #6242

This patch based on prior work , feedback and contributions of @sgryphon

Signed-off-by: Denys Fedoryshchenko denys.f@collabora.com

This patch partially depends on: espressif/esp32-arduino-lib-builder#67 Without this patch we will get only Link local IPv6 (still useful for MDNS and etc). With patch we will get also global IPv6 address by SLAAC. By default IPv6 disabled, until it is properly tested. Tested on BasicHttpClient by adding: wifiMulti.IPv6(true); before: wifiMulti.addAP() call Enabling Core Debug Level: verbose If IP6 obtained, in logs will be visible: [ 8028][V][WiFiGeneric.cpp:380] _arduino_event_cb(): IF[0] Got IPv6: IP Index: 0, Zone: 2, fe80:0000:0000:0000:xxxx:xxxx:xxxx:xxxx [ 8028][D][WiFiGeneric.cpp:852] _eventCallback(): Arduino Event: 8 - STA_GOT_IP6 [ 11028][V][WiFiGeneric.cpp:380] _arduino_event_cb(): IF[0] Got IPv6: IP Index: 1, Zone: 0, 2a0d:yyyy:0000:4000:yyyy:yyyy:yyyy:yyyy [ 11028][D][WiFiGeneric.cpp:852] _eventCallback(): Arduino Event: 8 - STA_GOT_IP6 This is linked to: espressif#6242 Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>

One of most useful features of IPv6 to have arduino accessible from internet, without any port forward and etc. Change is fairly trivial and backward compatible with old code, tested with WiFiTelnetToSerial and AsyncUDPServer. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>

For RemoteIP and AF_INET6 socket i added support ip6 to ip4 mapping, so .remoteIP will return IPv4 address on dual stack socket, if available. Scenarios tested: WiFiTelnetToSerial, wifiMulti.IPv6(true), connect both from IPv4 and IPv6 WiFiTelnetToSerial, wifiMulti.IPv6(true); but set to listen on IPv4 only. WiFiTelnetToSerial, IPv6 disabled, with or without bind to specific IP4. AsyncUDPServer, without IPv6 support. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>

To demonstrate new abilities of dual stack WiFiServer and remoteIP6 we add this example. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>

We need to be able to connect to remote servers over IPv6 too, and thats need different approach in DNS queries and connect(). As i'm trying to keep maximum compatibility, i introduce different behaviour if IPv6 is enabled, and backward compatible (as much as possible), if IPv6 is not enabled. IN future when IPv6 functions are tested well enough, it can be simplified. This implementation tested on esp32 in following scenarios using BasicHttpClient: IPv6 true: IPv6 only website (caveat 1) - OK Website with A and AAAA is present (caveat 1) - OK IPv4 only website - OK IPv6 not enabled: IPv6 only website - wont open (expected) Website with A and AAAA is present - OK, opens over IPv4 IPv4 only website - OK caveat 1 - sometimes SLAAC is slower than DHCPv4, so we might have status WL_CONNECTED, but IPv6 global scope is not ready yet. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>

nuclearcat · 2022-11-27T16:46:23Z

@VojtechBartoska this is updated PR that is more standard compliant with upstream Arduino and easier(more transparent) to use.

Example contained API from previous IPv6 implementation, fixing it. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>

sgryphon · 2022-11-27T21:51:58Z

libraries/WiFi/src/WiFiGeneric.cpp

+    waitStatusBits(WIFI_DNS_IDLE_BIT, 16000);
+    clearStatusBits(WIFI_DNS_IDLE_BIT | WIFI_DNS_DONE_BIT);
+
+    err_t err = dns_gethostbyname_addrtype(aHostname, &addr, &wifi_dns6_found_callback,


Maybe add a comment above this to explain:
// Retrieves a single IP address, either from the cache or lookup
// The address type parameter means to try IPv6 first, and then fallback to IPv4 if that fails

sgryphon · 2022-11-27T21:53:14Z

libraries/WiFi/src/WiFiGeneric.cpp

+/**
+ * Resolve the given hostname to an IP6 address.
+ * @param aHostname     Name to be resolved
+ * @param aResult       IPv6Address structure to store the returned IP address


Return could be IPv6 or IPv4, so change comment:

* @param aResult IP address structure to store the returned IP address, either IPv6 if available, otherwise IPv4

sgryphon · 2022-11-27T22:35:19Z

libraries/WiFi/src/WiFiGeneric.cpp

+    setStatusBits(WIFI_DNS_IDLE_BIT);
+    return (uint32_t)err == ERR_OK || (err == ERR_INPROGRESS && arg.result == 1);
+}
+
 IPAddress WiFiGenericClass::calculateNetworkID(IPAddress ip, IPAddress subnet) {
 	IPAddress networkID;



This function calculateNetworkID and the two below calculateBroadcast and calculateSubnetCIDR only work for IPv4.

They also have an assumption that the subnet mask is in the correct format (all 1's followed by all 0's) -- if the format is incorrect, they will give weird results (but I think returning an error would be better). e.g. calculateSubnetCIDR("255.128.0.0") returns 9, whilst calculateSubnetCIDR("255.1.0.255") is invalid but returns 16.

IPv6 doesn't use the mask format and calls the CIDR value the Prefix Length.

Networks still exist, but a more useful signature would be: calculateNetworkID(IPAddress ip, uint_8 prefixLength)

Broadcast addresses don't exist in IPv6; instead various multicast address ranges fulfil the same purpose.

Suggestion:

Have calculateSubnetCIDR return 255 if invalid

Have calculateSubnetCIDR work for IPv6 (return value will be 0-128, whilst IPv4 will be 0-32), but probably won't be used.

Add an alternative for calculateNetwork(IPAddress, uint_8 prefixLength).

Call to calculateNetworkID(IPAddress ip, IPAddress subnet) can call calculateSubnetCIDR() to get the length, then return an error value if invalid, otherwise chain to the one that takes length parameter

Call to calculateBroadcast should check IPv4 only, and could also use calculateSubnetCIDR() to make sure it is valid.

I will do additional commit to handle that, i will try to check example projects that use it, to make some test cases.

sgryphon · 2022-11-27T22:37:37Z

libraries/WiFi/src/WiFiServer.cpp

-  server.sin_port = htons(_port);
+  server.sin6_family = AF_INET6;
+  if (_addr.type() == IPv4) {
+    log_e("---------------- IPv4");


Are these logs meant to still be here? why all the "-" at the beginning?

Forgotten debug, will remove

VojtechBartoska · 2022-11-28T08:25:32Z

thanks @nuclearcat for your work and PR! will be reviewed for next major release 3.0.0. It will take us some time.

TD-er · 2022-12-03T21:14:00Z

cores/esp32/IPAddress.h

@@ -83,14 +88,21 @@ class IPAddress: public Printable
    virtual size_t printTo(Print& p) const;
    String toString() const;

+    IPType type() { return _type; }


Please make this a const function.

vortigont · 2022-12-03T22:12:18Z

Just out of curiosity is there any reason not to reuse LWIP's ip_addr_t from lwip/ip_addr.h instead of _address union member in this class?
I mean, LWIP already has many useful macro's and functions, like ipaddr_ntoa() so not to reinvent methods like IPAddress::fromString6 or IPAddress::printTo.
ESP-IDF examples and code also rely on lwip's types, so exposing ip_addr_t as IPAddress's class member could make life easier in many cases.

sgryphon · 2022-12-03T23:16:51Z

Just out of curiosity is there any reason not to reuse LWIP's ip_addr_t

Probably because the implementation is copied from the ArduinoCore-API (I was an author), which does not specify/depend on LWIP.

Seeing as you are using LWIP, then it makes sense to replace the implementation with LWIP, so this change sounds like a good idea, and will simplify things. So long as you match the API that is what matters.

Using LWIP should make things easier, and I had a quick look and it seems compatible, e.g. the conversion to string is mostly the same (and it is what is being used underneath anyway for the networking).

It would be nice to have unit tests for the arduino-esp32 library (the arduniocore-api has them) for some corner cases.

e.g. It looks like ntoa in LWIP only does a single pass, which means it doesn't produce canonical format if there is a second longer run of zeros (it always does the first, i.e. left-most, run of 2 or more, instead of the longest, left-most, run of 2 or more). It is still a valid string representation, and in the majority of cases (probably all of them in practice; two long runs of zeros is uncommon) it will be the same, but it would be nice to have tests to validate this.

Also, note that the fix would be upstream in LWIP, not in this code. I still support using LWIP ip_addr_t as a far simpler implementation (with IPAddress.h just being a wrapper that adapts it to the Arduino API).

vortigont · 2022-12-04T12:07:23Z

Also, note that the fix would be upstream in LWIP, not in this code. I still support using LWIP ip_addr_t as a far simpler implementation (with IPAddress.h just being a wrapper that adapts it to the Arduino API).

Indeed, LWIP's ntoa implementation is not longest greedy as recommended rfc5952 but still could a be better option to bridge with ESP-IDF code if IPAddress is supplied with c-tor overload to instantiate/copy from ip_addr_t. I like the idea of a wrapper!
Thanks for your opinion, @sgryphon!
Let's see for community's feedback here )

s-hadinger · 2022-12-04T14:57:42Z

cores/esp32/IPAddress.h

-        uint8_t bytes[4];  // IPv4 address
-        uint32_t dword;
+        uint8_t bytes[16];
+        uint32_t dword[4];
    } _address;
+    IPType _type;


Would it be possible to use ip_addr_t instead of this local structure. This is what is done in ESP8266 and makes it easier to call esp-idf functions, saving conversions from IPAddress and ip_addr_t

Please also initialize _type as it is not always initialized. (e.g. when using fromString4 or fromString6)
So maybe the type could be extended to have a 3rd type: NotSet or something similar.
But then the checks for the _type should include both checks, for IPv4 and IPv6 as now it is more like a boolean check. It is either IPv4 or IPv6.

Esp-idf has a 'not set' IP although I'm not a big fan because it creates confusion to users and needs additional tests.

I'm more in favor of having IP ADDRESS always initialized to ANY to the widest supported length, I.e. v6 of LWIP_IPV6 is defined

_type as it is not always initialized

fromString is an instance method, not static/factory. By default _type is initialized to IPv4, to be backwards compatible with existing code. Using xxx(IPv6) initialises to IN6ADDR_ANY.

(Which I noticed setting the constant was missing / not copied from ArduinoCore-API).

If you then call fromString() and it successfully parses as either 4 or 6 then it will be set accordingly.

If it fails to parse, then it will remain as is (usually IPv4), but the byte values will be meaningless (partially parsed).

Maybe if parse fails, then it should set to all zeros? (which will be IPv4 ANY or IPv6 ANY, depending on the original)

@sgryphon I guess you're right.

Missed that one. I haven't run into situations where the _type variable was uninitialized, just looked through the code to see if all were initialized as I've been bitten by uninitialized variables myself quite a lot lately and thus was looking through the code to see if all were initialized.

Maybe if parse fails, then it should set to all zeros? (which will be IPv4 ANY or IPv6 ANY, depending on the original)

I guess it should be set to some default constructed state, where it would be obvious to take the default IPv4 constructor for the fromString4 and IPv6 for the fromString6.

Then calls like isSet (or whatever the call is for IPAddress) will return false.

just to keep in mind to be on the same wave with LWIP (we have it under the hood anyway) lwip_ip_addr_type has only 3 options - IPADDR_TYPE_V4, IPADDR_TYPE_V6, IPADDR_TYPE_ANY for dual-stack. Maybe it's not a good idea to mix address type with object state itself, i.e. determine if it was initialized from a properly formatted string or not based on type value.

Ah yes, so the type itself should have an any type as well. (or maybe a bitmask, for IPv4 any and IPv6 any?, but this will break LWIP consistency, so maybe not...)
So I guess it should just not change the address in any way if the fromStringX call was not given a properly formatted string?

Yes, it should be consistent with LWIP.

I am not really sure what IPADDR_TYPE_ANY means; I know the comments says "for dual stack", which doesn't make a lot of sense, as dual stack generally just means running both IPv4 and IPv6. There isn't really such a thing as a dual stack IP address, e.g. "2001:db8::2:1" can't be dual stack.

There are IPv4-mapped IPv6 addresses and IPv4-compatible IPv6 addresses, which are used on some dual stack implementations to represent the IPv4 addresses internally, but they still aren't "dual-stack" addresses.

From looking at the code, it appears that IPADDR_TYPE_ANY is actually a special type that is applied to the ANY address (all zeros), so there are 3 types of any : IPv6 any, IPv4 any, and Any Any (which is either).

On a dual stack system you might want to list on IPv6 any, or IPv4 any, or Any Any (both of them).

So, regular addresses are either v4 or v6, whereas all zeros has 3 possible types v4, v6, or dualStack any.

I added a suggestion for this in the comments around the ANY constants.

s-hadinger · 2022-12-04T14:58:34Z

cores/esp32/IPAddress.h

@@ -83,14 +88,21 @@ class IPAddress: public Printable
    virtual size_t printTo(Print& p) const;
    String toString() const;

+    IPType type() { return _type; }


Can you please make it const:

IPType type() const { return _type; }

sgryphon · 2022-12-04T23:47:26Z

cores/esp32/IPAddress.cpp

+            _address.bytes[i] = 0;
+    }
+
+    _type = IPv6;
    return true;
 }



Need to also define the IN6ADDR_ANY constant

const IPAddress arduino::IN6ADDR_ANY(IPv6);

Based on discussion of type, and compatibility with LWIP, we might want 3 constants for ANY, i.e. representing to listen on all IPv4, all IPv6, or all (dual stack) addresses.

And corresponding third value value IPType (above):

enum IPType { IPv4 = 0, IPv6 = 6, //numeric values for compatibility with LWIP ANY = 46 // only used for dual stack INADDR_ANY to listen on all addresses }; IPAddress INADDR_NONE(0, 0, 0, 0); // Represents IPv4 any, for backwards compatibility, i.e. listen on all IPv4 address. IPAddress IN6ADDR_ANY(IPv6); // Represents IPv6 any, i.e. listen on all IPv6 address. IPAddress INADDR_ANY(ANY); // Represents dual stack any, i.e. listen on all addresses, both IPv4 and IPv6

s-hadinger · 2022-12-07T18:52:04Z

cores/esp32/IPAddress.cpp

+        return s;
+    }
+
+    // IPv4
    char szRet[16];
    sprintf(szRet,"%u.%u.%u.%u", _address.bytes[0], _address.bytes[1], _address.bytes[2], _address.bytes[3]);


This line is wrong now. It should be

sprintf(szRet,"%u.%u.%u.%u", _address.bytes[IPADDRESS_V4_BYTES_INDEX], _address.bytes[IPADDRESS_V4_BYTES_INDEX+1], _address.bytes[IPADDRESS_V4_BYTES_INDEX+2], _address.bytes[IPADDRESS_V4_BYTES_INDEX+3]);

Otherwise it always returns 0.0.0.0

s-hadinger · 2022-12-09T20:32:09Z

libraries/WiFi/src/WiFiClient.cpp

+
+    if (ip.type() == IPv6) {
+        struct sockaddr_in6 *tmpaddr = (struct sockaddr_in6 *)&serveraddr;
+        memset((char *) tmpaddr, 0, sizeof(struct sockaddr_in));


Shouldn't the size be sizeof(sockaddr_in6)?

Actually you can remove both memset by simply declaring

struct sockaddr_storage serveraddr = {};

which forces the compiler to initialize the structure with zeros

tablatronix · 2023-01-15T16:51:46Z

Is this expected? localip ?

[  2257][V][WiFiServer.h:41] WiFiServer(): WiFiServer::WiFiServer(port=80, ...)
[  2257][V][WebServer.cpp:87] WebServer(): WebServer::Webserver(port=80)
[  2266][E][WiFiServer.cpp:91] begin(): ---------------- IPv6
[ 67108][V][WebServer.cpp:296] handleClient(): New client: client.localIP()=0.0.0.0

sgryphon · 2023-01-16T01:02:47Z

Is this expected? localip ?

[ 67108][V][WebServer.cpp:296] handleClient(): New client: client.localIP()=0.0.0.0

Could be the bug that @s-hadinger mentions above, where the sprintf uses the wrong bytes?

nuclearcat · 2023-01-16T07:39:07Z

I am going to work on updating PR in 2 days, just was too busy. Will address this too...

s-hadinger · 2023-01-16T08:27:15Z

@nuclearcat I have already fixed and expanded this PR including using ip_addr_t, and adding support for UDP. It is now used by default in Tasmota for a few weeks and proved stable and solid.

I don't know how to do a PR on this PR so I will push a new PR, although I don't want to shadow your work.

The branch is here.
https://github.com/tasmota/arduino-esp32/tree/PR_IPv6

I will do a proper PR later today

nuclearcat · 2023-01-16T09:40:49Z

If it includes any of my work please include my Signed-off-by, then i think it will be ok, i will close mine after you send yours.

s-hadinger · 2023-01-16T19:34:35Z

#7722 sent as a superset for this PR.

tablatronix · 2023-09-16T12:03:05Z

What is the status of this afaik this is not fixed

[ 37114][V][WebServer.cpp:296] handleClient(): New client: client.localIP()=0.0.0.0

*wm:[2] [SYS] WM version:  v2.0.15-rc.1
*wm:[2] [SYS] Arduino version:  2.0.10
*wm:[2] [SYS] ESP SDK version:  4.4.5.230614
*wm:[2] [SYS] Free heap:        180812
*wm:[2] [SYS] Chip ID: 2665141460
*wm:[2] [SYS] Chip Model: ESP32-PICO-D4

s-hadinger · 2023-09-17T13:27:08Z

We will shortly propose a new IPv6 PR for Arduino 3 and esp-idf 5.1. It's already in extensive testing in Tasmota.

sgryphon and others added 11 commits November 27, 2022 18:06

Update header

2a668d2

Update IPAddress implementation to support V6

f93c766

Fix cut and paste error

184ad9a

Explicit not equals

e15cc72

Explicitly reference StreamString

d2178b1

Remove != test (it is causing a conflict)

cd126c0

Add WiFiTelnetToSerialIPv6 example

63552ff

To demonstrate new abilities of dual stack WiFiServer and remoteIP6 we add this example. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>

nuclearcat mentioned this pull request Nov 27, 2022

Adding IPv6 support to arduino-esp32 #6712

Closed

WiFiTelnetToSerialIPv6.ino: fix obsolete remoteIP6 call to remoteIP

7fa2767

Example contained API from previous IPv6 implementation, fixing it. Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>

sgryphon reviewed Nov 27, 2022

View reviewed changes

VojtechBartoska added this to the 3.0.0 milestone Nov 28, 2022

VojtechBartoska added the Area: BT&Wifi BT & Wifi related issues label Nov 28, 2022

TD-er reviewed Dec 3, 2022

View reviewed changes

s-hadinger reviewed Dec 4, 2022

View reviewed changes

sgryphon reviewed Dec 4, 2022

View reviewed changes

s-hadinger reviewed Dec 7, 2022

View reviewed changes

s-hadinger reviewed Dec 9, 2022

View reviewed changes

tablatronix mentioned this pull request Jan 15, 2023

wifi manager seems to redirect to 0.0.0.0 for the CaptivePortal tzapu/WiFiManager#1543

Closed

s-hadinger mentioned this pull request Jan 16, 2023

Ipv6 support v3 #7722

Closed

nuclearcat closed this Jan 16, 2023

Ipv6 support v2 #7520

Ipv6 support v2 #7520

Conversation

nuclearcat commented Nov 27, 2022

nuclearcat commented Nov 27, 2022

Choose a reason for hiding this comment

sgryphon Nov 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VojtechBartoska commented Nov 28, 2022

Choose a reason for hiding this comment

vortigont commented Dec 3, 2022 • edited Loading

sgryphon commented Dec 3, 2022 • edited Loading

vortigont commented Dec 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-hadinger Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tablatronix commented Jan 15, 2023 • edited Loading

sgryphon commented Jan 16, 2023

nuclearcat commented Jan 16, 2023

s-hadinger commented Jan 16, 2023

nuclearcat commented Jan 16, 2023

s-hadinger commented Jan 16, 2023

tablatronix commented Sep 16, 2023

s-hadinger commented Sep 17, 2023

sgryphon Nov 27, 2022 •

edited

Loading

vortigont commented Dec 3, 2022 •

edited

Loading

sgryphon commented Dec 3, 2022 •

edited

Loading

s-hadinger Dec 7, 2022 •

edited

Loading

tablatronix commented Jan 15, 2023 •

edited

Loading