From 207b8c30db73a30c60c2054ed4e65aae4b843cac Mon Sep 17 00:00:00 2001
From: Mike <mike@sillyhouse.net>
Date: Fri, 12 Oct 2018 11:01:57 +0100
Subject: [PATCH] Various improvements to NtpClient class (#1482)

Tested with SystemClock_NTP sample, but note that time will be wrong as we don't yet have any timezone management in place. The sample also sets local time, but NTP provides time in UTC. No point in fixing that yet as without timezone it just makes things worse.

Fix #include guard name
Tidy #includes
Put NTP_DEFAULT_SERVER into Flash
Use const String& for parameters
Use SimpleTimer instead of Timer as appropriate (efficiency, saves RAM)
timeoutTimer doesn't need to be initialised until a request has been queued, so moved out of constructor
Replace staticDnsResponse() with lambda
Tidy extraction of timestamp from received packet using LWIP macro
Rewrite using one timer
Set timer to retry on initial request, and only stop it on success or cancellation
Add retry if bad NTP packet received
---
 Sming/SmingCore/Network/NtpClient.cpp | 128 +++++++++++++-------------
 Sming/SmingCore/Network/NtpClient.h   |  82 +++++++++++------
 2 files changed, 116 insertions(+), 94 deletions(-)

diff --git a/Sming/SmingCore/Network/NtpClient.cpp b/Sming/SmingCore/Network/NtpClient.cpp
index cf014fb375..6b4488197a 100644
--- a/Sming/SmingCore/Network/NtpClient.cpp
+++ b/Sming/SmingCore/Network/NtpClient.cpp
@@ -1,21 +1,30 @@
+/****
+ * Sming Framework Project - Open Source framework for high efficiency native ESP8266 development.
+ * Created 2015 by Skurydin Alexey
+ * http://github.com/anakod/Sming
+ * All files of the Sming Core are provided under the LGPL v3 license.
+ *
+ ****/
+
 #include "NtpClient.h"
+#include "Platform/Station.h"
+#include "SystemClock.h"
 
-NtpClient::NtpClient() : NtpClient(NTP_DEFAULT_SERVER, NTP_DEFAULT_QUERY_INTERVAL_SECONDS, nullptr)
+NtpClient::NtpClient() : NtpClient(NTP_DEFAULT_SERVER, NTP_DEFAULT_AUTOQUERY_SECONDS, nullptr)
 {
 }
 
 NtpClient::NtpClient(NtpTimeResultDelegate onTimeReceivedCb)
-	: NtpClient(NTP_DEFAULT_SERVER, NTP_DEFAULT_QUERY_INTERVAL_SECONDS, onTimeReceivedCb)
+	: NtpClient(NTP_DEFAULT_SERVER, NTP_DEFAULT_AUTOQUERY_SECONDS, onTimeReceivedCb)
 {
 }
 
-NtpClient::NtpClient(String reqServer, int reqIntervalSeconds, NtpTimeResultDelegate delegateFunction /* = NULL */)
+NtpClient::NtpClient(const String& reqServer, unsigned reqIntervalSeconds, NtpTimeResultDelegate delegateFunction)
 {
-	// init timer but do not start, correct interval set later below.
-	autoUpdateTimer.initializeMs(NTP_DEFAULT_QUERY_INTERVAL_SECONDS * 1000,
-								 TimerDelegate(&NtpClient::requestTime, this));
+	debug_d("NtpClient(\"%s\", %u, 0x%08x", reqServer.c_str(), reqIntervalSeconds, delegateFunction);
 
-	timeoutTimer.initializeMs(NTP_RESPONSE_TIMEOUT_MS, TimerDelegate(&NtpClient::requestTime, this));
+	// Setup timer, but don't start it
+	timer.setCallback(TimerDelegate(&NtpClient::requestTime, this));
 
 	this->server = reqServer;
 	this->delegateCompleted = delegateFunction;
@@ -23,28 +32,35 @@ NtpClient::NtpClient(String reqServer, int reqIntervalSeconds, NtpTimeResultDele
 		autoUpdateSystemClock = true;
 	}
 
-	if(reqIntervalSeconds == 0) {
-		setAutoQuery(false);
-	} else {
+	if(reqIntervalSeconds) {
 		setAutoQueryInterval(reqIntervalSeconds);
 		setAutoQuery(true);
 		requestTime();
 	}
 }
 
-NtpClient::~NtpClient()
-{
-}
-
 void NtpClient::requestTime()
 {
+	debug_d("NtpClient::requestTime()");
+
+	// Schedule a retry in anticipation of failure
+	startTimer(NTP_CONNECTION_TIMEOUT_MS);
+
 	if(!WifiStation.isConnected()) {
-		connectionTimer.initializeMs(1000, TimerDelegate(&NtpClient::requestTime, this)).startOnce();
+		debugf("NtpClient waiting for connection...");
 		return;
 	}
 
 	ip_addr_t resolvedIp;
-	int result = dns_gethostbyname(this->server.c_str(), &resolvedIp, staticDnsResponse, (void*)this);
+	int result = dns_gethostbyname(server.c_str(), &resolvedIp,
+								   [](const char* name, LWIP_IP_ADDR_T* ip, void* arg) {
+									   // We do a new request since the last one was never done.
+									   if(ip)
+										   reinterpret_cast<NtpClient*>(arg)->internalRequestTime(*ip);
+								   },
+								   this);
+
+	debug_d("dns_gethostbyname() returned %d", result);
 
 	switch(result) {
 	case ERR_OK:
@@ -58,7 +74,7 @@ void NtpClient::requestTime()
 		break;
 	case ERR_INPROGRESS:
 		// currently finding ip, internalRequestTime() will be called when its found.
-		//debug_d("DNS IP lookup in progress.");
+		debug_d("DNS IP lookup in progress...");
 		break;
 	default:
 		debug_d("DNS lookup error occurred.");
@@ -68,13 +84,13 @@ void NtpClient::requestTime()
 
 void NtpClient::internalRequestTime(IPAddress serverIp)
 {
-	// connect to current active serverIp, on NTP_PORT
-	this->connect(serverIp, NTP_PORT);
+	debug_d("NtpClient::internalRequestTime()");
 
-	uint8_t packet[NTP_PACKET_SIZE];
+	// connect to current active serverIp, on NTP_PORT
+	connect(serverIp, NTP_PORT);
 
 	// Setup the NTP request packet
-	memset(packet, 0, NTP_PACKET_SIZE);
+	char packet[NTP_PACKET_SIZE] = {0};
 
 	// These are the only required values for a SNTP request. See page 14:
 	// https://tools.ietf.org/html/rfc4330
@@ -82,47 +98,36 @@ void NtpClient::internalRequestTime(IPAddress serverIp)
 	packet[0] = (NTP_VERSION << 3 | 0x03); // LI (0 = no warning), Protocol version (4), Client mode (3)
 	packet[1] = 0;						   // Stratum, or type of clock, unspecified.
 
-	// Start timeout timer, if no response is recieved within NTP_RESPONSE_TIMEOUT
-	// a new request will be sent.
-	timeoutTimer.startOnce();
+	// Start timer to retry if no response received
+	startTimer(NTP_RESPONSE_TIMEOUT_MS);
 
 	// Send to server, serverAddress & port is set in connect
-	NtpClient::send((char*)packet, NTP_PACKET_SIZE);
-}
-
-void NtpClient::setNtpServer(String server)
-{
-	this->server = server;
+	NtpClient::send(packet, NTP_PACKET_SIZE);
 }
 
 void NtpClient::setAutoQuery(bool autoQuery)
 {
-	if(autoQuery)
-		autoUpdateTimer.start();
-	else
-		autoUpdateTimer.stop();
-}
-
-void NtpClient::setAutoQueryInterval(int seconds)
-{
-	// minimum 10 seconds interval.
-	if(seconds < 10)
-		autoUpdateTimer.setIntervalMs(10000);
-	else
-		autoUpdateTimer.setIntervalMs(seconds * 1000);
+	autoQueryEnabled = autoQuery;
+	if(autoQueryEnabled) {
+		startTimer(autoQuerySeconds * 1000U);
+	} else {
+		stopTimer();
+	}
 }
 
-void NtpClient::setAutoUpdateSystemClock(bool autoUpdateClock)
+void NtpClient::setAutoQueryInterval(unsigned seconds)
 {
-	autoUpdateSystemClock = autoUpdateClock;
+	autoQuerySeconds = max(seconds, NTP_MIN_AUTOQUERY_SECONDS);
+	if(autoQueryEnabled) {
+		setAutoQuery(true);
+	}
 }
 
 void NtpClient::onReceive(pbuf* buf, IPAddress remoteIP, uint16_t remotePort)
 {
-	// stop timeout timer since we received a response.
-	if(timeoutTimer.isStarted()) {
-		timeoutTimer.stop();
-	}
+	debug_d("NtpClient::onReceive(%s:%u)", remoteIP.toString().c_str(), remotePort);
+
+	stopTimer();
 
 	// We do some basic check to see if it really is a ntp packet we receive.
 	// NTP version should be set to same as we used to send, NTP_VERSION
@@ -136,10 +141,9 @@ void NtpClient::onReceive(pbuf* buf, IPAddress remoteIP, uint16_t remotePort)
 	if(mode == NTP_MODE_SERVER && (ver == NTP_VERSION || ver == (NTP_VERSION - 1))) {
 		//Most likely a correct NTP packet received.
 
-		uint8_t data[4];
-		pbuf_copy_partial(buf, data, 4, 40); // Copy only timestamp.
-
-		uint32_t timestamp = (data[0] << 24 | data[1] << 16 | data[2] << 8 | data[3]);
+		uint32_t timestamp;
+		pbuf_copy_partial(buf, &timestamp, sizeof(timestamp), 40); // Copy only timestamp.
+		timestamp = ntohl(timestamp);
 
 		// Unix time starts on Jan 1 1970, subtract 70 years:
 		uint32_t epoch = timestamp - 0x83AA7E80;
@@ -149,19 +153,13 @@ void NtpClient::onReceive(pbuf* buf, IPAddress remoteIP, uint16_t remotePort)
 		}
 
 		if(delegateCompleted) {
-			this->delegateCompleted(*this, epoch);
+			delegateCompleted(*this, epoch);
 		}
-	}
-}
 
-void NtpClient::staticDnsResponse(const char* name, LWIP_IP_ADDR_T* ip, void* arg)
-{
-	// DNS has been resolved
-
-	NtpClient* self = (NtpClient*)arg;
-
-	if(ip != NULL) {
-		// We do a new request since the last one was never done.
-		self->internalRequestTime(*ip);
+		// If auto query is enabled, schedule the next check
+		setAutoQuery(autoQueryEnabled);
+	} else {
+		// Got a dodgy packet so treat it as we would a response failure
+		startTimer(NTP_RESPONSE_TIMEOUT_MS);
 	}
 }
diff --git a/Sming/SmingCore/Network/NtpClient.h b/Sming/SmingCore/Network/NtpClient.h
index 351cfbee8c..da7215e244 100644
--- a/Sming/SmingCore/Network/NtpClient.h
+++ b/Sming/SmingCore/Network/NtpClient.h
@@ -1,18 +1,25 @@
+/****
+ * Sming Framework Project - Open Source framework for high efficiency native ESP8266 development.
+ * Created 2015 by Skurydin Alexey
+ * http://github.com/anakod/Sming
+ * All files of the Sming Core are provided under the LGPL v3 license.
+ *
+ ****/
+
 /** @defgroup   ntp Network Time Protocol client
  *  @brief      Provides NTP client
  *  @ingroup    datetime
  *  @ingroup    udp
  *  @{
  */
-#ifndef APP_NTPCLIENT_H_
-#define APP_NTPCLIENT_H_
+#ifndef _SMING_CORE_NETWORK_NTPCLIENT_H_
+#define _SMING_CORE_NETWORK_NTPCLIENT_H_
 
 #include "UdpConnection.h"
-#include "../Platform/System.h"
-#include "../Timer.h"
-#include "../SystemClock.h"
-#include "../Platform/Station.h"
-#include "../Delegate.h"
+#include "Platform/System.h"
+#include "Timer.h"
+#include "../Services/DateTime/DateTime.h"
+#include "Delegate.h"
 
 #define NTP_PORT 123
 #define NTP_PACKET_SIZE 48
@@ -20,9 +27,11 @@
 #define NTP_MODE_CLIENT 3
 #define NTP_MODE_SERVER 4
 
-#define NTP_DEFAULT_SERVER "pool.ntp.org"
-#define NTP_DEFAULT_QUERY_INTERVAL_SECONDS 600 // 10 minutes
-#define NTP_RESPONSE_TIMEOUT_MS 20000		   // 20 seconds
+#define NTP_DEFAULT_SERVER F("pool.ntp.org")
+#define NTP_DEFAULT_AUTOQUERY_SECONDS 30U // (10U * SECS_PER_MIN) ///< Refresh time if autoupdate set
+#define NTP_MIN_AUTOQUERY_SECONDS 10U	 ///< Minimum autoquery interval
+#define NTP_CONNECTION_TIMEOUT_MS 1666U   ///< Time to retry query when network connection unavailable
+#define NTP_RESPONSE_TIMEOUT_MS 20000U	///< Time to wait before retrying NTP query
 
 class NtpClient;
 
@@ -47,9 +56,11 @@ class NtpClient : protected UdpConnection
      *  @param  reqIntervalSeconds Quantity of seconds between NTP requests
      *  @param  onTimeReceivedCb Callback delegate to be called when NTP time result is received (Default: None)
      */
-	NtpClient(String reqServer, int reqIntervalSeconds, NtpTimeResultDelegate onTimeReceivedCb = nullptr);
+	NtpClient(const String& reqServer, unsigned reqIntervalSeconds, NtpTimeResultDelegate onTimeReceivedCb = nullptr);
 
-	virtual ~NtpClient();
+	virtual ~NtpClient()
+	{
+	}
 
 	/** @brief  Request time from NTP server
      *  @note   Instigates request. Result is handled by NTP result handler function if defined
@@ -59,7 +70,10 @@ class NtpClient : protected UdpConnection
 	/** @brief  Set the NTP server
      *  @param  server IP address or hostname of NTP server
      */
-	void setNtpServer(String server);
+	void setNtpServer(const String& server)
+	{
+		this->server = server;
+	}
 
 	/** @brief  Enable / disable periodic query
      *  @param  autoQuery True to enable periodic query of NTP server
@@ -69,12 +83,15 @@ class NtpClient : protected UdpConnection
 	/** @brief  Set query period
      *  @param  seconds Period in seconds between periodic queries
      */
-	void setAutoQueryInterval(int seconds);
+	void setAutoQueryInterval(unsigned seconds);
 
 	/** @brief  Enable / disable update of system clock
      *  @param  autoUpdateClock True to update system clock with NTP result.
      */
-	void setAutoUpdateSystemClock(bool autoUpdateClock);
+	void setAutoUpdateSystemClock(bool autoUpdateClock)
+	{
+		autoUpdateSystemClock = autoUpdateClock;
+	}
 
 protected:
 	/** @brief  Handle UDP message reception
@@ -89,24 +106,31 @@ class NtpClient : protected UdpConnection
      */
 	void internalRequestTime(IPAddress serverIp);
 
+	/** @brief Start the timer running
+	 *  @param time to run in milliseconds
+	 */
+	void startTimer(uint32_t milliseconds)
+	{
+		debug_d("NtpClient::startTimer(%u)", milliseconds);
+		timer.setIntervalMs(milliseconds);
+		timer.startOnce();
+	}
+
+	void stopTimer()
+	{
+		debug_d("NtpClient::stopTimer()");
+		timer.stop();
+	}
+
 protected:
-	String server = NTP_DEFAULT_SERVER; ///< IP address or Hostname of NTP server
+	String server; ///< IP address or Hostname of NTP server
 
 	NtpTimeResultDelegate delegateCompleted = nullptr; ///< NTP result handler delegate
 	bool autoUpdateSystemClock = false;				   ///< True to update system clock with NTP time
-
-	Timer autoUpdateTimer; ///< Periodic query timer
-	Timer timeoutTimer;	///< NTP message timeout timer
-	Timer connectionTimer; ///< Wait for WiFi connection timer
-
-	/** @brief  Handle DNS response
-     *  @param  name Pointer to c-string containing hostname
-     *  @param  ip Ponter to IP address
-     *  @param  arg Pointer to the NTP client object that made the DNS request
-     *  @note   This function is called when a DNS query is serviced
-     */
-	static void staticDnsResponse(const char* name, LWIP_IP_ADDR_T* ip, void* arg);
+	bool autoQueryEnabled = false;
+	unsigned autoQuerySeconds = NTP_DEFAULT_AUTOQUERY_SECONDS;
+	Timer timer; ///< Deals with timeouts, retries and autoquery updates
 };
 
 /** @} */
-#endif /* APP_NTPCLIENT_H_ */
+#endif /* _SMING_CORE_NETWORK_NTPCLIENT_H_ */