From 9541c8d1f88178bb70f382bcbc388664144b7f24 Mon Sep 17 00:00:00 2001 From: electricant Date: Wed, 7 Apr 2021 14:07:30 +0200 Subject: [PATCH] Make NTP request more robust. Previously, only the memory for the NTP header was declared and set. Actually, apart from the headers, the rest of the NTP packet (48 bytes) contains timestamps. Those timestamps are interpreted by the NTP server as offsets from their time and are used to calculate the new time. Therefore, if the rest of the packet is not zero, a wrong time is received. By reserving memory only of the header and sending 48 bytes nonetheless we send as timestams whatever values the microcontroller has on the stack. If the stack content is not zero, we receive back a garbage timestamp. This commit fixes this issue and also make the code a bit more readable by moving some magic constants in the header file. --- src/EasyNTPClient.cpp | 45 ++++++++++++++++++++++++------------------- src/EasyNTPClient.h | 12 +++++++++++- 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/src/EasyNTPClient.cpp b/src/EasyNTPClient.cpp index 22a2620..c0991d7 100644 --- a/src/EasyNTPClient.cpp +++ b/src/EasyNTPClient.cpp @@ -41,9 +41,17 @@ void EasyNTPClient::setTimeOffset (int offset) { unsigned long EasyNTPClient::getServerTime () { static int udpInited = this->mUdp->begin(123); // open socket on arbitrary port - // Only the first four bytes of an outgoing NTP packet need to be set - // appropriately, the rest can be whatever. - const long ntpFirstFourBytes = 0xEC0600E3; // NTP request header + // Only the first four bytes of an NTP request have to be set. The rest + // of the packet has to be 0, to avoid the server getting confused with the + // timestamps. + // see https://labs.apnic.net/?p=462 for details about the NTP packet + // structure + byte packetBuffer[NTP_PACKET_SIZE] = {0}; + + packetBuffer[0] = NTP_HEADER_LI|NTP_HEADER_VN|NTP_HEADER_MODE; + // packetBuffer[1] (stratum) skipped because it is already 0 + packetBuffer[2] = 6; // polling interval in log2 seconds + packetBuffer[3] = 0xEC; // precision // Fail if WiFiUdp.begin() could not init a socket if (! udpInited) @@ -54,7 +62,7 @@ unsigned long EasyNTPClient::getServerTime () { // Send an NTP request if (! (this->mUdp->beginPacket(this->mServerPool, 123) // 123 is the NTP port - && this->mUdp->write((byte *)&ntpFirstFourBytes, 48) == 48 + && this->mUdp->write(packetBuffer, NTP_PACKET_SIZE) == NTP_PACKET_SIZE && this->mUdp->endPacket())) return 0; // sending request failed @@ -63,31 +71,28 @@ unsigned long EasyNTPClient::getServerTime () { const byte maxPoll = 15; // poll up to this many times int pktLen; // received packet length for (byte i=0; imUdp->parsePacket()) == 48) - break; - delay(pollIntv); + if ((pktLen = this->mUdp->parsePacket()) == NTP_PACKET_SIZE) + break; + delay(pollIntv); } - if (pktLen != 48) - return 0; // no correct packet received + if (pktLen != NTP_PACKET_SIZE) + return 0; // no correct packet received - // Read and discard the first useless bytes - // Set useless to 32 for speed; set to 40 for accuracy. - const byte useless = 40; - for (byte i = 0; i < useless; ++i) - this->mUdp->read(); + this->mUdp->read(packetBuffer, NTP_PACKET_SIZE); - // Read the integer part of sending time - unsigned long time = this->mUdp->read(); // NTP time - for (byte i = 1; i < 4; i++) - time = time << 8 | this->mUdp->read(); + // Read the integer part (32 bits) of the Transmit Timestamp (64 bits total) + unsigned long time = packetBuffer[NTP_TX_TIMESTAMP_OFFSET]; + time = time << 8 | packetBuffer[NTP_TX_TIMESTAMP_OFFSET + 1]; + time = time << 8 | packetBuffer[NTP_TX_TIMESTAMP_OFFSET + 2]; + time = time << 8 | packetBuffer[NTP_TX_TIMESTAMP_OFFSET + 3]; // Round to the nearest second if we want accuracy - // The fractionary part is the next byte divided by 256: if it is + // The fractionary part is the last byte divided by 256: if it is // greater than 500ms we round to the next second; we also account // for an assumed network delay of 50ms, and (0.5-0.05)*256=115; // additionally, we account for how much we delayed reading the packet // since its arrival, which we assume on average to be pollIntv/2. - time += (this->mUdp->read() > 115 - pollIntv/8); + time += (packetBuffer[NTP_TX_TIMESTAMP_OFFSET + 4] > 115 - pollIntv/8); // Discard the rest of the packet this->mUdp->flush(); diff --git a/src/EasyNTPClient.h b/src/EasyNTPClient.h index 003b32f..654da94 100644 --- a/src/EasyNTPClient.h +++ b/src/EasyNTPClient.h @@ -17,6 +17,16 @@ #include "Arduino.h" #include +/* + * NTP packet-related macros. + * see https://labs.apnic.net/?p=462 for details about the NTP packet structure. + */ +#define NTP_PACKET_SIZE 48 // Size of an NTP packet +#define NTP_HEADER_LI 0b11000000 // leap indicator = 3 (unsynchronized) +#define NTP_HEADER_VN 0b00100000 // version number = 4 +#define NTP_HEADER_MODE 0b00000011 // mode = 3 (client) +#define NTP_TX_TIMESTAMP_OFFSET 40 // offset within the packet for the TX time + class EasyNTPClient { public: @@ -37,4 +47,4 @@ class EasyNTPClient unsigned long getServerTime(); }; -#endif \ No newline at end of file +#endif