Added support for server IPAddress argument to NTPClient #32
Conversation
| @@ -30,6 +31,7 @@ class NTPClient { | |||
| NTPClient(UDP& udp); | |||
| NTPClient(UDP& udp, int timeOffset); | |||
| NTPClient(UDP& udp, const char* poolServerName); | |||
| NTPClient(UDP& udp, IPAddress poolServerIP); | |||
| NTPClient(UDP& udp, const char* poolServerName, int timeOffset); | |||
| NTPClient(UDP& udp, const char* poolServerName, int timeOffset, int updateInterval); | |||
There was a problem hiding this comment.
@sheffieldnick thanks for submitting this pull request!
The change above looks good to me. However, what do you think about adding the IPAddress option to the other constructors that accept a string?
NTPClient(UDP& udp, IPAddress poolServerIP, int timeOffset);
NTPClient(UDP& udp, IPAddress poolServerIP, int timeOffset, int updateInterval);There was a problem hiding this comment.
That would certainly work. I didn't add that code because I find it strange having multiple functions all doing much the same thing - I guess I'm just not used to the way things are structured in C++ ? A single function with optional arguments would seem more 'natural' to me. e.g.,
NTPClient(UDP& udp, IPAddress poolServerIP, int timeOffset = -1, int updateInterval = -1);
There was a problem hiding this comment.
Fair enough, I would suggest leaving the current style for now.
We can open another PR later to discuss the default constructor args with @FWeinb.
There was a problem hiding this comment.
Sounds good to me.
There was a problem hiding this comment.
@sheffieldnick ping any process on this?
|
?? I thought you were happy with my pull request? |
|
Ooops, sorry there was some confusion, still waiting for #32 (comment). |
|
Since no response by @sheffieldnick was given since 2017 (and his fork of |
Added option to connect to NTP server with binary poolServerIP argument, as alternative to string poolServerName. This takes slightly less memory, and is faster than doing a DNS lookup. If a string IP address was provided, it would still incur the overhead of being recognised as a numeric string, and converted to binary.
Fixes #31