Adding IPv6 Support for Arduino 3.0.0 #8907

Jason2866 · 2023-11-21T17:22:16Z

Follow up or better replaces PR #7722
Port is done by @s-hadinger

fyi and review @P-R-O-C-H-Y

VojtechBartoska · 2023-11-22T12:19:37Z

Thanks @Jason2866 for this PR.

Just writing a note to close/process other Ipv6 PRs if this one will be merged.

cores/esp32/IPAddress.cpp

cores/esp32/IPAddress.h

libraries/WiFi/src/WiFiUdp.cpp

me-no-dev · 2023-11-22T19:06:26Z

Why not use the official IPAddress class from Arduino.cc? https://github.com/arduino/ArduinoCore-API/blob/master/api/IPAddress.h

libraries/WiFi/src/WiFiClient.cpp

me-no-dev · 2023-11-22T19:11:41Z

libraries/WiFi/src/WiFiGeneric.cpp

+ * @return 1 if aHostname was successfully converted to an IP address,
+ *          else error code
+ */
+int WiFiGenericClass::hostByName6(const char* aHostname, ip_addr_t& aResult)


this should accept IPAddress instead. We use Arduino APIs if we can. Also this function is not guarded if v6 is enabled.

I will check this one, I think it comes from ESP8266

me-no-dev · 2023-11-22T19:15:01Z

libraries/WiFi/src/WiFiGeneric.h

@@ -196,6 +202,7 @@ class WiFiGenericClass
    static const char * getHostname();
    static bool setHostname(const char * hostname);
    static bool hostname(const String& aHostname) { return setHostname(aHostname.c_str()); }
+    static int hostByName6(const char *aHostname, ip_addr_t& aResult);


This method should be private and user should need to call only hostByName. You can add extra argument to hostByName that can force v6 answer, but if not set, there should be a logic to pick which result to give.

Actually I agree, this code comes from another source (can't remember which) and we don't use it in Tasmota.

Host are more likely to have multiple IPv6 addresses than multiple IPv4, but either is possible, so rather than one hostByName() with one address results, there probably should be a way to get multiple results.

For a function that only returns one result, it should kind of follow the logic of RFC 6724. Usually that logic is used to select both the source and destination address, given a list of both. e.g. if both have a local scope use that.

However in the case of hostByName we only have the destination address (not the source); as it is a DNS lookup, it is probably also a global address, and for safety we should assume that, i.e. return a global address in preference to a local scope one.

Generally then the algoritm would preference IPv6 over IPv4 over deprecated addresses.

I have a very rough algorithm GsmModemCommon::compareIPAddress at https://github.com/sgryphon/AdvancedGsmClient/blob/main/src/AdvancedGsm/GsmModemCommon.cpp

Note that it was written pre-support for IPv6 and so operates on raw strings. This means scopes like link-local are determined by "fe80:" prefix, rather than actual bytes. But it could be converted relatively easily.

To be fair, I don't use this one anymore in Tasmota. I call directly dns_gethostbyname_addrtype() with some custom logic to favor IPv4 or IPv6 depending on configuration of options.
I'm ok with any implementation.

libraries/WiFi/src/WiFiMulti.cpp

libraries/WiFi/src/WiFiServer.cpp

s-hadinger · 2023-11-22T21:34:56Z

Why not use the official IPAddress class from Arduino.cc? https://github.com/arduino/ArduinoCore-API/blob/master/api/IPAddress.h

The official is missing too many features. This one should be an superset, and it is well aligned with ESP8266

me-no-dev · 2023-11-23T07:43:51Z

The official is missing too many features. This one should be an superset, and it is well aligned with ESP8266

My issue is mainly that it depends on esp-idf as is the code currently now. I would like those classes to have nothing to do with the underlying OS (just like they are in mainstream Arduino).

sgryphon · 2023-11-24T06:41:08Z

libraries/WiFi/src/WiFiSTA.h

@@ -92,6 +92,7 @@ class WiFiSTAClass
    uint8_t subnetCIDR();

    bool enableIpV6();
+    bool IPv6(bool state);
    IPv6Address localIPv6();


Is this now to be marked deprecated (i.e. use IPAddress in preference to IPv6Address).

Also, will the localIP() function be updated to return IPv6 addresses, e.g. if IPv6-only or dual stack?

True that we don't need IPv6Address anymore.

For localIP(), we had already the discussion with Core2. For compatibility, I think that localIP() should only return IPv4, otherwise a lot of code will break. We need a separate function to get local IPv6

Should you be able to guard with LWIP_IPV6 or maybe some additional attributes?

Legacy code that isn't recompiled should be fine. Legacy code with LWIP_IPV6 will also be fine (previous behaviour).

Even with LWIP_IPV6 turned on, if you don't enable WiFiSTA IPv6 then it won't get a v6 address, i.e. localIP() will only have an IPv4 to return. i.e. legacy code will be fine as long as it doesn't enable IPv6.

At the point you call IPv6(true) to enable IPv6, I don't think it is legacy code anymore, and you shouldn't be surprised if legacy handling of localIP() fails after that.

What would be weird is if you turned on all the IPv6 stuff, e.g. called IPv6(true), and then localIP() did not return IPv6.

Maybe you do need a way to show multiple local addressess, i.e. pick to get only IPv4, but while there might be one IPv4, often there are multiple IPv6.

sgryphon · 2023-11-24T06:49:35Z

libraries/WiFi/src/WiFiSTA.cpp

+       WiFiGenericClass::clearStatusBits(WIFI_WANT_IP6_BIT);
+   return true;
+}
+


There is also a bunch of code, like localIP(...) that only returns the IPv4 version. If IPAddress now supports both, then it should return both types of values as appropriate, e.g. if IPv6 only or dual stack.

There should also be tests to ensure things like config(IPAddress local_ip, ...) support configuration with a static IPv6.

See above, if legacy localIP() returns IPv6, a lot of legacy code will break

See above, if legacy localIP() returns IPv6, a lot of legacy code will break

No it won't.

If you have IPv6 turned off (e.g. WIFI_WANT_IP6_BIT) then you will be backwards compatible, e.g. comparing strings, storing in 15 chars, display on screen, user input, etc. will all work. In an IPv4 only network, localIP() will only ever return an IPv4 address.

If you have legacy code that does not support IPv6, then don't turn IPv6 on.

However, once you have updated all your code to support IPv6, then setting the flag should work. You don't want to have to do additional work to go through your entire code base and convert a single call to localIP() to a complex set of checking two separate functions and then deciding which is best.

me-no-dev · 2023-11-24T10:29:55Z

@sgryphon there is a lot more work that will be done. Not just IPv6 but the whole network stack. Many things will change places. We still unfortunately need to keep compatible with original Arduino WiFi lib, ESP8266 and so on.

s-hadinger · 2023-11-24T16:16:28Z

The official is missing too many features. This one should be an superset, and it is well aligned with ESP8266

My issue is mainly that it depends on esp-idf as is the code currently now. I would like those classes to have nothing to do with the underlying OS (just like they are in mainstream Arduino).

I will at least check that the public interface is the same

SuGlider

The official is missing too many features. This one should be an superset, and it is well aligned with ESP8266

My issue is mainly that it depends on esp-idf as is the code currently now. I would like those classes to have nothing to do with the underlying OS (just like they are in mainstream Arduino).

After reviewing this PR, I agree to @me-no-dev.
The IPV6 support should use not just IPAdrress, but also IPv6Adress -- maybe unify both...

The WiFi/Eth and other classes shall identify which address type (v4|v6) is used and execute the IDF Calls related to it. We shall make it as transparent as possible and correctly organized in software layers, trying to unify the methods.

There may be some work to be done here.

s-hadinger · 2023-11-26T17:22:39Z

There may be some work to be done here.

Agree with this, but I'm not equipped to make such big changes. Are you taking over from now?

SuGlider · 2023-11-26T19:12:04Z

There may be some work to be done here.

Agree with this, but I'm not equipped to make such big changes. Are you taking over from now?

Thanks @s-hadinger, @sgryphon and @Jason2866

We will discuss it here and take over based on your good work!

TD-er · 2023-11-26T21:27:31Z

After reviewing this PR, I agree to @me-no-dev. The IPV6 support should use not just IPAdrress, but also IPv6Adress -- maybe unify both...

I am in favour of making it separate classes.
Not in the least because lots of legacy code, settings etc. assume 4 bytes for an IP-address.
But also because it isn't mutual exclusive IPv4 xor IPv6.
For example:

result from a DNS lookup can have both an IPv4 and IPv6 address)
IP address of an interface can have IPv4 and IPv6 addresses (even multiple and also with different scope)

Also if you have a single class for both IPv4 and IPv6, you still need quite a lot of checking in user code.
And IPv6 does have quite a few features completely unknown to IPv4, so it makes no sense to have functions for those in the class which also needs to support IPv4.

You could consider making a single wrapping class or helper to keep the result of a DNS lookup or "the IP-address" of an interface.
For example with a pointer to an IPv4 and a pointer to an array of IPv6 addresses.
Preferrably with some kind of smart pointer concept to make sure the user doesn't have to worry about deleting stuff.

But please keep IPv4 and IPv6 as separated classes.
This will keep functions that really need only IPv4 or IPv6 much simpler.

The WiFi/Eth and other classes shall identify which address type (v4|v6) is used and execute the IDF Calls related to it. We shall make it as transparent as possible and correctly organized in software layers, trying to unify the methods.

There may be some work to be done here.

Not sure whether WiFi/Eth etc should be aware of layer-3 (and above) stuff.
So in principle such interfaces do not need to know about IPv4 or IPv6.
They only handle layer-2 stuff.

me-no-dev · 2023-11-27T08:11:54Z

@TD-er Mainstream Arduino has one class for both v4 and v6 IPs and we need to stick to that. We could probably extend the IPAddress class into two new classes IPv4Address and IPv6Address to be used where we need a specific type of address, or just blankly accept IPAddress if it does not matter. In any case, we need to pick a route now and stick to it.

If we properly integrate this and make sure that all code does not care about the address type, we can make it work. The network itself does not care much what address it would connect to, as long as it can reach it.

Since we do not now support IPv6 properly, we could ease into it a bit, use a common IPAddress class and see how thing will go, then expand from there.

TD-er · 2023-11-27T09:23:23Z

@me-no-dev Do you have a link to this Arduino class implementation you're referring to?

Is it this one? https://github.com/arduino/ArduinoCore-API/blob/master/api/IPAddress.h
At first glance, it does look quite similar to what's being done for ESP8266 and also what @Jason2866 (and others from the Tasmota team) have done for ESP32.

What you could do is let new classes IP4Address and IP6Address inherit from IPAddress, to allow function signatures match on a specific type when required.
For example a Matter class should be using strictly IPv6.

As you can see, it was already a poor design decision for Arduino to try to merge IPv4 and IPv6 in the same class as the many comments in the .h file need to make clear whether a function is IPv4 only.
And even functions like operator== do not even seem to work for IPv6 addresses.

Also the default constructor doesn't seem to set all bytes to 0, so the other operator== function will not even work predictable.

bool test() {
  IPAddress ip1, ip2;
  for (int i = 0; i < 4; ++i)
  {
    ip1[i] = i;
    ip2[i] = i;
  }
  return ip1 == ip2;
}

Not tested, but above function will probably return false most of the times, unless there is some compiler flag used to initialize all memory to 0.

TL;DR
When you really want to follow the Arduino implementation, please do not include those clear bugs ;)

TD-er · 2023-11-27T09:27:07Z

cores/esp32/IPAddress.cpp

+  _ip = *IP6_ADDR_ANY;
+#else
+  _ip = *IP_ADDR_ANY;
+#endif


Default constructor does not set all bytes to 0.

sgryphon · 2023-11-27T13:27:41Z

@me-no-dev Do you have a link to this Arduino class implementation you're referring to?
Is it this one? https://github.com/arduino/ArduinoCore-API/blob/master/api/IPAddress.h

Yes, I think that is the one.

And even functions like [operator==] do not even seem to work for IPv6 addresses.

Comparing, via ==, to a raw pointer (the specific version of == you linked to) is a bit of an uncommon thing to do.

The other version of == in ArduinoCore-API, comparing two instances, does work for IPv6.

I mean you can't even assume the pointer has at least 4 bytes, but if it didn't then why would you even be comparing. If you did need to use this specific function (unlikely), and wanted to support IPv6, then a small amount of additional code would be needed for those checks. (but far less than if there were two completely separate v4 and v6 classes)

Also the default constructor doesn't seem to set all bytes to 0
bool test() {
IPAddress ip1, ip2;
Not tested, but above function will probably return false most of the times

~~There is a test suite, so if you are concerned you could add your test there, or raise an issue.~~

There is already a test for the default constructor, but you could add more if you like (and test them): https://github.com/arduino/ArduinoCore-API/blob/1cec094bddec923824f48a756455d5a75215102f/test/src/IPAddress/test_IPAddress.cpp#L17

~~Also, the default constructor does appear to do a memset on all bytes to zero: https://github.com/arduino/ArduinoCore-API/blob/1cec094bddec923824f48a756455d5a75215102f/api/IPAddress.cpp#L30~~

Sorry, I though you were talking about a bug in ArduinoCore-API not setting the default constructor and failing the == test, but I release you were talking about this code (it kind of ran together straight after talking about Core).

sgryphon · 2023-11-27T13:52:42Z

I am in favour of making it separate classes. Not in the least because lots of legacy code, settings etc. assume 4 bytes for an IP-address.

Legacy code should not enable IPv6 and so will work exactly as it does today. Only enable IPv6 if you want to use IPv6; and if you want to support it then you will need to update settings, etc.

But also because it isn't mutual exclusive IPv4 xor IPv6. For example:

result from a DNS lookup can have both an IPv4 and IPv6 address)

IP address of an interface can have IPv4 and IPv6 addresses (even multiple and also with different scope)

Kind of... a DNS lookup (or an interface) may have more than one results, e.g. 7 results. Those results could be any mix of IPv4 and IPv6 results. So you would have 7 instances of the IPAddress class. At least you can treat all instances the same, e.g. loop through and try to connect.

If you had two classes, and two methods, you would need to write all the code twice: once to get the 2x IPv4 addresses, and once for the 5x IPv6 addresses (or whatever combination you have).

The fact that you can have multiple of both, and they are not mutually exclusive, I think it would be easier to manage one collection of 7 instances, rather that two collections (of 2 and 5, or 0 and 7, or 7 and 0, etc).

Also if you have a single class for both IPv4 and IPv6, you still need quite a lot of checking in user code.

More likely the other way around.

When you do finally want to upgrade legacy code to IPv6, then enable it and recompile. Most things that pass addresses around, etc. will just work (just using 16 bytes instead of 4). There will be a few places where assumptions are not correct, e.g. if you store a config in a fixed size string database, or try to output to a fixed size string on screen.

To support IPv6 these will need to be updated, e.g. more space on the screen for the string) no matter what.

However, if you had two classes then you would need to duplicate all your IPAddress handling code, to check both 4 and 6, and determine if you have one or both, and then pick which one to use. You will still need to make the same change to your string storage, to store the picked value, just have to double everything else on top.

e.g. with one address class type you can (psuedo-code):

IPAddress found;
IPAddress addresses[10];
GetAddresses(addresses, max: 10);
foreach (address in addresses) {
  if (TryConnect(address)) {
    found = address;
    break;
  }
}

With separate classes you would need separate lists of IPv4 addresses and IPv6 addresses, then need to loop through both. And how would you prioritise the order between the two types, which you could not do if they were in separate lists?

And IPv6 does have quite a few features completely unknown to IPv4, so it makes no sense to have functions for those in the class which also needs to support IPv4.

Most applications don't care about IPv4 vs IPv6, they just want a connection to send stuff.

i.e. (pseudo code)

address = hostByName(name);
connection = connect(address);
connection.send(..)

It would be rare for most business or consumer applications to need to know whether "address" is IPv4 or IPv6 or to care about any of the low level differences.

Note: You could have IPv4Address and IPv6Address inherit from a generic IPAddress... (other object oriented languages do this) although the vast majority of code would never need to use the lower level classes. But probably not worth it.

compare the above to, assuming we would need something like:

address6 = hostByName6(name);
address4 = hostByName4(name);
if (address6 != null && address6.type != link-local) {
  connection = connect6(address6);
} else if (address4 != null && addres4.type != link-local) {
  connection = connect4(address4);
} else if (address6 != null) {
  connection = connect6(address6);
} else {
  connection = connect4(address4);  
}
connection.send(..)

This is assuming the preferred order of using addresses is IPv6 global, IPv4 global, IPv6 link-local, IPv4 link-local, and that each of the functions hostByName6() and hostByName4() gives back the prioritised address, but only from that family.

i.e. hostByName6() will give back IPv6 global, else IPv6 link-local, else nothing.

In contrast, a single hostByName() function can simply give you back the highest priority of all addresses

sgryphon · 2023-12-17T07:21:33Z

Rather than have discussions across multiple issues and pull requests, I created a general IPv6 discussion that can be used: #9009

me-no-dev · 2023-12-18T12:48:36Z

Arduino's version of IPAddress has been merged into master. Please rebase this PR to not modify that code.

Jason2866 · 2023-12-18T13:26:54Z

@me-no-dev A simple merge will break the function of the PR.

As already mentioned earlier we have no resources to keep this PR up to date, integrate and test changes made in branch master. Feel free to close or do cherry picking.

me-no-dev · 2023-12-18T13:31:09Z

@Jason2866 clarify please? We would like to base on the Arduino.cc IPAddress. If you need something extra, you can add it here, but please do not rewrite IPAddress to use Espressif structures and use the one we merged.

github-actions · 2023-12-18T13:35:42Z

	Warnings
⚠️	Some issues found for the commit messages in this PR: the commit message `"Fix WiFiServer IPv4 in IPv6"`: summary looks empty type/action looks empty the commit message `"Fix warning in WifiUdp"`: summary looks empty type/action looks empty the commit message `"Guard IPv6"`: summary looks empty type/action looks empty the commit message `"IPv6 for Arduino 3.0.0"`: summary looks empty type/action looks empty the commit message `"Remove` if (1)`"`: summary looks empty type/action looks empty the commit message `"Simplify` IPv6()`"`: summary looks empty type/action looks empty the commit message `"add comment"`: summary looks empty type/action looks empty the commit message `"remove comment / formating"`: summary looks empty type/action looks empty Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: `<type/action>(<scope/component>): <summary>`, for example `fix(esp32): Fixed startup timeout issue` allowed types are: `change,ci,docs,feat,fix,refactor,remove,revert` sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) `TIP:` Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).
⚠️	Please consider squashing your 8 commits (simplifying branch history).
⚠️	The source branch `"IPv6"` incorrect format: contains uppercase letters. This can cause troubles on case-insensitive file systems (macOS). Please rename your branch.
⚠️	The target branch for this Pull Request must be the default branch of the project (`master`). If you would like to add this feature to a different branch, please state this in the PR description and we will consider it.

👋 Hello Jason2866, we appreciate your contribution to this project!

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 5740c69

sdfsd

Jason2866 · 2023-12-18T14:18:45Z

@me-no-dev Don't want to do different. We have no resources to implement every needed! change here to get a nice and Arduino compatible IPv6 integration. You wrote earlier you take over this PR. So i am a bit surprised.
To be very clear, do whatever is needed with this PR. We are 100% fine.

me-no-dev · 2023-12-18T16:22:41Z

@Jason2866 closing this in favor of #9016 let's discuss what you might be missing there. We can not edit this PR, because of permissions :)

Jason2866 and others added 2 commits November 21, 2023 18:17

IPv6 for Arduino 3.0.0

23fa1f8

Fix warning in WifiUdp

a78841a

P-R-O-C-H-Y requested review from SuGlider, P-R-O-C-H-Y and lucasssvaz November 22, 2023 10:50

P-R-O-C-H-Y added this to the 3.0.0 milestone Nov 22, 2023

VojtechBartoska added Area: WiFi Issue related to WiFi Status: Review needed Issue or PR is awaiting review labels Nov 22, 2023

VojtechBartoska changed the title ~~IPv6 for Arduino 3.0.0~~ Adding IPv6 Support for Arduino 3.0.0 Nov 22, 2023

Jason2866 mentioned this pull request Nov 22, 2023

Ipv6 support v3 #7722

Closed

lucasssvaz reviewed Nov 22, 2023

View reviewed changes

cores/esp32/IPAddress.cpp Outdated Show resolved Hide resolved

cores/esp32/IPAddress.cpp Outdated Show resolved Hide resolved

cores/esp32/IPAddress.h Outdated Show resolved Hide resolved

Jason2866 added 2 commits November 22, 2023 16:01

remove comment / formating

13d55a7

add comment

0cff29d