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

EthernetCompat - static IP auto gw,mask,dns as in Arduino libraries #9024

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

JAndrassy
Copy link
Contributor

The parameters of begin in EthernetCompat.h are optional but automatic values for gw IP, mask and DNS IP were not calculated.

In the begin(mac, local_ip, arg1), arg1 is assumed to be the DNS IP, because it can be an address outside of the local network. Automatic gw IP based on local_ip with .1 at the end is more likely to be a right auto value than DNS IP. Basically this is the reason for the Arduino ordering of begin parameters.

@JAndrassy JAndrassy force-pushed the ethernet_begin_auto branch 3 times, most recently from 9f632bb to 20c47fe Compare November 10, 2023 08:57
@d-a-v
Copy link
Collaborator

d-a-v commented Nov 10, 2023

We already have a logic reordering network parameters and it is used in config() which is called just below your proposal.

Did you check if the operations are similar ?

Also, const& are there for IPAddress because IPv6 addresses are more than one single uint32.

@JAndrassy
Copy link
Contributor Author

JAndrassy commented Nov 10, 2023

this doesn't do a reordering. with no param entered it creates arduino ordering, which is then reordered in the mentioned funtion

IPAddress with IPv6 is not copyable?

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 10, 2023

IPAddress with IPv6 is not copyable?

They are copyable, but when IPv6 is enabled they are bigger than a single reference or a single IPv4.
So it takes less memory / less time to transfer them as constref. It is not a big deal, there are many places where they are passed by copy. In this PR you remove the constref so I explain why they were there: I find it to be a good reason to recall that they can be bigger.

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 10, 2023

this doesn't do a reordering. with no param entered it creates arduino ordering, which is then reordered in the mentioned funtion

OK I misread it.
The goal is to ensure that there is not nothing for IPv4 dns and gateway, by copying static address and setting .1 on the last digit (why not .254?). Out of curiosity is it specified somewhere in their doc or is it only something that is done over many of their implementations from the beginning ?

Why are you not doing this in our global config() (link above) so this arduino logic would spread over all type of interface ?

@JAndrassy
Copy link
Contributor Author

JAndrassy commented Nov 10, 2023

https://www.arduino.cc/reference/en/libraries/ethernet/ethernet.begin/
these Arduino docs are now imported to the web page from GitHub
https://github.com/arduino-libraries/Ethernet/blob/master/docs/api.md

I am not sure about this in WiFi.config. EthernetCompat is relatively new and aimed to be compatible.
In WiFi.config there is a test for local_ip, gw and dns 0 to return to DHCP. maybe testing local_ip would be enough.

I can prepare a separate PR as an alternative to this PR. There I do the auto values in ipAddressReorder.

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

a step towards API coherence & compatibility
thanks

@d-a-v d-a-v merged commit 5bd52d4 into esp8266:master Nov 10, 2023
28 checks passed
@JAndrassy JAndrassy deleted the ethernet_begin_auto branch November 10, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants