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

Change QNEthernet's setDnsServerIP() to setDNSServerIP() #8

Closed
ssilverman opened this issue Nov 12, 2023 · 9 comments
Closed

Change QNEthernet's setDnsServerIP() to setDNSServerIP() #8

ssilverman opened this issue Nov 12, 2023 · 9 comments

Comments

@ssilverman
Copy link
Contributor

@JAndrassy
Copy link
Member

I don't understand. Ethernet libraries have setDnsServerIP and alternative is setDNS as in WiFi libraries

@ssilverman
Copy link
Contributor Author

ssilverman commented Nov 12, 2023

Before I can answer that, let me ask you this question: Are you trying to follow Arduino's Ethernet API (here) or the WiFiNINI API (here)?

There's a case where the Ethernet API uses MACAddress(), while the WiFiNINA API uses macAddress(). Your project uses "macAddress()" and not "MACAddress()" for the common spelling. I agree with this because it keeps the naming style consistent. i.e. "start with a small-case letter". I tried to keep my API consistent with this naming scheme as well.

Look also at setMACAddress() vs. setDnsServerIP() in the Ethernet API. Those are inconsistent. In the same way that you've chosen consistency with "macAddress()" vs. "MACAddress()", I've also chosen to use "setDNSServerIP()" vs. "setDnsServerIP()".

Now, to keep my own API internally consistent, I have "setDNSServerIP()" and "dnsServerIP()" for both the "single server address" and the "nth server address" cases. Your document agrees with this naming convention because you have "setDNS()" vs. "setDns()". But, my API uses an index and your API uses a list of one or two DNS servers. Because I can have more than two DNS servers, I follow this convention:

  • void setDNSServerIP(index, IP)
  • IPAddress dnsServerIP(index)
  • From Ethernet API: void setDNSServerIP(IP)
  • From Ethernet API: IPAddress dnsServerIP()

I don't want to use the setDNS(a, b) form because it's inconsistent with the dnsIP(n) form (see your PR here). Both should use an index. This is why I have setDNSServerIP(index, IP) and dnsServerIP(index).

In summary, the correct way in QNEthernet to get the "nth" DNS server IP is dnsServerIP(n) (or no argument for index zero) and the correct way to set it is setDNSServerIP(n, IP) (or single-argument IP for index zero). Does that clarify things?

@ssilverman
Copy link
Contributor Author

I'll add a few miscellaneous notes I was just thinking about:

  1. The name "setDNS" isn't symmetrical with "dnsIP".
  2. All the other IP-related functions in the Arduino Ethernet reference are suffixed with the word "IP". (But I don't love how "setDNSIP(x)" looks.)
  3. I need to keep "setDnsServerIP", at least as an available option, because some code might use it, since it's the original "Arduino Ethernet API way".

@JAndrassy
Copy link
Member

JAndrassy commented Nov 12, 2023

all function names are from WiFi API or Ethernet API as they are. APIs evolving over a long time sometimes in separate libraries and the combined back are usually inconsistent in many ways.

In the API usage overview tables I use WiFi API names in the column title (if the WiFi API has that function) just because there are more WiFi libraries and some Ethernet libraries have the 'WiFi version' of the function name too.

It is your library so you can have any functions and function names you want, of course. I recommend the set of functions of the legacy Ethernet API. And maybe having the WiFi alternatives for users coming from WiFi libraries or if a function doesn't exist in Ethernet API like for example DNS address getter with index dnsIP(n) or the two IP DNS setter from WiFi API setDNS.

There are more documents in the repo now, all still work in progress. I still continue the research in details, I wrote test sketches to compare compilation and working of the libraries and I have discussions like this with developers in different repositories.

You could add WiFi support to your library too :-)

@ssilverman
Copy link
Contributor Author

I think my main point is that in the table, the most correct thing to put for my library is “setDNSServerIP” vs. “setDnsServerIP”, for now. (We can always adjust in the future, as needed. This is just my request.) I’ll do some thinking about supporting the others, I just don’t love how “setDNS” and “dnsIP” have asymmetrical names and also asymmetrical arguments types and behaviours.

Thanks again for caring about standards and standardization.

@JAndrassy
Copy link
Member

JAndrassy commented Nov 13, 2023

setDNS(ip1, ip2) was already in the first WiFi library by Arduino and dnsIP(n) originates from ESP82666WiFi library where it was added next to localIP() and gatewayIP()

the only way to set/add a DNS server is IP address. but you can query more than IP about a DNS server. there isn't a symmetry

@JAndrassy
Copy link
Member

JAndrassy commented Nov 13, 2023

I edited the "Network interface getters and setters" section

@ssilverman
Copy link
Contributor Author

Cool. I'm going to think about it some more.

@ssilverman
Copy link
Contributor Author

Closing for now. I’ll come back with another issue if I have more requests.

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

No branches or pull requests

2 participants