-
Notifications
You must be signed in to change notification settings - Fork 7.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
This is a continuation on the topic of adding IPv6 Support to ESP32 Arduino #9016
Conversation
👋 Hello me-no-dev, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
@Jason2866 @s-hadinger PTAL While this is not based on Espressif structures, it does support converting from one to the other and also adds support for zones. |
Thanks @me-no-dev I still strongly advise to use Also |
cores/esp32/IPAddress.cpp
Outdated
char szRet[40]; | ||
snprintf(szRet, sizeof(szRet), "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x", | ||
_address.bytes[0], _address.bytes[1], _address.bytes[2], _address.bytes[3], | ||
_address.bytes[4], _address.bytes[5], _address.bytes[6], _address.bytes[7], | ||
_address.bytes[8], _address.bytes[9], _address.bytes[10], _address.bytes[11], | ||
_address.bytes[12], _address.bytes[13], _address.bytes[14], _address.bytes[15]); | ||
return String(szRet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: this is non-standard way of printing IPv6. I believe there was a discussion earlier about providing a good IPv6 printing funciton.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not disagree. But if you provide a good solution (that does not depend on lwip), it can be proposed and merged upstream, giving benefits to much wider audience :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it, mentioned here: #8907 (comment)
Some implementation is here: https://github.com/arduino/ArduinoCore-API/blob/82a5055a0588976c8df8c1ff3d978f62d68410f3/api/IPAddress.cpp#L302-L365
@s-hadinger here is the function: https://github.com/espressif/arduino-esp32/pull/9016/files#diff-723fb3da400ec1786a781f7ad5e3407c2c10b1f4009405806660ef4baf8f6b4dR389 I understand your reasons and I hope that you will understand ours. IPAddress is part of the Arduino Core API and the spirit of Arduino as a whole is to have as much portable code as possible. LwIP is not part of that official API, so others will not be able to use the code that we produce in cores and projects that work in different ways. With that said, we are all for expanding on that core API, so we do find it OK to extend the API with functions that allow more functionality. Currently you can convert the IPAddress to ip_addr_t, it's not hard to add the opposite as well. With such two functions, all other extra functionality you need can be implemented. |
Ok, thanks. As long as we can convert from one to another, I'm good with it :) |
For what it's worth, when you add the IPv6 printing function, don't forget the zone output as well. Here is the snippet we use:
|
libraries/WiFi/src/WiFiGeneric.cpp
Outdated
* @return 1 if aHostname was successfully converted to an IP address, | ||
* else error code | ||
*/ | ||
int WiFiGenericClass::hostByName6(const char* aHostname, ip_addr_t& aResult) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think hostByName6 is not a good approach. See #9009
Backwards compatibility should simply leave IPv6 off. Basic support of IPv6 wants hostByName() to support both 6 & 4 (whichever is relevant). Advanced IPv6 wants multiple address support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be changed. will be one version for all.
@s-hadinger please check 305d276 for the mentioned changes |
@s-hadinger @sgryphon last commit also changes |
// A class to make it easier to handle and pass around IP addresses | ||
#include "Printable.h" | ||
#include "WString.h" | ||
#include "lwip/ip_addr.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to have a separate IPAddressConverter class, that converts to & from LWIP? That would mean you don't need the dependency here. Just an idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it make more sense to have a constructor accepting the LWIP implementation and some set/get-functions?
If you want, you can also wrap those in some #ifdef
or #if
check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep it now as it is. It has the conversion methods and is not a big deal to make them used in constructors, but I would rather not pollute the class with non-Arduino things.
IPv4, | ||
IPv6 | ||
}; | ||
|
||
class IPAddress: public Printable | ||
{ | ||
class IPAddress : public Printable { | ||
private: | ||
union { | ||
uint8_t bytes[16]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a discussion (and another PR... or maybe it was in ArduinoCore) about removing the union, as technically conversions between the different types in a union is undefined. Just have a single uint8_t bytes[16] array, and then cast as necessary. (although I'm pretty sure the standard C socket interface uses a very similar union).
Anyway, I don't think the explicit union adds a lot of value, especially as we are mostly just accessing it through Arduino methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not a fan of union, but how it is used here, you also get the IPv4-as-IPv6-address implementation for free. (unless I'm making the same mistake as always where I mess up the bit order in my mind, exactly the reason why I'm not a fan of unions but still use them myself...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole class (both header and implementation) I copied directly from Arduino's Core API. Anything that is not to/from_ip_addr_t
and _zone
is a carbon copy of the official code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an effort to change the approach in Arduino, if that happens so, we will update the class here too.
return _address.bytes[index]; | ||
} | ||
|
||
size_t IPAddress::printTo(Print& p) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff seems to have a lot of unnecessary changes, e.g. reordering methods, that makes it hard to see what the actual changes are.
I suggest trying to keep the same order, so that the functional differences are easy to review. (Reordering can be done as a separate PR that makes no functional changes and only changes order).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reordering came because we moved to the official code from Arduino.cc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why in a separate PR?
I get it when you want those non-functional changes to be in a separate commit, but having them in a separate PR is a bit too much overhead tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I didn't check the commits. But if you directly copied from ArduinoCore, and then made changes, that's fine with me (seeing as a bunch of it was my code :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is exactly what I did :) just copied the official source from the core API and only then started modifying it (zone and to/from ip_addr_t)
@@ -336,6 +210,12 @@ bool IPAddress::fromString6(const char *address) { | |||
colons++; | |||
acc = 0; | |||
} | |||
else if (c == '%') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a simple enough change that I think it should work, but this is exactly the sort of thing it would be really nice to have unit tests for.
e.g. what happens if you try and parse "fe80::1:2:3:4%"? Should that be an error, because the zone identifier is blank?
(A unit test could explicitly specify this by provided the expected answer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input fuzzing to come in the future. For now just wanted to add zone support in the already written parser (from Arduino.cc)
So, to be specific, that is not a DHCP address, and not even a prefix from RA (router advertisement), but just an autoconfigured network your phone (or any device) can set itself without any instructions from the network. It then uses neighbour discovery to find others on the same link. The "AP DHCP issue is fixed" I presume is talking about DHCPv4. I don't know much about Access Point mode ... does it have a DHCPv4 server where it gives out IPv4 addresses when asked? For IPv6 to do similar it should use RA to advertise a ULA prefix, allowing others to connect (which avoids the issues using link-local zones). |
Well there was a separate issue which I had reported where no IP was being assigned at all since the DHCP service wasn't started when running the ESP in AP mode. And indeed it seems my Android phone does asssign this IP itself. I just made the remark about it as I was surprised to see it showed an IPv6 address on the connection info page. |
Hey @me-no-dev I'm trying to get this branch building with PlatformIO, but getting an error with a missing libs reference. I see this is working in the integration tests, and saw your name on the work. If you can help, can you email me sly @ gamertheory.net . Thanks. |
Added constructor that takes `const ip_addr_t *`. Added `addr_type()` getter Organize header to highlight the Espressif additions to IPAddress
@s-hadinger @TD-er check the latest commit please @s-hadinger could you or @Jason2866 help @sgryphon? |
@sgryphon This https://github.com/espressif/arduino-esp32/blob/master/tools/platformio-build.py#L40 is not working since the needed libs are not in Platformio registry. Needs to be changed to fetch from github https://github.com/espressif/esp32-arduino-libs/tree/idf-release/v5.1 The libs needs to be added here https://github.com/platformio/platform-espressif32/blob/develop/platform.json and https://github.com/platformio/platform-espressif32/blob/develop/platform.py The needed newer toolchains are available from Platformio registry. So you can add in platform.json and no need to load from espressif github. You can compare how i did for Tasmota https://github.com/tasmota/platform-espressif32/tree/Arduino/IDF5 |
cores/esp32/IPAddress.cpp
Outdated
@@ -413,5 +417,14 @@ IPAddress& IPAddress::from_ip_addr_t(ip_addr_t* addr){ | |||
return *this; | |||
} | |||
|
|||
esp_ip6_addr_type_t IPAddress::addr_type(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this function then be named ip6_addr_type()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TD-er we are procrastinating at this point... I like it the way it is. This PR needs to be merged already so we can progress further. Let's not get overly picky?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just mentioned it as it is a "breaking change" whenever it needs to be changed after this has been merged.
Also shouldn't it be a const
function?
Or does the Espressif code prohibit this (maybe you could use a const_cast
)
Anyway, do you plan on making a follow-up on this topic after it is merged?
I get it that this is blocking for your other PR with the network rework, so I do see the need for a quick merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can revisit everything after the overhaul. Let's get to use it a bit and see what would need adjusting.
Thanks Jason. Not fully there yet, but I got the project to download and build with the new libs. Project reference is here: https://github.com/sgryphon/iot-demo-build/blob/70ebc18cd598062b0116e8b72674235eeb738ff9/m5stack/m5core2_wifi_https/platformio.ini#L21 The fork I am using of platform-espressif32 is here: https://github.com/sgryphon/platform-espressif32/commits/feature/arduino-esp32-v3-libs-2 It is a pretty simple change, similar to what was done to get the integration tests working, i.e. just add the library. The pointers from @Jason2866 really helped. The libraries load, but my project is failing for other reasons... it looks like M5Stack is using some outdated (SPI) driver libraries and so getting compile fails, and some manual esp_netif_ calls. I might try and get a simple sample (maybe one of the integration tests) compiling ... but will do that another day. |
libraries/WiFi/src/WiFiGeneric.cpp
Outdated
@@ -1113,6 +1126,9 @@ esp_err_t WiFiGenericClass::_eventCallback(arduino_event_t *event) | |||
|
|||
} else if(event->event_id == ARDUINO_EVENT_WIFI_AP_START) { | |||
setStatusBits(AP_STARTED_BIT); | |||
if (getStatusBits() & AP_WANT_IP6_BIT){ | |||
esp_netif_create_ip6_linklocal(get_esp_interface_netif(ESP_IF_WIFI_AP)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing a race condition here. esp_netif_create_ip6_linklocal
returns -1
because:
get_esp_interface_netif(ESP_IF_WIFI_AP)->lwip_netif->flags
has the flag NETIF_FLAG_UP
not set when it is called. However if I add
delay(1)
the flag NETIF_FLAG_UP
is set and the link-local address is successfully assigned.
I have no clue why and I have never seen this behavior with our previous code.
Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed some weir stuff on the AP interface. I will report those and see what is causing it. On our end, it's as it should. We need to do this on START for the AP and CONNECT on the rest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry. The race condition is at ARDUINO_EVENT_WIFI_STA_CONNECTED
, the two codes look alike, I mixed up both. Let me copy it at the right place.
libraries/WiFi/src/WiFiGeneric.cpp
Outdated
} else if(event->event_id == ARDUINO_EVENT_WIFI_STA_CONNECTED) { | ||
if (getStatusBits() & STA_WANT_IP6_BIT){ | ||
esp_netif_create_ip6_linklocal(get_esp_interface_netif(ESP_IF_WIFI_STA)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting back the problem where it belongs. Sorry for the confusion.
I'm seeing a race condition here. esp_netif_create_ip6_linklocal
returns -1
because:
get_esp_interface_netif(ESP_IF_WIFI_AP)->lwip_netif->flags
has the flag NETIF_FLAG_UP
not set when it is called. However if I add
delay(1)
the flag NETIF_FLAG_UP
is set and the link-local address is successfully assigned.
I have no clue why and I have never seen this behavior with our previous code.
Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not check the return value, but other than that IPv6 works normally over STA and ETH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did update to the later platform builds @Jason2866 made including later IDF code and the final merged IPv6 code and ran into similar issues as described by @s-hadinger
But now the really strange part(s)....
- ESP32-S2 is getting IPv6 just fine, no need to add an extra delay
- ESP32 (classic) is not getting IPv6. Neither on WiFi nor on Ethernet (LAN8720A)
N.B. This was working fine about 2 weeks ago when I was running platform builds before your code was merged, but this also had older IDF code)
Now I added this delay to the WiFi connected event and it is showing IPv6 addresses just as before.
So I also added it to the Ethernet code and now the really funky part.
The ETH class doesn't show any IP address (0.0.0.0
for IPv4 and ::
for IPv6) when I query it, but the ESP is accessible via Ethernet on its assigned DHCP IP.
Still have to look into this a bit more to see if it does also get IPv6.
If it does, then this is more like an event from IDF code is not received/processed by the Arduino code and not about not getting the IP assigned.
Also I will check the other ESP32-variants like C3/C2/C6/S3 to see if those work just fine without this delay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the 0.0.0.0
/ ::
issue for Ethernet....
That was a PEBKAC problem.
Anyway the delay is still needed for ESP32 classic boards.
Still have to check the other ESP32-variants to see if these also need this delay.
{ | ||
// A class to make it easier to handle and pass around IP addresses | ||
|
||
enum IPType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you define enum IPType : uint8_t {
By default enum
is size of int
and takes 32 bits, which would be a waste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather leave it as-is. If it's OK to waste 24 bits per value on 2KB RAM m328p, then it should be OK on chips with 300+KB of RAM. This is original Arduino.cc code and I would rather modify as little as possible from it.
@TD-er First tests are successful, except the race condition with Lwip when trying to get a link-local address. But I have a workaround for now. I will test further but from what I can see I'm ok to merge. |
/* Dual-stack: Unmap IPv4 mapped IPv6 addresses */ | ||
if (remote_ip.type() == IPv6 && ip6_addr_isipv4mappedipv6(ip_2_ip6(&addr))) { | ||
unmap_ipv4_mapped_ipv6(ip_2_ip4(&addr), ip_2_ip6(&addr)); | ||
IP_SET_TYPE_VAL(addr, IPADDR_TYPE_V4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry there is one last problem here. IP_SET_TYPE_VAL(addr, IPADDR_TYPE_V4);
changes addr
but not remote_ip
so this code does nothing.
So I propose to add:
remote_ip = IPAddress(addr.u_addr.ip4.addr);
Or if you prefer:
remote_ip.from_ip_addr_t(addr);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do have the constructor now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, now it's all good for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good to me. Everything I was going to point out has already been done by another user. Just some nitpicks to make the code cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good to me. Tested with HTTPClient and WebServer.
Merge :-) |
Awesome work! Tasmota has now moved to this IPv6 version. |
Not sure if notifications get sent for merged PRs, but I just wanted to post an update that I got around to testing with my M5Stack kit. IPv6 is working in dual stack, including configuring both prefixes (on general global and one ULA) advertised by my router (i.e. get 3 IPv6 with the link-local as well). [ 1679][D][WiFiNetworkManager.cpp:34] wifiOnEvent(): [Network] WiFi station start Requests to an IPv6 test dual stack endpoint show that it can see my remote address (TLS was not working): [ 36859][I][Core2Logger.cpp:198] log(): [Core2Logger] CORE2: Button 1, scenario 0, v0.1.0-150-g27c8f84-dev Running on my IPv6 only network, it connects to the network and gets IPv6 addresses, but DNS is empty so HTTP isn't working: [ 1682][D][WiFiNetworkManager.cpp:34] wifiOnEvent(): [Network] WiFi station start [ 18509][I][Core2Logger.cpp:198] log(): [Core2Logger] CORE2: Button 1, scenario 0, v0.1.0-150-g27c8f84-dev |
Original: #8907
Discussion: #9009
Closes: #9069
Closes: #5624
Closes: #9085
Closes: #7065