Problem with setSyncProvider() [from Time Library]

I'm getting mad on this...
My application grew and grew and even though it is my first try, everything worked fine until I tried to implement a clock. I stuck already a week on this.
Meanwhile I located the error, but I have no idea, what the root cause could be.

In my program I want to set the clock by NTP, but when calling the function setSyncProvider(getNtpTime); normally nothing will happen except that there will be a timeout after a certain time (not the while... loop, the printout "Time Status after setSyncProvider" does not appear). The time status will not change and the clock is not set. A very few times it works fine, but I did not find out in which cases. I sometimes thought I have some evidence, but a few trials later it was gone.

Below the code I printed the serial output. In this example two times the setting of the clock did not work, the third try runs and the clock output begins.
It is obvious, that the function getNtpTime(); always returns the correct time (as you also can see in the printout). But the setSyncProvider() function only gets the pointer to the function getNtpTime(); I think, and I don't know how the clock is being set when using this construction, so I don't know how to locate the real problem.
Has anybody an idea?

// NTP time issue
// IDE 1.0.3
// DHCP Server required
// Arduino Mega2560 + Ethernetshield for Mega with SD-slot

#include <MsTimer2.h>
#include <Time.h>
#include <SPI.h>
#include <Ethernet.h>
#include <EthernetUdp.h>
#include <Dns.h>


#define SD_CS 4

// Ethernet library configuration
byte mac[] = { 0x24, 0x7B, 0xA2, 0x4A, 0x52, 0x10 };
IPAddress timeServer;

/* ALTER THESE VARIABLES AT YOUR OWN RISK */
// local port to listen for UDP packets
unsigned int localPort = 8888;
// NTP time stamp is in the first 48 bytes of the message
const int NTP_PACKET_SIZE= 48;      
// Buffer to hold incoming and outgoing packets
byte packetBuffer[NTP_PACKET_SIZE];  
// A UDP instance to let us send and receive packets over UDP
EthernetUDP Udp;                    

boolean s_flag = false;



void setup() {                

  // disable SD SPI Chip Select 
  pinMode(SD_CS,OUTPUT);
  digitalWrite(SD_CS,HIGH);

  Serial.begin(19200);
  

// Setup Ethernet by DHCP

  Serial.println("Wait for DHCP Server");
  if (Ethernet.begin(mac) == 0) {  // start Ethernet by DHCP conf.
    Serial.println("DHCP config failed! ");
  }

  DNSClient dns;
  dns.begin(Ethernet.dnsServerIP());
  dns.getHostByName("pool.ntp.org",timeServer); // at this point the function works
  Serial.print("NTP Server: ");
  Serial.println(timeServer);

  setSyncInterval(5); // Set seconds between re-sync (5s for test only)
 
  Udp.begin(localPort);
  Serial.print("Time: ");
  Serial.println(getNtpTime());
  Serial.print("Time Status before setSyncProvider:");
  Serial.println(timeStatus());
  setSyncProvider(getNtpTime);
  Serial.print("Time Status after setSyncProvider:");
  Serial.println(timeStatus());

  while(timeStatus()== timeNotSet)
     ; // wait until the time is set by the sync provider


  MsTimer2::set(1000, update1s); // 1s period
  MsTimer2::start();


} 
  
  
void loop() {

  if (s_flag){
    s_flag=false;  
    digitalClockDisplay();
  }
}  



void update1s(){
// 1s is over flag
  s_flag=true;
}


void digitalClockDisplay(){
  // digital clock display of the time
  Serial.print(day());
  Serial.print(".");
  Serial.print(month());
  Serial.print(".");
  Serial.print(year());
  Serial.print(" ");
  Serial.print(hour());
  printDigits(minute());
  printDigits(second());
  Serial.println(" ");
}

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

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

unsigned long getNtpTime()
{
  sendNTPpacket(timeServer); // send an NTP packet to a time server
    delay(500);
    
    if ( Udp.parsePacket() ) { 
     Udp.read(packetBuffer,NTP_PACKET_SIZE);  // read packet into buffer


     //the timestamp starts at byte 40, convert four bytes into a long integer
     unsigned long hi = word(packetBuffer[40], packetBuffer[41]);
     unsigned long low = word(packetBuffer[42], packetBuffer[43]);
     // this is NTP time (seconds since Jan 1 1900
     unsigned long secsSince1900 = hi << 16 | low;  
     // Unix time starts on Jan 1 1970
     const unsigned long seventyYears = 2208988800UL;     
     unsigned long epoch = secsSince1900 - seventyYears + adjustDstEurope();  // subtract 70 years, add Daylight Saving and Timezone adjust
     return epoch;
  }
  return 0; // return 0 if unable to get the time
}

// send an NTP request to the time server at the given address
unsigned long sendNTPpacket(IPAddress& address)
//sendNTPpacket(IPAddress address)
{


  memset(packetBuffer, 0, NTP_PACKET_SIZE);  // set all bytes in the buffer to 0

  // Initialize values needed to form NTP request
  packetBuffer[0] = B11100011;   // LI, Version, Mode
  packetBuffer[1] = 0;     // Stratum
  packetBuffer[2] = 6;     // Max Interval between messages in seconds
  packetBuffer[3] = 0xEC;  // Clock Precision
  // bytes 4 - 11 are for Root Delay and Dispersion and were set to 0 by memset
  packetBuffer[12]  = 49;  // four-byte reference ID identifying
  packetBuffer[13]  = 0x4E;
  packetBuffer[14]  = 49;
  packetBuffer[15]  = 52;

  // send the packet requesting a timestamp:
  Udp.beginPacket(address, 123); //NTP requests are to port 123

  Udp.write(packetBuffer,NTP_PACKET_SIZE);

  Udp.endPacket();

}


int adjustDstEurope()
{
  // last sunday of march
  int beginDSTDate=  (31 - (5* year() /4 + 4) % 7);
  //Serial.println(beginDSTDate);
  int beginDSTMonth=3;
  //last sunday of october
  int endDSTDate= (31 - (5 * year() /4 + 1) % 7);
  //Serial.println(endDSTDate);
  int endDSTMonth=10;
  // DST is valid as:
  if (((month() > beginDSTMonth) && (month() < endDSTMonth))
      || ((month() == beginDSTMonth) && (day() >= beginDSTDate))
      || ((month() == endDSTMonth) && (day() <= endDSTDate)))
  return 7200;  // DST europe = utc +2 hour
  else return 3600; // nonDST europe = utc +1 hour
}
Wait for DHCP Server
NTP Server: 91.143.83.62
Time: 1360396530
Time Status before setSyncProvider:0

---restarted due to USB interrupt

Wait for DHCP Server
NTP Server: 46.4.54.78
Time: 1360396556
Time Status before setSyncProvider:0

---restarted due to USB interrupt

Wait for DHCP Server
NTP Server: 178.63.9.212
Time: 1360396573
Time Status before setSyncProvider:0
Time Status after setSyncProvider:2
9.2.2013 7:56:14 
9.2.2013 7:56:15 
9.2.2013 7:56:16 
9.2.2013 7:56:17 
...

It looks like update1s is an ISR so try making s_flag volatile.

Nick,

did you think of this?

volatile boolean s_flag = false;

it did not work. (I'm not a specialist in C...)

Meanwhile I replaced the setting of the clock by

setTime((time_t)getNtpTime());

It works for the moment, but it is not what wanted initially, as the getSyncProvider cares also for re-syncing.
Ans I don't know if the cast trick has negative side effects.

In my program I want to set the clock by NTP, but when calling the function setSyncProvider(getNtpTime);

setSyncProvider does not cause a synchronization. It simply defines what function to call when the synchronization needs to happen.

So, your fundamental assumption is flawed.

There is another method to define when the synchronization is to occur. It might be possible to modify the library to have some specific interval (perhaps 0) have special meaning, like sync now but only once. It might even already have that capability.

purehunter:
...
Below the code I printed the serial output. In this example two times the setting of the clock did not work, the third try runs and the clock output begins.

unsigned long getNtpTime()

{
  sendNTPpacket(timeServer); // send an NTP packet to a time server
    delay(500);
   
    if ( Udp.parsePacket() ) {
    ... guts ...
     return epoch;
    }
  return 0; // return 0 if unable to get the time
}

In order to succeed, getNtpTime must get an answer back with 0.5 sec-- I wonder if this is enough to be reliable? The examples that came with the Time library I am using uses delay(1000) here, and is different in other ways. Which Time library are you using? I believe I am using Arduino Playground - Time.

My time library does appear to attempt a sync at the time setSyncProvider is called, so as far as I can tell your code should work.

John

My time library does appear to attempt a sync at the time setSyncProvider is called, so as far as I can tell your code should work.

But not JUST when setSyncProvider is called.

PaulS:

My time library does appear to attempt a sync at the time setSyncProvider is called, so as far as I can tell your code should work.

But not JUST when setSyncProvider is called.

Yes, expectation is probably that it be called every 5 sec per

setSyncInterval(5); // Set seconds between re-sync (5s for test only)

So what is the "flawed fundamental assumption" that you think he is making? Maybe I am making it too.

@johncc
the 500ms should be no problem. As I wrote: I always get the time, if I call getNtpTime(); with the method "setTime((time_t)getNtpTime());" the clock will always be set.
I'm using the same time lib, the files already came with IDE 1.0.1 and are from Jul and Sept 2011.

@PaulS
If just calling setSyncProvider(getNtpTime); is fundamentally wrong, I'm asking why is this method presented in every example?
I began with copy and paste.
And again: getNtpTime(); works well!! Just setSyncProvider(getNtpTime); does not return. And this must work independently from the chosen interval.

What I'm wondering now is that the function getNtpTime() is returning an "unsigned long" while setTime() -and maybe setSyncProvider internally also- is requesting "time_t". I know, there is only a pointer argument in the call setSyncProvider(getNtpTime); , but somehow the time has to be transferred.

So what is the "flawed fundamental assumption" that you think he is making? Maybe I am making it too.

That setSyncProvider() needs to be called every time a sync is to occur, and that that is the only time the sync occurs.

PaulS:

So what is the "flawed fundamental assumption" that you think he is making? Maybe I am making it too.

That setSyncProvider() needs to be called every time a sync is to occur, and that that is the only time the sync occurs.

Well, that's not an assumption I am making. But I can't speak for purehunter...

Hi purehunter,

I believe your problem is in adjustDstEurope(), it works if I remove the call from getNtpTime as so:

     unsigned long epoch = secsSince1900 - seventyYears ;  // removed + adjustDstEurope();

adjustDstEurope is calling time library functions including month() and day(). It would not suprise me if this is causing recursive calls to getNtpTime, which would explain why it "never returns" once you call setSyncProvider in setup.

Is this part of the standard example that comes with 1.0.3?

Cheers,
John

johncc:
Is this part of the standard example that comes with 1.0.3?

P.S. I downloaded 1.0.3 and confirmed there is no Time library that "comes with" the IDE. The most current/recommended Time library appears to be Arduino Playground - Time which does not include the adjustDstEurope in it's example. But it does require an additional library, "udpbytewise"

John

purehunter:
Nick,

did you think of this?

volatile boolean s_flag = false;

Yes that was what I meant. I didn't necessarily think it would solve your problem, but I like to eliminate obvious problem areas.

Hello everybody

Ran into the same problem syncing GPS with the time-library. Solution was easy but hard to find:

Sync (routine defined with setSyncProvider) is executed with every call of now() after syncInterval has expired. Just place now() somewhere in the loop() portion of your program.

See also: http://www.adafruit.com/forums/viewtopic.php?f=31&t=36378

Greetings
hpb

PaulS:
.....
There is another method to define when the synchronization is to occur. It might be possible to modify the library to have some specific interval (perhaps 0) have special meaning, like sync now but only once. It might even already have that capability.

I know, this in an old thread - but I had the other day a similar problem in that I want the system to only sync once.

I am now using

setTime((time_t)getNtpTime());

as proposed by purehunter in this thread. It works nicely.

I also tried

setSyncProvider(getNtpTime);
setSyncInterval(0);

as above quote suggests that it might be implemtented in the library.
It seems not to be implemented, as above 'setSync' code queries the time frequently instead of once.

Just FYI for others running in this problem.

You could point the setSync at a null function, after getting it once from the effective one.

hpbertsch solution works for me. Thanks.