Clock display is inconsistent

Greetings;
My goal is to build a desktop clock using an ESP8266 and LCD and learn about Arduino (ESP8266) programming the process. I am using the Arduino IDE (1.6.9) and I am a complete beginner (as the code will demonstrate). My objective is to use NTP (and later an RTC) to achieve a synchronous time display with a known time standard such as audio broadcasts on WWV (to within +/-0.5 seconds).

As implemented below, the clock displays time/date and generally does what it's supposed to do (displays time, AM/PM indicator, Daylight Savings vs Standard time, etc.).

However, there are a couple of issues that I don't understand and was hoping others might educate me.

First, the display will occasionally pause for a second, and then continue (maybe one second will be displayed for 1.5 seconds or so). Also, occasionally a second will get skipped; ...3, 4, 6, 7, 8, 9, 11... The seconds that get skipped are not the same seconds every minute. It may do this "skipping" & "pausing" 3 or 6 times a minute. When it's not skipping or pausing the clock keeps time - in other words, the clock is not drifting, it's just stuttering in the display (if that makes any sense...).

Is this behavior a result of an error in my coding (likely), or is this the nature of the way the time library works?

Second, after synchronizing the clock is almost always slow by ~2 seconds when compared to a standard time reference (e.g. WWV). This delay can vary from 1 to 2 seconds. I am guessing it's due to the latency in getting the packets from the NTP servers and for the controller to parse and process the packet it receives - I guess I was just expecting the magnitude of this processing would be a very small portion of one second and thus negligible. But it's more likely that I am doing something in the code that is causing the problem.

If it's just latency, then is there a way to measure this in the routine and then add that to the time stamp - if so, how? Is there a better way to correct this error? Again, this is just an education for me.

Thank you in advance for any suggestions/wisdom.

Rob

/* 
 *  
 * ESP8266 module using Arduino IDE v1.6.9
 * 20 x4 LCD module
 * DS3231 RTC module (not yet implemented)
 * 
 * FEB-26-2017:  NTP only, no RTC.  Added the TimeZone library.  The clock is about 2 seconds slow; occassionally the digits stop and it skips numbers (seconds). I fiddled with a fudge factor in the getNtpTime routine - toward the
 * end just before the return statement, but I would like to add some method of measuring the packet latency and adding that back-in to the time reading.
 * 
 * IDEAS/FIXES:  Add BMP280 for temp, humidity & pressure; top of hour beep; add calibration routine to adjust for time it takes to get packets
 * add error handling & flag for network errors; add RTC with periodic NTP-sync
 * 
 */
#include <TimeLib.h>
#include <Timezone.h>
#include <ESP8266WiFi.h>
#include <WiFiUdp.h>
#include <LiquidCrystal_I2C.h>
#include <Wire.h>

const char ssid[] = "******";  // your network SSID (name)
const char pass[] = "*****";   // your network password

// NTP Servers:
static const char ntpServerName[] = "us.pool.ntp.org";

WiFiUDP Udp;
unsigned int localPort = 2390;  // local port to listen for UDP packets

LiquidCrystal_I2C lcd(0x3F, 2, 1, 0, 4, 5, 6, 7, 3, POSITIVE);

//US Pacific Time Zone
TimeChangeRule usPDT = {"PDT ", Second, Sun, Mar, 2, -420};  //UTC - 7 hours
TimeChangeRule usPST = {"PST ", First, Sun, Nov, 2, -480};   //UTC - 8 hours
                    //{abbrev, week, dow, mon, hr, offset in minutes}
Timezone usPacific(usPDT, usPST);
TimeChangeRule *tcr;        //pointer to the time change rule, use to get TZ abbrev
time_t pacific, utc;

void setup() 
{
  lcd.begin(20,4);
  Wire.begin();
  lcd.setCursor(6,0);
  lcd.print("NTP Clock");
  lcd.setCursor(0,1);
  lcd.print("ssid: ");
  lcd.print(ssid);
  WiFi.begin(ssid, pass);
   while (WiFi.status() != WL_CONNECTED) {
    delay(500);
    lcd.print(".");
   }
  lcd.setCursor(0,2);
  lcd.print("IP: ");
  lcd.print(WiFi.localIP());
  Udp.begin(localPort);
  lcd.setCursor(0,3);
  lcd.print("port: ");
  lcd.print(Udp.localPort());
  delay(2500);
  lcd.clear();

  setSyncProvider(getNtpTime); //defaults to sync every 5 minutes
  setSyncInterval(180); // changes sync interval in seconds

}

time_t prevDisplay = 0; // when the digital clock was displayed

void loop()
{
    {
    utc = now();
    pacific = usPacific.toLocal(utc, &tcr);
    }
  if (timeStatus() != timeNotSet) {
    if (now() != prevDisplay) { //update the display only if time has changed
      prevDisplay = now();
      digitalClockDisplay(pacific, tcr -> abbrev);  
    }
  }
}

void digitalClockDisplay(time_t t, char *tz)
{  
// print system time to LCD
  lcd.setCursor(3,1);
  lcd.print(hourFormat12(t));
  printDigits(minute(t));
  printDigits(second(t));
  if (isAM(t)==true) {lcd.print(" AM ");
   lcd.print(tz);}
  else {lcd.print(" PM ");
  lcd.print(tz);}  
  lcd.setCursor(0,3);
  lcd.print("  ");
  lcd.setCursor(2,3);
  lcd.print(dayShortStr(weekday(t)));
  lcd.print(", ");
  lcd.print(monthShortStr(month(t)));
  lcd.print(" ");
  lcd.print(day(t));
  lcd.print(", ");
  lcd.print(year(t));
}

void printDigits(int digits)    // utility for digital clock display: prints preceding colon and leading 0
{
  lcd.print(":");
  if (digits < 10)
    lcd.print('0');
  lcd.print(digits);
}

/*-------- NTP code ----------*/

const int NTP_PACKET_SIZE = 48; // NTP time is in the first 48 bytes of message
byte packetBuffer[NTP_PACKET_SIZE]; //buffer to hold incoming & outgoing packets

time_t getNtpTime()
{
  int attempts = 10;

while (attempts--) {
  
  IPAddress ntpServerIP; // NTP server's ip address

  while (Udp.parsePacket() > 0) ; // discard any previously received packets
  // get a random server from the pool
  WiFi.hostByName(ntpServerName, ntpServerIP);
  lcd.setCursor(0,3);
  lcd.print(ntpServerIP);
  sendNTPpacket(ntpServerIP);
  uint32_t beginWait = millis();
  while (millis() - beginWait < 1500) {
    int size = Udp.parsePacket();
    if (size >= NTP_PACKET_SIZE) {
      Udp.read(packetBuffer, NTP_PACKET_SIZE);  // read packet into the buffer
      unsigned long secsSince1900;
      // convert four bytes starting at location 40 to a long integer
      secsSince1900 =  (unsigned long)packetBuffer[40] << 24;
      secsSince1900 |= (unsigned long)packetBuffer[41] << 16;
      secsSince1900 |= (unsigned long)packetBuffer[42] << 8;
      secsSince1900 |= (unsigned long)packetBuffer[43];
//      return secsSince1900 - 2208988800UL + timeZone * SECS_PER_HOUR; //adjusted for time zone
      return secsSince1900 - 2208988800UL + 2;  //UTC option, with a fudge factor
    }
    delay(10);
  }
}
  return 0; // return 0 if unable to get the time
}

// send an NTP request to the time server at the given address
void sendNTPpacket(IPAddress &address)
{
  // set all bytes in the buffer to 0
  memset(packetBuffer, 0, NTP_PACKET_SIZE);
  // Initialize values needed to form NTP request
  // (see URL above for details on the packets)
  packetBuffer[0] = 0b11100011;   // LI, Version, Mode
  packetBuffer[1] = 0;     // Stratum, or type of clock
  packetBuffer[2] = 6;     // Polling Interval
  packetBuffer[3] = 0xEC;  // Peer Clock Precision
  // 8 bytes of zero for Root Delay & Root Dispersion
  packetBuffer[12]  = 49;
  packetBuffer[13]  = 0x4E;
  packetBuffer[14]  = 49;
  packetBuffer[15]  = 52;
  // all NTP fields have been given values, now
  // you can send a packet requesting a timestamp:                 
  Udp.beginPacket(address, 123); //NTP requests are to port 123
  Udp.write(packetBuffer, NTP_PACKET_SIZE);
  Udp.endPacket();
}

Could it have something to do with this?

while (millis() - beginWait < 1500) {

Regards,
Ray L.

Please always do a Tools > Auto Format on your code before posting it. This will make it easier for you to spot bugs and make it easier for us to read.

n6rob:
First, the display will occasionally pause for a second, and then continue (maybe one second will be displayed for 1.5 seconds or so).

Does that happen every 180 seconds during the sync?

n6rob:
I am guessing it's due to the latency in getting the packets from the NTP servers and for the controller to parse and process the packet it receives - I guess I was just expecting the magnitude of this processing would be a very small portion of one second and thus negligible.

I've found communicating with the NTP server can be kind of slow but it's only the time between the NTP server sending the packet and the Time library being synced that causes latency from the server time. You could get a approximation of that latency from millis() - beginWait, which you could convert to seconds and add to the return value of getNtpTime(). If you add debugging code to print that value to the Serial Monitor or LCD it could give you some indication of how long that delay is.

Thanks for the replies.

@RayL - I included that code from a reference example. I'm still learning this, but I read it as, "send the packet and for the next 1.5 seconds check to see if you have a valid response". So if it doesn't get a valid packet it delays 10 mS and tries again - until the 1.5 second time limit is up. Then, the entire process is wrapped in a "while (attempts --) " routine set at 10 attempts. I've seen this code snippet presented a couple of different ways and this seemed like the one that returned a packet most reliably (i.e. by adding multiple attempts).

@pert - thanks for the note about auto formatting; I didn't know that. Good point about debugging output. I could also ask it to report the number of attempts it is making to get the valid packet, as well as how long the round-trip is taking to/from the server.

The display stuttering happens randomly and is not (apparently) related to the 180 second sync cycle.

I'll take these suggestions and work at it for a while and report-back what I learn.

Thank you!

Rob

I have revised the code to add debugging output as pert suggested. The routine now sends the millis() - beginWait value in milliseconds to the serial output. The values are much lower than 200 mS. Here is the output with IP address and millisecond reading:

38.126.113.11
21
38.126.113.11
20
198.206.133.14
81
198.206.133.14
70
23.239.24.67
50
45.127.112.2
30
104.156.99.226
41
45.79.11.217
50
45.79.11.217
50
208.53.158.34
70
45.79.1.70
50
74.120.8.2
61
71.210.146.228
71
71.210.146.228
71
198.211.106.151
90
23.239.24.67
50
199.241.184.162
90
69.167.160.102
70
128.10.19.24
90
208.76.1.123
30
128.10.19.24
80
4.53.160.75
60
195.21.137.209
101
198.58.105.63
60

Based on this I removed the code that allows 10 attempts, and dropped the wait time for the response packet from 1500 to 200 mS. The clock is still retrieving time without problems.

However, the clock is still pausing and skipping seconds as I described previously. And the setting ranges from 0.5 to 2.0 seconds behind a reference.

More observations: I can confirm the skipping & pausing is not happening during the sync interval (now set to 60 seconds in the code below). Also, when the display skips a second, say going from :31 to :33, then it pauses on :33 more than 1 second and continues counting.

Could this be a result of the way the main loop is calling digitalClockDisplay?

/*

   ESP8266 module using Arduino IDE v1.6.9
   20 x4 LCD module
   DS3231 RTC module (not yet implemented)

   FEB-26-2017:  NTP only, no RTC.  Added the TimeZone library.  The clock is about 2 seconds slow; occassionally the digits stop and it skips numbers (seconds). I fiddled with a fudge factor in the getNtpTime routine - toward the
   end just before the return statement, but I would like to add some method of measuring the packet latency and adding that back-in to the time reading.

   IDEAS/FIXES:  Add BMP280 for temp, humidity & pressure; top of hour beep; add calibration routine to adjust for time it takes to get packets
   add error handling & flag for network errors; add RTC with periodic NTP-sync

*/
#include <TimeLib.h>
#include <Timezone.h>
#include <ESP8266WiFi.h>
#include <WiFiUdp.h>
#include <LiquidCrystal_I2C.h>
#include <Wire.h>

const char ssid[] = "";  // your network SSID (name)
const char pass[] = "";   // your network password

// NTP Servers:
static const char ntpServerName[] = "us.pool.ntp.org";

WiFiUDP Udp;
unsigned int localPort = 2390;  // local port to listen for UDP packets

LiquidCrystal_I2C lcd(0x3F, 2, 1, 0, 4, 5, 6, 7, 3, POSITIVE);

//US Pacific Time Zone
TimeChangeRule usPDT = {"PDT ", Second, Sun, Mar, 2, -420};  //UTC - 7 hours
TimeChangeRule usPST = {"PST ", First, Sun, Nov, 2, -480};   //UTC - 8 hours
//                     {abbrev, week, dow, mon, hr, offset in minutes}
Timezone usPacific(usPDT, usPST);
TimeChangeRule *tcr;        //pointer to the time change rule, use to get TZ abbrev
time_t pacific, utc;

void setup()
{
  Serial.begin(115200);
  lcd.begin(20, 4);
  Wire.begin();
  lcd.setCursor(6, 0);
  lcd.print("NTP Clock");
  lcd.setCursor(0, 1);
  lcd.print("ssid: ");
  lcd.print(ssid);
  WiFi.begin(ssid, pass);
  while (WiFi.status() != WL_CONNECTED) {
    delay(500);
    lcd.print(".");
  }
  lcd.setCursor(0, 2);
  lcd.print("IP: ");
  lcd.print(WiFi.localIP());
  Udp.begin(localPort);
  lcd.setCursor(0, 3);
  lcd.print("port: ");
  lcd.print(Udp.localPort());
  delay(2500);
  lcd.clear();

  setSyncProvider(getNtpTime); //defaults to sync every 5 minutes
  setSyncInterval(60); // changes sync interval in seconds

}

time_t prevDisplay = 0; // when the digital clock was displayed

void loop()
{
  {
    utc = now();
    pacific = usPacific.toLocal(utc, &tcr);
  }
  if (timeStatus() != timeNotSet) {
    if (now() != prevDisplay) { //update the display only if time has changed
      prevDisplay = now();
      digitalClockDisplay(pacific, tcr -> abbrev);
    }
  }
}

void digitalClockDisplay(time_t t, char *tz)
{
  // print system time to LCD
  lcd.setCursor(3, 1);
  lcd.print(hourFormat12(t));
  printDigits(minute(t));
  printDigits(second(t));
  if (isAM(t) == true) {
    lcd.print(" AM ");
    lcd.print(tz);
  }
  else {
    lcd.print(" PM ");
    lcd.print(tz);
  }
  lcd.setCursor(0, 3);
  lcd.print("  ");
  lcd.setCursor(2, 3);
  lcd.print(dayShortStr(weekday(t)));
  lcd.print(", ");
  lcd.print(monthShortStr(month(t)));
  lcd.print(" ");
  lcd.print(day(t));
  lcd.print(", ");
  lcd.print(year(t));
  lcd.print(" ");
}

void printDigits(int digits)    // utility for digital clock display: prints preceding colon and leading 0
{
  lcd.print(":");
  if (digits < 10)
    lcd.print('0');
  lcd.print(digits);
}

/*-------- NTP code ----------*/

const int NTP_PACKET_SIZE = 48; // NTP time is in the first 48 bytes of message
byte packetBuffer[NTP_PACKET_SIZE]; //buffer to hold incoming & outgoing packets

time_t getNtpTime()
{
  {

    IPAddress ntpServerIP; // NTP server's ip address

    while (Udp.parsePacket() > 0) ; // discard any previously received packets
    // get a random server from the pool
    WiFi.hostByName(ntpServerName, ntpServerIP);
    Serial.println(ntpServerIP);
    sendNTPpacket(ntpServerIP);
    uint32_t beginWait = millis();
    while (millis() - beginWait < 200) {
      int size = Udp.parsePacket();
      if (size >= NTP_PACKET_SIZE) {
        Udp.read(packetBuffer, NTP_PACKET_SIZE);  // read packet into the buffer
        unsigned long secsSince1900;
        // convert four bytes starting at location 40 to a long integer
        secsSince1900 =  (unsigned long)packetBuffer[40] << 24;
        secsSince1900 |= (unsigned long)packetBuffer[41] << 16;
        secsSince1900 |= (unsigned long)packetBuffer[42] << 8;
        secsSince1900 |= (unsigned long)packetBuffer[43];
        Serial.println(millis() - beginWait);
        //      return secsSince1900 - 2208988800UL + timeZone * SECS_PER_HOUR; //adjusted for time zone
        return secsSince1900 - 2208988800UL;  //UTC option, with a fudge factor
      }
      delay(10);
    }
  }
  return 0; // return 0 if unable to get the time
}

// send an NTP request to the time server at the given address
void sendNTPpacket(IPAddress &address)
{
  // set all bytes in the buffer to 0
  memset(packetBuffer, 0, NTP_PACKET_SIZE);
  // Initialize values needed to form NTP request
  // (see URL above for details on the packets)
  packetBuffer[0] = 0b11100011;   // LI, Version, Mode
  packetBuffer[1] = 0;     // Stratum, or type of clock
  packetBuffer[2] = 6;     // Polling Interval
  packetBuffer[3] = 0xEC;  // Peer Clock Precision
  // 8 bytes of zero for Root Delay & Root Dispersion
  packetBuffer[12]  = 49;
  packetBuffer[13]  = 0x4E;
  packetBuffer[14]  = 49;
  packetBuffer[15]  = 52;
  // all NTP fields have been given values, now
  // you can send a packet requesting a timestamp:
  Udp.beginPacket(address, 123); //NTP requests are to port 123
  Udp.write(packetBuffer, NTP_PACKET_SIZE);
  Udp.endPacket();
}

I experimented with changing the call in the main loop that updates the time display. Refer to the previous post for the complete sketch.

Here is the specific change I am referring to.

it was...

 if (now() != prevDisplay) { //update the display only if time has changed
      prevDisplay = now();
      digitalClockDisplay(pacific, tcr -> abbrev);

I changed it to...

 if (utc != prevDisplay) { //update the display only if time has changed
      prevDisplay = now();
      digitalClockDisplay(pacific, tcr -> abbrev);

The clock display no longer skips seconds or pauses. The time is now set much closer to the reference time; it varies but well within 0.5s. This result meets my objective.

However, I am still a bit vague on exactly how this works. I believe that triggering a display update on now() and asking the clock to display utc, that they became out of sync. But I am confused as to the exact flow and what factors caused them to be out of sync. Any insights are appreciated.

Thank you.

Rob

here you call now() twice:

void loop()
{
  {
    utc = now();
    pacific = usPacific.toLocal(utc, &tcr);
  }
  if (timeStatus() != timeNotSet) {
    if (now() != prevDisplay) { //update the display only if time has changed

the time may tick a second forward between the two calls to now() seemingly omitting a second, as you observed.

By freezing now in your utc time object, you are not allowing the time to tick, thus you should see better second incrementing (as you also observe). In your new code, it is a little forgiving of the exact moment of the tick.

most folks would would do it this way.

The peccadillo here is the time it takes to poll the NTP servers. Unfortunately with single threading, this is a limitation of the hardware.

Your Atomic Clock approach or even a GPS clock may prove more amicable to your desire for accuracy. Correcting for a lag as those signals would be considerably more reliable (when you actually have them consistently).

Thank you BullDog.

Yes, as I learn more I plan to add RTC and GPS. Because the micro is an ESP8266, NTP seemed like a good place to start. For now this is a lesson on how the details work.

Early-on I had attempted to integrate more of these things all-at-once and realized I had extended myself beyond my capabilities - so I decided to understand this approach first (i.e. start with Time library, then add NTP update, ...) - and expand from there.

I realize now that a call to "now()" at two different times will give two different answers (face-palm...) - thanks for that explanation.

Contrary to my previous post, I am still getting about 1 to 2 seconds behind a time reference. The latency (millis() - beginWait), or the time between sending the packet and returning a value from the getNtpTime routine, ranges from 30 to 90 milliseconds; occasionally it runs up to 150 or so (if I am measuring it correctly...).

I figure two clock displays within 0.1 to 0.2 seconds of each other will challenge human perception, which is to say, one may have a difficult time seeing the difference.

I can fudge the getNtpTime return value by adding 1 second to move the clock closer to a reference; but it still doesn't explain why a ~100mS routine is losing 1 or 2 seconds at the display. I suspect I am mishandling the time variable, or I am wasting resources in a way that is causing the delay.

Still have a lot to learn.

Rob