Skip to content
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

Ipv6 support v3 #7722

Closed
wants to merge 49 commits into from
Closed

Ipv6 support v3 #7722

wants to merge 49 commits into from

Conversation

s-hadinger
Copy link
Contributor

Description of Change

This is a follow-up of #7520. @nuclearcat I don't know how to propose a PR on your PR, so I submit a brand new PR. Thanks for your work!

This is the full IPv6 implementation now used in Tasmota. It has been checked for Wifi, Ethernet, HTTP/HTTPS, UDP, ICMP6 and DNS6.

IPV6 must be enabled an esp-idf level:

  • CONFIG_LWIP_IPV6 to enabled support for IPv6
  • CONFIG_LWIP_IPV6_AUTOCONFIG=y to enable support for SLAAC, which configures an IPv6 global address, router and DHCP servers
  • CONFIG_LWIP_IPV6_RDNSS_MAX_DNS_SERVERS=2 to get DHCP servers from SLAAC

Compared to #7520:

  • IPAddress now uses ip_addr_t to make it simplet to interface with LWIP
  • IPAddress is now heavily based on ESP8266 https://github.com/esp8266/Arduino/blob/master/cores/esp8266/IPAddress.cpp
  • IPAddress default to IPv6 "any address" which makes TCP and UDP servers listen to both IPv4 and IPv6 by default
  • WiFiUdp now supports IPv6
  • multiple fixes to server code to support remote addresses in IPv6

@s-hadinger s-hadinger mentioned this pull request Jan 16, 2023
@VojtechBartoska VojtechBartoska added this to the 3.0.0 milestone Jan 25, 2023
@VojtechBartoska VojtechBartoska added the Status: Review needed Issue or PR is awaiting review label Jan 25, 2023
@VojtechBartoska
Copy link
Collaborator

Thanks for the PR @s-hadinger, adding this to 3.0.0 milestone, we will review this a bit later.

@MartinVerges
Copy link

IPv6 is an important topic, any chance to get that anytime soon? Kudos to the authors.

@Jason2866
Copy link
Collaborator

@MartinVerges
You can use the Tasmota Arduino framework core 2.0.7
It is here https://github.com/tasmota/arduino-esp32/releases
Full Platformio support here https://github.com/tasmota/platform-espressif32

me-no-dev and others added 4 commits March 1, 2023 16:37
…use with RS485 auto RTS (espressif#7935)

* Added setMode function to set the esp32 uart mode

Used to set the esp32 uart mode for use with RS485 Half Duplex and the auto RTS pin mode. This will set/clear the RTS pin output to control the RE/DE pin on most RS485 chips.

* Add Success (bool) return in some functions

* Add Success (bool) return code to some functions

* Add Success (bool) return to some functions

* Add Success (bool) return to some functions

* Fix uartSetRxTimeout return type

---------

Co-authored-by: Rodrigo Garcia <rodrigo.garcia@espressif.com>
…spressif#7913)

* Add v2.0.7 in issue template (espressif#7871)

* Fix the F_CPU frequency for the ESP32-S3

Hello, I was using the FastLED library and it was complaining about F_CPU not being defined. So, I just noticed that it is not defined for the ESP32-S3 module. So I made this change in the header file and it compiled. Therefore I wanted to propose this change to the HAL library to improve compatibility. Thank you for your time.

* Makes F_CPU generic based on the SoC frequency

Works for ESP32, ESP32C3, ESP32S2, ESP32S3

* Includes ESP32C3 in the F_CPU definition

Necessary for ESP32 Arduino Core 2.0.x based on IDF 4.4

---------

Co-authored-by: Vojtěch Bartoška <76958047+VojtechBartoska@users.noreply.github.com>
Co-authored-by: Rodrigo Garcia <rodrigo.garcia@espressif.com>
@@ -212,6 +218,7 @@ class WiFiGenericClass

public:
static int hostByName(const char *aHostname, IPAddress &aResult);
static int hostByName6(const char *aHostname, ip_addr_t& aResult);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you adding a separate interface just to call hostByName6? Obviously no clients are using it (because you just added it). I suggest removing it as unnecessary.

The purpose of DNS is to be address-agnostic, a client should be able to just call hostByName() and get back the corresponding IPAddress (4 or 6). (The priority order of addresses should be based on RFC 6724, usually IPv6 first if supported).

There should be no situations in which a separate call to hostByName6() (or hostByName4()) needs to be called.

It is true that hosts may have multiple addresses (they may have multiple IPv6 and/or multiple IPv4), but this doesn't really address this. e.g. if the first one failed, maybe you want to check the second -- but that applies to multiple addresses of any type.

Even if you made it a more generic IP type parameter, if you want to pre-filter for IPv4 or IPv6, then I struggle to see when you would need to use this. i.e. how is the client code ever supposed to know when it only wants IPv6?

It really should not care. It should say "given me the IP address for this host". If v6 is available, it will be returned, and if not then v4 will be.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about common use cases like ping6 host to explicitly check connectivity using IPv6? When I want to create a similar function, do you then need this or how can it be done? (asking out of curiosity)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so what is the relevance of ping6? You might want to know if your device can connect to server "example.org"; i.e. you send a message and the server receives it. What does it matter how it gets there? How is your app going to behave different? What logic would you want to write where you app does, or doesn't, do something based on ping6?

i.e. there might be logic based on whether an address (6 or 4) is reachable or not, but I can't think of any scenario where there is a specific dependence on which protocol is used.


NOTE: This is different from saying if ping address1 fails, then try address2, because that is more generic than which IP version is being used, e.g. if a1 & a2 are both IPv4, then if a1 fails you want to use a2, or if a1 & a2 are both IPv6, then if a1 fails you want to use a2. The scenario "if IPv6 fails use IPv4" is just a specific combination of the more generic check. e.g. if an address has a1 IPv6, a2 IPv6, and a3 IPv4, and ping a1 fails, then you would want to try a2 (not a3).

Also, if there is no IPv6 network (you don't have an address to send from) and there is only IPv4, then even if there are DNS entries for both AAAA and A, then the address selection function should, according to RFC 6724, only give you the IPv4 address (as it is the only reachable address).

There are some cases where a server has multiple addresses and you want to check the up/down of all the addresses, but again IPv4/v6 is not relevant, e.g. they might have 2x IPv6 and 2x IPv4.

Maybe in a very rare diagnostic app you want to be able to report if any of the IPv6 are working or not (vs any of the IPv4), but again such an app would want to check all addresses, not just the first IPv6 and then jump to the first IPv4.

The other time 4 vs 6 might be relevant is if you are implementing happy eyeballs type algorithm, where it tries both and uses the first that responds. IoT devices don't usually have this, but I guess it could be a case (and even though you would probably want all IPv6 and all IPv4, not just the first of each).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a network administrator I do such tests and checks quite often. A lot of tools provide ways to explicitly select the IP Version.

Lets assume I want to build a small device that tracks and checks connectivity like the existing RIPE Probe. This device would be in need of a way to test each protocol while being connected dual stack.

Please note, I clearly do not state this is the default, but in my experience it's a thing you often need to debug things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, diagnostics needs to cover both, and you probably want to report the stack as part of those diagnostics, but I don't know when you would want to write separate code paths, rather than one that simply returns them all.

e.g. say a server "example.org" has four configured addresses AAAA 2606:2800:220:1::1946, AAAA 2606:2400:1000:1::1946, A 93.184.216.34, A 193.184.216.35.

I can see the need to test all four network connections. Maybe you even want to filter on an attribute and only test the first two (the IPv6 ones).

At a code level, this only needs two method signatures:
hostByName(name, &result) -- where you just want the first one of them, i.e. a basic connection
hostsByName(name, results[], max_length), or similar -- get multiple.

You could always then filter results, if you wanted only the IPv6 addresses.

It would be a bit strange to build those filters into the signatures:
hostByName(name, &result)
hostByName6(name, &result)
hostByNameSecondResult(name, &result)
hostByName6SecondResult(name, &result)

And why write code for only two of those specific conditions (the first IP and the first IPv6).

Not to mention the logic to then handle if one of them is or isn't null (the host name may only have A, or only AAAA, or the network may be only IPv4 or only IPv6). That a lot of ifs and checking for nulls.

A simple hostsByName() handles all these cases, e.g. if the server has 3x AAAA and no A's, then it will return three records that you can loop through. If it only has a single A record, then only one item will be returned in results. The diagnostic code would be a lot simpler.

Not to mention that I think the proposed implementation of hostByName6() will return either IPv6 or IPv4 results, depending on what is available, i.e. if the server only has one A record, then it will return that IPv4 address.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, there is one case I guess, for the testing probe where you are connected dual stack, but want to test both v4 and v6.

If you are dual stack, especially if you have NAT64 running, then hostByName(), if supporting dual stack as I suggest, would under RFC 6724 always return the IPv6 address as the top result. You could have two different host names for the AAAA and A records, but that won't help if you have NAT64.

You could also recompile/rebuild without IPv6 enabled, but that would be time consuming and not always practical.

If hostByName() is always going to give you the RFC 6724, address, then you can test that and show if it is IPv4 it means there is no IPv6 (either network doesn't support it, server doesn't support it, etc). If there is an IPv6 then maybe you could have a method:

hostByName4()

that only resolves IPv4 addresses. If hostByName() gave you an IPv4, you don't need it, but if hostByName() gave a v6 address, you could use this second method to down test IPv4 is also working, even in dual stack.

I still think hostsByName() is a better approach, providing a full list, which you can then pick out the IPv6/v4 addresses as needed.

But sure, if you don't have something that returns a full list, want diagnostics to test both in dual stack, then as well as the main method (that returns the RFC 6724 address), you would need a second method to get the IPv4 only address.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are dual stack

I think that's the default for decades to come.
We will probably never completely get rid of IPv4.
I have dual-stack since 2010 here at home and only in the last few years I see people with new internet subscriptions to have some DS-lite or CG-NAT for IPv4. Some still don't even have IPv6.
I think DS-lite or CG-NAT for IPv4 is how it will be for the next decades.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a code level, this only needs two method signatures:
hostByName(name, &result) -- where you just want the first one of them, i.e. a basic connection
hostsByName(name, results[], max_length), or similar -- get multiple.

That should be ok for what I have in mind. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default for decades to come

Some ISPs are starting to deploy IPv6 only connections.

Some telcos have also moved to IPv6 only, e.g. my local carrier Telstra, in Australia, switched to single stack IPv6 several years ago, in 2020: https://www.sidn.nl/en/news-and-blogs/australias-telstra-switches-mobile-users-to-ipv6-only

The do provide NAT64+DNS64 / XLAT464, so that most IPv4 only services still work.

And technically you can still get single stack IPv4 if your mobile doesn't support IPv6.

But there is no dual stack. If you connect dual stack, then you only get IPv6. It is either SS IPv6 or SS IPv4.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here in the Netherlands, we have a few providers offering true dual stack, but those are more like the "tech enthusiast" or "professional" subscriptions.
The large "home" providers offer CG-NAT + IPv6. This way you stil have IPv4 but more like a very powerful router performing NAT for you.
This way you can't do port forwarding for IPv4 and also keeping up a VPN to an IPv4 end point is next to impossible.

I don't think that's the same as NAT64 or XLAT464 as it simply is like sharing your old IPv4 NAT router with your neighbors (but then separated).
For the end user there isn't much difference between CG-NAT + IPv6 and true dual stack, apart from not having your own IPv4 public address and not being able to do any port forwarding. (and the stability issues for services like VPN connections)

@@ -30,6 +30,7 @@

WiFiMulti::WiFiMulti()
{
ipv6_support = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the default? Why default to turning off half the internet? By default clients should probably support the full IP stack (both); note that if the network only gives an IPv4 address (or only IPv6), then the other half won't be used -- but we don't know in advance what the network will support (or potentially support in the future), so should generally accept "whatever IP the network provides".

Or is it to reduce the size of the binary? If so, why not also have ipv4_support flag. If you know you are going to be operating in an IPv6 environment (or dual stack), then you could reduce the size by getting rid of IPv4 handling.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can imagine this to be a proper default when the code for IPv6 in this SDK isn't yet fully mature.
Then the ones knowing about IPv6 will turn it on and experiment with it, ironing out all the bugs.

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

bool enableIpV6();
bool IPv6(bool state);
IPv6Address localIPv6();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, you should mark this as deprecated ... there should be no need for the client to have to handle the difference between IPv6 and not IPv6.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like above I would agree in theory, but existing code would brake. IPv6 should need to be explicitly accepted by the code running, or many things will break.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these were debug statements that needed to be cleaned up


/**
* Resolve the given hostname to an IP address.
* @param aHostname Name to be resolved
* @param aResult IPAddress structure to store the returned IP address
* @return 1 if aIPAddrString was successfully converted to an IP address,
* else error code
*/
int WiFiGenericClass::hostByName(const char* aHostname, IPAddress& aResult)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once IPAddress is generic, and can handle both 4 and 6, then you only need one version of this function (this name, but with IPv6 support), which will return whichever address is available (and don't need a separate hostByName6 method).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe good to have an optional 3rd parameter (enum class type) to set what kind of address you want.
The default being IPv6 if available, otherwise IPv4. (if IPv6 is enabled)

The reason one may explicitly want an IPv4 address could be for legacy reasons where a value needs to be stored and only 4 bytes are available for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid we have legacy here. Many of the existing APIs expect an IPv4, and code using it will crash badly if it's not. I don't think that adding a parameter will solve the backwards compatibility issue. That's why the hostByName6 exists, so if you accept v6, you should use the latter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value of such a parameter may be debatable.
However if a check like this is present in the code, you can simply have the default preferred IP type set to IPv4.

if (WiFiGenericClass::getStatusBits() & WIFI_WANT_IP6_BIT)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the hostByName6 exists

It looks like hostByName6 actually returns both IPv6 and IPv4 addresses (i.e. IPv6 if available, and IPv4 if not, as per RFC 6724).

So maybe they are just badly named, i.e. I would call the generic (return any IP) hostByName(), and then call one that only did IPv4 as hostByName4().

But I guess for legacy if there is a bunch of code that is using hostByName() and then sticking the result in a 4 byte storage then we probably do need to keep the name, no matter how bad it is.

In that case, the dual stack / any version (whatever is available; it's not really dual stack) probably still shouldn't be called with a '6', which is misleading.

Either call it hostByName2(), to simply show it is the newer version (that supersedes the earlier), or call it something like hostByNameDS() or hostAddressByName(). (Really, the whole issue about calling things 'New' or slight variations, e.g. DS, is what happens when you get a third, or fourth 'ReallyNew'... hence suffix 2, 3,... is a generic way that avoids these).

If we need to keep hostByName() as IPv4 only, for legacy, then I would recommend hostByName2() as the new version, that returns the 'best' address, i.e. IPv6 if available, or IPv4 if not.

The '2' suffix makes it clear it is a new version, that should be used for new development; code that uses it needs to handle that it may sometimes return an IPv6 address; and, if IPv6 is turned off, it is backwards compatible. i.e. new code, even if there is no current IPv4 planned, should still be using it, just in case (knowing it may have IPv6).

The '6' suffix is misleading.

Copy link
Contributor

@sgryphon sgryphon Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that doesn't make sense.

If you are doing stuff that doesn't support IPv6, like sticking values into 4 byte fields, wouldn't you just compile with IPv6 turned off. Then it would just work exactly the same as before, and there would be no compatibility issue.

Then, when you do find/fix whatever the limitation was, all you need to do is turn IPv6 on, and the rest of your code will work, without having to go around everywhere and change references.

Meanwhile, for the usual case, where the value is opaque, i.e. you just do a host lookup to get an IP address, then use that IP address to connect, without caring what is inside that value, then that code can take advantage of IPv6 without having to make any code change. The just use hostByName() and pass the result to connect(). (Without having to go through their code base and change hostByName() to hostByName2() everywhere). The only difference is they use slightly more storage.

They won't know, or care, if they are in an IPv6, dual stack, or IPv4 environment.

The only case where you would be looking inside addressess is for diagnostics, and there you might care about displaying what you got (like "hey, why is it giving me IPv4 when I should have IPv6 available"), and you might want a way to get back multiple addresses. But you don't want to have to write different code for IPv6 vs IPv4.

Given that with IPv6 turned off, the code behaves exactly the same as before, I don't see any reason for, when the flag is turned on, to have separate methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have legacy here. Many of the existing APIs expect an IPv4, and code using it will crash badly if it's not.

Yeah, if your code is going to crash with IPv6, then don't turn it on. Use hostByName() (no suffix/prefix) and compile with only IPv4 enabled.

When you do update the legacy code, so that is supports IPv6, then continue to use hostByName() and compile with IPv6.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really think adding a separate hostByName6() method makes things worse, as it confuses a client user what they should call.

The should be able to just call hostByName(), pass in the DNS name, and get back the address (v6 or v4), whatever is supported by the current network -- and noting that networks, and hosts, can change over time, e.g. maybe today it has one IPv4 address, tomorrow it has a different one, in the future it is dual stack, and maybe one day it is IPv6 only.

The client shouldn't need to know or handle this. It just passes in the name, and gets back an IPAddress (which could be a v6 or v4).

A useful extension would, however, be hostsByName(), to get multiple addresses, in case part of the network is down... try address 1 (6 or 4), then address 2 (6 or 4), then address 3 (6 or 4), etc.

(The order of destination addresses should be as per RFC 6724 for default address selection.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A useful extension would, however, be hostsByName(), to get multiple addresses, in case part of the network is down... try address 1 (6 or 4), then address 2 (6 or 4), then address 3 (6 or 4), etc.

That's rather tricky as it involves allocations and making the caller responsible to manage the de-allocation.
Also I don't think there are lots of use cases for IoT stuff where you will see multiple IP-addresses for the same hostname (apart from IPv4/IPv6)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see value in changing the APIs here. Multiple addresses should be managed by caller, and would be a rare case anyways.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's rather tricky as it involves allocations and making the caller responsible to manage the de-allocation.

The way I've done it before is have a signature like hostsByName(name, addresses[], max_results)

And have the caller allocate an array for how many results they want, e.g. 4, so that you don't need any allocations. Not perfect, and there is a trade off between unused stack and dynamic allocation.

For destination addresses / DNS lookup, you probably are safe with just one of each (v6/v4), as usually a DNS lookup won't return scoped local addresses, ULAs, etc.

(In contrast locaIP() for IPv4 is usually only one address, either private, public, or link local, whereas for IPv6 there is usually at least two local IP addresses (link local and public), and possibly more (temporary public, other scopes, ULAs))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least, don't call the function hostByName6(), especially if it returns both v4 and v6 addresses.

My preferences/recommendation:

  • Just have a single function, hostByName().
  • With CONFIG_LWIP_IPV6 turned OFF, this functions exactly as before, and only returns v4 address
  • With IPv6 turned on, it returns either a v6 or v4 (same as the suggested hostByName6()), with v6 as preference.
  • This means to upgrade agnostic code to use IPv6, all you need to is turn the flag on. You never need to see the underlying IP addresses (or their type), just refer to destinations by name, e.g. "foo.example.com", and you will get the best address available (including if it changes in the future).
  • Note that an address than only have v4, or on a v4 only network, will continue to use v4 so will still work fine. IPv6 is only used when available and supported.

Second option, if you want some support for diagnostic testing:

  • Same as above, but when CONFIG_LWIP_IPV6 = ON, add a hostByName4() function, or a hostByName( ) function that takes a "type" parameter for v4/v6. This is normally no use, and no point to add it if V6 is off, but when V6 is on allows you to get a v4 response (if available) for debugging.
  • I think a multiple address result (with the caller allocating) is more flexible, e.g. for diagnostics it allows multiple. But a secondary option is this alternative. (And documentation should mark it as only useful for diagnostics; in normal circumstances you don't need it, as hostByName() will return v4 when needed; filtering the result type is only useful when (a) you are running dual stack, (b) the network is dual stack, (c) the target is dual stack, but (d) you want to skip the IPv6 result and test the IPv4 result, e.g. for diagnostics.

Third option (and a bad one):

  • At least don't call it hostByName6(), which is misleading, but call it hostByName2() or hostByNameAny().
  • This is a really messy upgrade path, as you can't just turn IPv6 only, but need to go around everywhere and change all calls to hostByName() to hostByName2().
  • It also leaves a trap, that even if IPv6 is on and working fine, a coder may accidentally use the old hostByName() and get stuck in using legacy IPv4.
  • All new code should be encourage to use the hostByName2() function
  • The downside of this is that new code is then backwards compatible, i.e. if you have working dual stack code (using hostByName2()) but want to force IPv4 via CONFIG_LWIP_IPV6 = OFF (maybe your environment has a broken IPv6 implementation), then you need to also change all your code back to hostByName().
  • This really is a bad option having an alternative function name.

@@ -294,6 +309,19 @@ int WiFiClient::connect(const char *host, uint16_t port)

int WiFiClient::connect(const char *host, uint16_t port, int32_t timeout)
{
if (WiFiGenericClass::getStatusBits() & WIFI_WANT_IP6_BIT) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no need for this check here.

You only need one version of hostByName() (and inside that function it should return IPv6 if available, and IPv4 if not, i.e. the check should be inside hostByName()).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To this I agree, it should be part of the function responsible to lookup an IP. But with the suggestion I made earlier, to allow to specifically set IPv4/IPv6 preference as an optional 3rd parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This links to the same argument. Legacy code supporting only IPv4 would break without this. We can't expect every Arduino code out there to magically support IPv6 if they hard-coded v4 address support everywhere (like we did in early Tasmota).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(like we did in early Tasmota).

... and current ESPEasy, just to name some random project :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a solution for apps that only support IPv4, just simply don't set the flag CONFIG_LWIP_IPV6 and it will be backwards compatible.

Having the WIFI_WANT_IP6_BIT status bit seems redundant, or at least useless if IPv6 is turned off, so only relevant when you want IPv6 turned ON but deliberately the choose not to use it, which breaks the DNS abstraction.

For backwards compatibility, turn CONFIG_LWIP_IPV6 = OFF.

If you have IPv6 turned on, then you should be able to just say connect ("foo.example.com", ...) and the DNS system will abstract away what the real IP address is of foo, and including whether it is v4 or v6, or which is resolvable/connectable based on the current network?

The only rare situation seems to for diagnostics testing, where you want to test an IPv4 path on a dual stack network.

And even then hard coded addresses will still work, e.g. connect( "1.2.3.4", ..., will resolve to IPv4 1.2.3.4.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but I don't agree. Changing CONFIG_LWIP_IPV6 = OFF is overly complex and requires to recompile the entire Arduino framework. This is far beyond the casual Arduino developer. Legacy dictates that code ignoring IPv6 should continue to work, and code willing to run with IPv6 has to enable it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems reasonable. If changing CONFIG_LWIP_IPV6 is difficult, then I understand the WIFI_WANT_IP6_BIT and think it would be a good solution.

But I would just want that one flag, and if that is all that is needed, that would be great.

i.e. have a single connect() method that resolves DNS and connects to either IPv4 if the WIFI_WANT_IP6_BIT flag is off, or to the best available (based on server, network, and device support) if it is on (i.e. IPv6 if available, otherwise also IPv4).

The same with a single hostByName() method that either has IPv4 only (if flag is off) or the best available supported (6 or 4) if on.

What I don't want is additional / different code beyond that single flag, i.e. a separate hostByName6() method ... otherwise that just causes confusion (setting WIFI_WANT_IP6_BIT would not work, as I would need to go around all over my code and change hostByName()).

(The only caveat to that is maybe you want a way to test an IPv4 route when IPv6 is available and supported, so for those diagnostics type operations have a separate hostByName(type = v4) or something method makes sense.)

@TD-er
Copy link
Contributor

TD-er commented Jun 2, 2023

Not sure if that last commit was intended like this?
It is touching nearly all files in the repo and changing paths to use Tasmota.

@Jason2866
Copy link
Collaborator

@TD-er The intention was to update to latest upstream version. Failed since the branches has been changed somehow different. Reverted.

@VojtechBartoska
Copy link
Collaborator

Hello, we will start reviewing this PR, milestone changed to 3.1.0 version as we expect ipv6 support will take some time. Thanks everyone for your comments and suggestions!

@Jason2866
Copy link
Collaborator

An adopted version for Arduino 3.0.0 is here https://github.com/tasmota/arduino-esp32/tree/v5.1_ip6 it is not based on latest commits.
The actual one (with has other custom changes for Tasmota) based on latest commits is here
https://github.com/tasmota/arduino-esp32

@P-R-O-C-H-Y
Copy link
Member

P-R-O-C-H-Y commented Nov 21, 2023

@s-hadinger @Jason2866 Hi folks, can we get this PR ready to be reviewed and tested for 3.0.0?
I see that you have another branch for 3.0.0, but can you add there latest commits and open probably a new PR?
Thanks!

@Jason2866
Copy link
Collaborator

@P-R-O-C-H-Y Will provide an actual IPv6 PR. The base is still this one. @s-hadinger has adopted for Arduino 3.0.0.
Have to remove the additional changes we have for Tasmota.
It will be much easier when the C2 PR is merged before
So i don't have to undo the C2 changes. I will wait for the C2 merge, okay?

@P-R-O-C-H-Y
Copy link
Member

P-R-O-C-H-Y commented Nov 21, 2023

@P-R-O-C-H-Y Will provide an actual IPv6 PR. The base is still this one. @s-hadinger has adopted for Arduino 3.0.0. Have to remove the additional changes we have for Tasmota. It will be much easier when the C2 PR is merged before So i don't have to undo the C2 changes. I will wait for the C2 merge, okay?

Thanks @Jason2866. We will merge the C2 ASAP as it's already review by most of our team, and then you can come up with the IPv6 PR :)

@P-R-O-C-H-Y
Copy link
Member

@Jason2866 C2 support has been merged. Please ping me on the new PR when ready, thanks :)

@Jason2866
Copy link
Collaborator

This PR is replaced by PR #8907 and has to be closed when #8907 is merged.

@VojtechBartoska VojtechBartoska modified the milestones: 3.1.0, 3.0.0 Dec 12, 2023
@me-no-dev me-no-dev modified the milestones: 3.0.0, 3.0.0-RC1 Dec 14, 2023
@sgryphon
Copy link
Contributor

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

@me-no-dev
Copy link
Member

@s-hadinger closing in favor of #9016 let's discuss if something is missing there

@me-no-dev me-no-dev closed this Dec 18, 2023
@Jason2866 Jason2866 deleted the PR_IPv6_final branch January 15, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review needed Issue or PR is awaiting review
Projects
Development

Successfully merging this pull request may close these issues.