This is a continuation on the topic of adding IPv6 Support to ESP32 Arduino #9016
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,114 +1,116 @@ | ||
| /* | ||
| IPAddress.h - Base class that provides IPAddress | ||
| Copyright (c) 2011 Adrian McEwen. All right reserved. | ||
|
|
||
| This library is free software; you can redistribute it and/or | ||
| modify it under the terms of the GNU Lesser General Public | ||
| License as published by the Free Software Foundation; either | ||
| version 2.1 of the License, or (at your option) any later version. | ||
|
|
||
| This library is distributed in the hope that it will be useful, | ||
| but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
| Lesser General Public License for more details. | ||
|
|
||
| You should have received a copy of the GNU Lesser General Public | ||
| License along with this library; if not, write to the Free Software | ||
| Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <stdint.h> | ||
| #include "Printable.h" | ||
| #include "WString.h" | ||
| #include "lwip/ip_addr.h" | ||
|
|
||
| #define IPADDRESS_V4_BYTES_INDEX 12 | ||
| #define IPADDRESS_V4_DWORD_INDEX 3 | ||
|
|
||
| // A class to make it easier to handle and pass around IP addresses | ||
|
|
||
| enum IPType { | ||
|
There was a problem hiding this comment. Could you define By default There was a problem hiding this comment. 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. |
||
| IPv4, | ||
| IPv6 | ||
| }; | ||
|
|
||
| class IPAddress : public Printable { | ||
| private: | ||
| union { | ||
| uint8_t bytes[16]; | ||
|
There was a problem hiding this comment. 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. 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. This whole class (both header and implementation) I copied directly from Arduino's Core API. Anything that is not There was a problem hiding this comment. There is an effort to change the approach in Arduino, if that happens so, we will update the class here too. |
||
| uint32_t dword[4]; | ||
| } _address; | ||
| IPType _type; | ||
| uint8_t _zone; | ||
me-no-dev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Access the raw byte array containing the address. Because this returns a pointer | ||
| // to the internal structure rather than a copy of the address this function should only | ||
| // be used when you know that the usage of the returned uint8_t* will be transient and not | ||
| // stored. | ||
| uint8_t* raw_address() { return _type == IPv4 ? &_address.bytes[IPADDRESS_V4_BYTES_INDEX] : _address.bytes; } | ||
|
|
||
| public: | ||
| // Constructors | ||
|
|
||
| // Default IPv4 | ||
| IPAddress(); | ||
| IPAddress(IPType ip_type); | ||
| IPAddress(uint8_t first_octet, uint8_t second_octet, uint8_t third_octet, uint8_t fourth_octet); | ||
| IPAddress(uint8_t o1, uint8_t o2, uint8_t o3, uint8_t o4, uint8_t o5, uint8_t o6, uint8_t o7, uint8_t o8, uint8_t o9, uint8_t o10, uint8_t o11, uint8_t o12, uint8_t o13, uint8_t o14, uint8_t o15, uint8_t o16, uint8_t z=0); | ||
| // IPv4; see implementation note | ||
| IPAddress(uint32_t address); | ||
| // Default IPv4 | ||
| IPAddress(const uint8_t *address); | ||
| IPAddress(IPType ip_type, const uint8_t *address, uint8_t z=0); | ||
| // If IPv4 fails tries IPv6 see fromString function | ||
| IPAddress(const char *address); | ||
| IPAddress(const IPAddress& address); | ||
|
|
||
| bool fromString(const char *address); | ||
| bool fromString(const String &address) { return fromString(address.c_str()); } | ||
|
|
||
| // Overloaded cast operator to allow IPAddress objects to be used where a uint32_t is expected | ||
| // NOTE: IPv4 only; see implementation note | ||
| operator uint32_t() const { return _type == IPv4 ? _address.dword[IPADDRESS_V4_DWORD_INDEX] : 0; }; | ||
|
|
||
| bool operator==(const IPAddress& addr) const; | ||
| bool operator!=(const IPAddress& addr) const { return !(*this == addr); }; | ||
|
|
||
| // NOTE: IPv4 only; we don't know the length of the pointer | ||
| bool operator==(const uint8_t* addr) const; | ||
|
|
||
| // Overloaded index operator to allow getting and setting individual octets of the address | ||
| uint8_t operator[](int index) const; | ||
| uint8_t& operator[](int index); | ||
|
|
||
| // Overloaded copy operators to allow initialisation of IPAddress objects from other types | ||
| // NOTE: IPv4 only | ||
| IPAddress& operator=(const uint8_t *address); | ||
| // NOTE: IPv4 only; see implementation note | ||
| IPAddress& operator=(uint32_t address); | ||
| // If IPv4 fails tries IPv6 see fromString function | ||
| IPAddress& operator=(const char *address); | ||
| IPAddress& operator=(const IPAddress& address); | ||
|
|
||
| virtual size_t printTo(Print& p) const; | ||
| String toString() const; | ||
|
|
||
| IPType type() const { return _type; } | ||
|
|
||
| uint8_t zone() const { return (type() == IPv6)?_zone:0; } | ||
|
|
||
| // LwIP conversions | ||
| void to_ip_addr_t(ip_addr_t* addr); | ||
| IPAddress& from_ip_addr_t(ip_addr_t* addr); | ||
|
There was a problem hiding this comment. Is it possible to pass argument as There was a problem hiding this comment. Sorry for another request, could it be possible to have a constructor that takes directly
Actually the only use-case I see is for initializing a IPAddress from I can live with that but a constructor looks cleaner to me. There was a problem hiding this comment. Maybe also add |
||
|
|
||
| friend class UDP; | ||
| friend class Client; | ||
| friend class Server; | ||
|
|
||
| protected: | ||
| bool fromString4(const char *address); | ||
| bool fromString6(const char *address); | ||
| }; | ||
|
|
||
| extern const IPAddress IN6ADDR_ANY; | ||
| extern const IPAddress INADDR_NONE; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1055,8 +1055,8 @@ esp_err_t WiFiGenericClass::_eventCallback(arduino_event_t *event) | |
| } else if(event->event_id == ARDUINO_EVENT_WIFI_STA_CONNECTED) { | ||
| WiFiSTAClass::_setStatus(WL_IDLE_STATUS); | ||
| setStatusBits(STA_CONNECTED_BIT); | ||
| if (getStatusBits() & WIFI_WANT_IP6_BIT) | ||
| esp_netif_create_ip6_linklocal(esp_netifs[ESP_IF_WIFI_STA]); | ||
| } else if(event->event_id == ARDUINO_EVENT_WIFI_STA_DISCONNECTED) { | ||
| uint8_t reason = event->event_info.wifi_sta_disconnected.reason; | ||
| // Reason 0 causes crash, use reason 1 (UNSPECIFIED) instead | ||
|
|
@@ -1546,6 +1546,13 @@ bool WiFiGenericClass::setDualAntennaConfig(uint8_t gpio_ant1, uint8_t gpio_ant2 | |
| // ------------------------------------------------ Generic Network function --------------------------------------------- | ||
| // ----------------------------------------------------------------------------------------------------------------------- | ||
|
|
||
| typedef struct gethostbynameParameters { | ||
| const char *hostname; | ||
| ip_addr_t addr; | ||
| uint8_t addr_type; | ||
| int result; | ||
| } gethostbynameParameters_t; | ||
|
|
||
| /** | ||
| * DNS callback | ||
| * @param name | ||
|
|
@@ -1554,58 +1561,68 @@ bool WiFiGenericClass::setDualAntennaConfig(uint8_t gpio_ant1, uint8_t gpio_ant2 | |
| */ | ||
| static void wifi_dns_found_callback(const char *name, const ip_addr_t *ipaddr, void *callback_arg) | ||
| { | ||
| gethostbynameParameters_t *parameters = static_cast<gethostbynameParameters_t *>(callback_arg); | ||
| if(ipaddr) { | ||
| if(parameters->result == 0){ | ||
| memcpy(&(parameters->addr), ipaddr, sizeof(ip_addr_t)); | ||
| parameters->result = 1; | ||
| } | ||
| } else { | ||
| parameters->result = -1; | ||
| } | ||
| xEventGroupSetBits(_arduino_event_group, WIFI_DNS_DONE_BIT); | ||
| } | ||
|
|
||
| /** | ||
| * Callback to execute dns_gethostbyname in lwIP's TCP/IP context | ||
| * @param param Parameters for dns_gethostbyname call | ||
| */ | ||
| static esp_err_t wifi_gethostbyname_tcpip_ctx(void *param) | ||
| { | ||
| gethostbynameParameters_t *parameters = static_cast<gethostbynameParameters_t *>(param); | ||
| return dns_gethostbyname_addrtype(parameters->hostname, ¶meters->addr, &wifi_dns_found_callback, parameters, parameters->addr_type); | ||
| } | ||
|
|
||
| /** | ||
| * 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, bool preferV6) | ||
|
There was a problem hiding this comment. I worry about changing the signature here. It means that every app that uses this code needs to change. Even if they don't want or aren't ready for IPv6. Rather than add this parameter, can we just do a check inside this function: params.addre_type = (WiFiGenericClass::getStatusBits() & WIFI_WANT_IP6_BIT) ? LWIP_DNS_ADDRTYPE_IPV6_IPV4 : LWIP_DNS_ADDRTYPE_IPV4; There was a problem hiding this comment. you again looking at the implementation... in the header a default value is given There was a problem hiding this comment. Or you could make it an optional parameter with some define as default. The if-no-defined-Arduino-default could then be Or add a 2nd function signature which then uses a static defined bool (which can be set by the user at runtime) to call the function with this bool argument. There was a problem hiding this comment. @TD-er read my comment. nothing needs to be done. default values are defined in headers, not in source files There was a problem hiding this comment. Yep, read your post after a reload of the page. (there is another post here where you even 'beat' me to it by about 45 mins which did not show up when I typed my reply) |
||
| { | ||
| err_t err = ERR_OK; | ||
| gethostbynameParameters_t params; | ||
|
|
||
| aResult = static_cast<uint32_t>(0); | ||
| params.hostname = aHostname; | ||
| params.addr_type = preferV6?LWIP_DNS_ADDRTYPE_IPV6_IPV4:LWIP_DNS_ADDRTYPE_IPV4_IPV6; | ||
|
There was a problem hiding this comment. Just make this: params.addr_type = (WiFiGenericClass::getStatusBits() & WIFI_WANT_IP6_BIT) ? LWIP_DNS_ADDRTYPE_IPV6_IPV4 : LWIP_DNS_ADDRTYPE_IPV4; If WIFI_WANT_IP6_BIT is turn off, then we only want IPV4. Do not return IPv6 at all, because it might break backwards clients. If a client turns off the IPv6 bit they should only get IPv4 results (like before), i.e. they are IPv4 only. Note the IPV4 only. If IPv6 is turned on, the return IPv6 if available (thats why they turned it on!), but with fall back to IPv4. There was a problem hiding this comment. This function only returns one address, so if you are doing something where you want both IPv6 global and link-local (although the later don't usually come from DNS) addresses, it isn't very useful. You really also need to know the context. e.g. if an address has global IPv6, global IPv4, and link-local IPv6, you probably want the global IPv6 (although it is has a relevant link-local maybe use that). But maybe if you have only link-local IPv6 and a global IPv4, you actually want to return the IPv4. There was a problem hiding this comment. Actually, (WiFiGenericClass::getStatusBits() & WIFI_WANT_IP6_BIT) is probably not sufficient. You probably want (WiFiGenericClass::getStatusBits() & WIFI_WANT_IP6_BIT) && haveGlobalIPv6Address. Just because you want IPv6 doesn't mean you got it. Maybe you are on an IPv4 only network. You might have auto-assigned yourself an IPv6 link-local address, but if there are no other IPv6 nodes that may be useless, as will a DNS result for google.com that returns you the global IPv6, because you have no way to reach it. Maybe the network stack takes care of this? (i.e. will it only return addresses you can reach, or will it return all addresses). RFC 6724 talks about address selection, so may be relevant, where the destination addresses need to be filtered by which make sense based on the available source addresses. There was a problem hiding this comment. yes. it can not be as simple as now, because there are many cases which will cause trouble. I do vote to not bother and change it here much, because after this is merged, I will continue on rewriting the network stack of Arduino and the new things will allow to ask if we have a proper IPv6 address present. What we need to do is define the proper algo to decide what kind of query to issue and how to make sure that results are what they should be. There was a problem hiding this comment. Actually you probably don't even need to check (WiFiGenericClass::getStatusBits() & WIFI_WANT_IP6_BIT) at all ... all you need to check is (the nonexistent) haveGlobalIPv6Address. You will only get an address if WANT is on, so re-checking WANT is not necessary. There are a few different scenarios, if WANT is off, then just do the IPv4 behaviour. If WANT is on, then sometimes you will be assigned an global/routable address, and then can use IPv6 lookups. Even if you don't get an address, you can still assign your own link-local, and communicate with other link-local. DNS probably isn't going to return link-local addresses, but mDNS might. Happy to go with something initial, and then refine. I did notice the LWIP has a ip6_select_source_address function based on RFC 6724 (given a particular destination, work out which source address to use from a given interface). There was a problem hiding this comment. Going with the current implementation, you should still change to: Using Because if IPv6 is off for backwards compatiblity we only want to return IPv4; otherwise we might break apps (e.g. if no IPv4 we should return nothing instead of IPv6). |
||
| params.result = 0; | ||
| aResult.to_ip_addr_t(&(params.addr)); | ||
|
|
||
| if (!aResult.fromString(aHostname)) { | ||
| waitStatusBits(WIFI_DNS_IDLE_BIT, 16000); | ||
| clearStatusBits(WIFI_DNS_IDLE_BIT | WIFI_DNS_DONE_BIT); | ||
|
|
||
| err = esp_netif_tcpip_exec(wifi_gethostbyname_tcpip_ctx, ¶ms); | ||
| if (err == ERR_OK) { | ||
| aResult.from_ip_addr_t(&(params.addr)); | ||
| } else if (err == ERR_INPROGRESS) { | ||
| waitStatusBits(WIFI_DNS_DONE_BIT, 15000); //real internal timeout in lwip library is 14[s] | ||
| clearStatusBits(WIFI_DNS_DONE_BIT); | ||
| if (params.result == 1) { | ||
| aResult.from_ip_addr_t(&(params.addr)); | ||
| err = ERR_OK; | ||
| } | ||
| } | ||
| setStatusBits(WIFI_DNS_IDLE_BIT); | ||
| } | ||
| if (err == ERR_OK) { | ||
| return 1; | ||
| } | ||
| log_e("DNS Failed for '%s' with error '%d' and result '%d'", aHostname, err, params.result); | ||
| return err; | ||
| } | ||
|
|
||
| IPAddress WiFiGenericClass::calculateNetworkID(IPAddress ip, IPAddress subnet) { | ||
|
|
||
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
#ifdefor#ifcheck.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.