Networking-for-Arduino/Arduino-Networking-API

Change QNEthernet's setDnsServerIP() to setDNSServerIP()

ssilverman opened this issue · 9 comments

JAndrassy commented

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

ssilverman commented

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 commented

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 commented

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 commented

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 commented

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 commented

I edited the "Network interface getters and setters" section

  • ❤️1
ssilverman commented

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

ssilverman commented

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