-
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
Add v6 support to IPAddress to match ArduinoCore-API #7174
Add v6 support to IPAddress to match ArduinoCore-API #7174
Conversation
I have not been able to run the tests locally, but did get them running (and now all successful / green) in my GitHub repo. Link to the successful run is here: https://github.com/sgryphon/arduino-esp32/actions/runs/2931290749 This PR says it needs a contributor to approve first-time running of the workflows. |
Concern: Regarding the != operator.
While adding the != operator is backwards compatible (it adds more), changing the existing conversion to be explicit would be a (fairly small) change in existing functionality. Personally I think the missing != is a bug, that needs to be fixed, even if it breaks existing implicit conversion. |
I see ArduinoCore does not have an IPv6 library. Opinion: If we are adding v6 support to IPAddress, we should delete IPv6Address library. |
That would be a breaking change. It is okay to do this for a major version, but usually you would want to have the replacement already added (i.e. add this backwards compatible change first), then foreshadow & let people know (mark the old code deprecated, if the language supports this, and tell them to use the new). Then a future version would remove the code. e.g. a plan could be to add the IPv6 support first; if that is going to be release v3.0.0, then add a comment that IPv6Address will be removed in v4.0.0 and to use IPAddress instead (and then remove it in v4). |
@sgryphon are you around to rebase this? And if possible, update it to the latest API from Arduino? We would like to merge it now :) |
Thanks for the ping. I will have a look, but shouldn't take too long. |
Just copying the latest IPAddress from ArduinoCore-API should be sufficient :) there are a couple changes since you submitted this PR. |
dec0c4b
to
b4e644f
Compare
👋 Hello sgryphon, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
I have rebased, and added the recent (minor) changes from ArduinoCore... but I see you now have a commit message linter, so will also need to clean them up (I may get time over the weekend, let me see). |
Don't worry, we are still experimenting with it :) Thanks for updating the files! |
b4e644f
to
fecde01
Compare
I've updated the commit messages to match the linter format. |
@sgryphon could you please sign the CLA (second message in the thread) |
CLA is signed. |
If anyone wants to have a discussion on wider IPv6 (not just IPAddress, which should be aligned with ArduinoCore), I created a general IPv6 discussion that can be used: #9009 |
I'm very annoyed by this PR that doesn't take into account what we have proposed in Tasmota. The main idea was to wrap ip I'm sorry but we just can't use this implementation at all. For reference here are the definitions from LWIP: /** This is the aligned version of ip6_addr_t,
used as local variable, on the stack, etc. */
struct ip6_addr {
u32_t addr[4];
#if LWIP_IPV6_SCOPES
u8_t zone;
#endif /* LWIP_IPV6_SCOPES */
};
/** IPv6 address */
typedef struct ip6_addr ip6_addr_t;
[...]
/** This is the aligned version of ip4_addr_t,
used as local variable, on the stack, etc. */
struct ip4_addr {
u32_t addr;
};
/** ip4_addr_t uses a struct for convenience only, so that the same defines can
* operate both on ip4_addr_t as well as on ip4_addr_p_t. */
typedef struct ip4_addr ip4_addr_t;
[...]
/**
* @ingroup ipaddr
* A union struct for both IP version's addresses.
* ATTENTION: watch out for its size when adding IPv6 address scope!
*/
typedef struct ip_addr {
union {
ip6_addr_t ip6;
ip4_addr_t ip4;
} u_addr;
/** @ref lwip_ip_addr_type */
u8_t type;
} ip_addr_t; |
Let me explain more for those who are not familiar with link-local IPv6. Link-local addressed are used extensively by Matter protocol. If you have multiple network interfaces, there is no way to for sure which network interface is used by which link-local addresses. Hence an additional information In text representation, they are separate with |
@s-hadinger check and comment #9016 please |
* revert broken IPv6 from upstream espressif#7174 * IPv6 support * remove WPA2 Enterprise * rm `WiFiClientSecure` * "--dont-append-digest" * rm not needed librarys * safeboot in Tasmota project * Revert "WiFiSTA - allow using DHCP again after disconnecting static IP (espressif#8848)" (#297) * remove zigbee libs * joltwallet/littlefs 1.11.0
Small steps. That PR was mostly written 1 year ago, to align with ArduinoCore (which does not have zone ID). ArduinoCore in turn was actually focussed for mobile (4G) devices, with a single interface assigned a global address, because carriers are moving to IPv6 only networks... so quite a different case that Matter / Thread. The new PR (linked) integrates in the zone ID ... and I suggesting contributing it back to ArduinoCore, for consistency between platforms. I also added a section to the discussion I created #9009 Because yes, people misunderstand things like link-local, e.g. confusing it with local, as opposed to remote (the ends of a connection) and link-local vs global (different scopes). |
By completing this PR sufficiently, you help us to review this Pull Request quicker and also help improve the quality of Release Notes
Checklist
This entire section above can be deleted if all items are checked.
Description of Change
Please describe your proposed Pull Request and it's impact.
Update the IPAddress class to support v6 addresses, using the same interface changes as ArduinoCore-API (and thus compatible with other Arduino based platforms).
Note that the actual API changes are minimal:
The changes are backward compatible, i.e. any IPv4 code will continue to get the same outputs for a given inputs. (There are extensive unit test cases in ArduinoCore-API).
Note: Whilst backwards compatible, this change does increase the memory footprint from 4 bytes to 20 bytes (assuming the enum compiles as an underlying int32_t).
Tests scenarios
Please describe on what Hardware and Software combinations you have tested this Pull Request and how.
(eg. I have tested my Pull Request on Arduino-esp32 core v2.0.2 with ESP32 and ESP32-S2 Board with this scenario)
NOTE: I am not sure how to build and test the project as I can't find instructions in the readme or contributing files. I can see the github actions scripts, but they are a bit more complicated that the ArduinoCore-API project, so I haven't reverse engineered out the steps to run all the tests yet (or the dependencies what is needed).
I am putting this code up for review while I figure this out (and maybe GitHub can run the tests).
Note that the code is mostly cut and paste from ArduinoCore-API project, which has unit tests, so should work. (The only bit not directly copied was the toString() function changes).
Related links
Please provide links to related issue, PRs etc.
(eg. Closes #number of issue)
Related to #7114, although it doesn't make it directly compatible with ESP8266 but with ArduinoCore, but does close off the issue.
Essentially fixes #6600 as it uses the RFC 5952 canonical format, which includes shortening of the left-most largest group of two or more zero fields. (The simple example in #6600 simply did the first zero fields). Note that the fromString() function parses all IPv6 representations, not just the canonical one. (There are extensive unit tests in the ArduinoCore-API project)
It is related to #6242 as it provides core support for the IPAddress class to have both v4 and v6 addresses, enabling support to be added (internally) to other classes (such as WiFi, Ethernet, etc) to handle IPv6 (without having to complicate their interfaces).