Help with Date library setSyncProvider(getNTPTime) within a class (SORTED)

Hi,

This is my first post, first Arduiono and first project! I've been programming a central heating controller with an ATMEGA 1280, LCD screen and an ethernet shield. The idea is that the scheduler program is a PHP script which runs at regular intervals (on a web server somewhere in California) and this sends commands to the Arduino via HTTP requests. It's been a fun and rewarding challengeso far and I've enjoyed making a working basic prototype; i.e. controlling an LED on the Arduino by sending commands from the server using the PHP CURL library. :slight_smile:

I've been cobbling code together and trying to do the sensible thing by keep everything neat and tidy using classes. I have written an NTP time class which wraps up some of the functionality used in the standard Time library but am having trouble with setSyncProvider(). I can see that the function expects a function but on complie. I get the following error:

NTPTime.cpp: In member function 'void NTPTime::retrieve()':
NTPTime.cpp:25: error: argument of type 'time_t (NTPTime::)()' does not match 'time_t (*)()'

My function void  NTPTime::retrieve() sets stuff up.

I'd be grateful for any pointers here on what I'm doing wrong. I'm from the world of VB.NET. :blush: Thanks!

p.s. the screen will be for showing off (GLCD lib): displaying the current whether using the google weather web service, current news using BBC news RSS (TinyXML lib), current time using NTP, etc. I'll post my results when complete :slight_smile:

// HEADER
#ifndef NTPTime_h
#define NTPTime_h

#include "WProgram.h"
#include <Time.h>

#define NTP_PACKET_SIZE 48

// typedef time_t(*getExternalTime)();

class NTPTime {
	public:
		NTPTime();
		void retrieve();
		char* time();
		char* date();
 	private:
                time_t getNTPTime();
                void sendNTPpacket(byte*);
		unsigned int _localPort;
		byte _timeServer[4];
		byte _packetBuffer[NTP_PACKET_SIZE];
};

#endif
#include "WProgram.h"
#include "NTPTime.h"

#include <Udp.h>
#include <Time.h>

// Constructor
NTPTime::NTPTime(){
   _timeServer[0] = 194; //{ 194, 35, 252, 7 }
   _timeServer[1] = 35;
   _timeServer[2] = 252;
   _timeServer[3] = 7;
  _localPort = 8888;
  Udp.begin(_localPort);
}

// Retrieive NTP time from server NTPTime::
void  NTPTime::retrieve(){
  setSyncProvider(getNTPTime); // I fail here :(
  while(timeStatus() == timeNotSet);
}

// Return current time 00:00:00
char* NTPTime::time(){
	static char buf[9];
	sprintf(buf, "%02d:%02d:%02d\0", hour(), minute(), second());
	return buf;
}

// Return current date
char* NTPTime::date(){
	static char buf[30];
	char _day[10];
	char _month[10];

	strcpy(_day, dayStr(weekday()));
	strcpy(_month, monthShortStr(month()));

	sprintf(buf, "%s %d %s %d\0", _day, day(), _month, year());

	return buf;
}


/***** Private functions *********/


// Retrieve NTP time
time_t NTPTime::getNTPTime(){

  sendNTPpacket(_timeServer); // send an NTP packet to a time server

    // wait to see if a reply is available
  delay(1000);

  if (Udp.available()) {

    Udp.readPacket(_packetBuffer, NTP_PACKET_SIZE);  // read the packet into the buffer

    //the timestamp starts at byte 40 of the received packet and is four bytes,
    // or two words, long. First, esxtract the two words:
    unsigned long highWord = word(_packetBuffer[40], _packetBuffer[41]);
    unsigned long lowWord = word(_packetBuffer[42], _packetBuffer[43]);

    // combine the four bytes (two words) into a long integer
    // this is NTP time (seconds since Jan 1 1900):
    unsigned long secsSince1900 = highWord << 16 | lowWord;

    // Unix time starts on Jan 1 1970. In seconds, that's 2208988800:
    const unsigned long seventyYears = 2208988800UL;

    // subtract seventy years:
    unsigned long epoch = secsSince1900 - seventyYears;

    return epoch;
  }
  return 0;
}


// send an NTP request to the time server at the given address
void NTPTime::sendNTPpacket(byte *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.sendPacket(_packetBuffer, NTP_PACKET_SIZE,  address, 123); //NTP requests are to port 123
}

It looks to me that the function you pass to setSyncProvider needs to be either a static class member, or not a member of a class at all. What you are trying to do requires a closure, but closures are not supported by C'98 (they are in C0'x).

The following should work around the problem [WARNING: untested code!], but note that it uses a static variable, so if you have two or more NTPTime objects trying to get the time concurrently, it may behave unexpectedly.

class NTPTime {
... // as before

private:
  static NTPTime *getTimeObject;

  static void globalGetNTPTime()
  {
     NTPobject->getNTPTime();
  }
}

void  NTPTime::retrieve(){
  getTimeObject = this;
  setSyncProvider(globalGetNTPTime); 
  while(timeStatus() == timeNotSet);
}

Thanks your help dc42! I shall give it a try now and let you know in a bit. :slight_smile:

You're right though; using a static member or a member not in a class is what the setSyncProvider() expects. I've worked this out after some Googley research. I didn't think this project would be so complicated but it's great to be learning a bit of C, even though my own requirements for this project are creeping!

I can see what you've done now: you've cleverly made a member function static by using the given object reference. I've changed the code slightly

            static NTPTime *getTimeObject;
                
             static time_t globalGetNTPTime() {
                   getTimeObject->getNTPTime();
              }

I can see it should work but shockingly it doesn't compile. :frowning: My C skills are lacking here. I even tried initialising my static object with NTPTime NTPTime::*getTimeObject = 0;

NTPTime.cpp.o: In function `NTPTime::globalGetNTPTime()':
/NTPTime.h:25: undefined reference to `NTPTime::getTimeObject'
/NTPTime.h:25: undefined reference to `NTPTime::getTimeObject'
NTPTime.cpp.o: In function `NTPTime::retrieve()':
C:\DOCUME~1\ADMINI~1\LOCALS~1\Temp\build4958169755527007255.tmp/NTPTime.cpp:21: undefined reference to `NTPTime::getTimeObject'
C:\DOCUME~1\ADMINI~1\LOCALS~1\Temp\build4958169755527007255.tmp/NTPTime.cpp:21: undefined reference to `NTPTime::getTimeObject'

Correct, the return type of globalGetNTPTime should indeed be time_t. Can you post the entire definition of class NTPTime and its members?

[EDIT: also you need to change "getTimeObject->getNTPTime();" to "return getTimeObject->getNTPTime();"]

Thanks, I just noticed I wasn't returning the time too :slight_smile: Here's the latest:

ZIPed http://www.supercrab.co.uk/NTPTest.zip

#ifndef NTPTime_h
#define NTPTime_h

#include "WProgram.h"
#include <Time.h>

#define NTP_PACKET_SIZE 48

class NTPTime {
	public:
		NTPTime();
		void retrieve();
		char* time();
		char* date();


 	private:
                time_t getNTPTime();
                void sendNTPpacket(byte*);
		unsigned int _localPort;
		byte _timeServer[4];
		byte _packetBuffer[NTP_PACKET_SIZE];

                static NTPTime *getTimeObject;
                
                static time_t globalGetNTPTime() {
                   return getTimeObject->getNTPTime();
              }
 
};

#endif
#include "WProgram.h"
#include "NTPTime.h"

#include <Udp.h>
#include <Time.h>

// Constructor
NTPTime::NTPTime(){
   _timeServer[0] = 194; //{ 194, 35, 252, 7 }
   _timeServer[1] = 35;
   _timeServer[2] = 252;
   _timeServer[3] = 7;
  _localPort = 8888;
  Udp.begin(_localPort);
}

//NTPTime NTPTime::*getTimeObject = 0;

// Retrieive NTP time from server NTPTime::
void  NTPTime::retrieve(){
  getTimeObject = this;
  setSyncProvider(globalGetNTPTime); 
  while(timeStatus() == timeNotSet);
}


// Return current time 00:00:00
char* NTPTime::time(){
	static char buf[9];
	sprintf(buf, "%02d:%02d:%02d\0", hour(), minute(), second());
	return buf;
}

// Return current date
char* NTPTime::date(){
	static char buf[30];
	char _day[10];
	char _month[10];

	strcpy(_day, dayStr(weekday()));
	strcpy(_month, monthShortStr(month()));

	sprintf(buf, "%s %d %s %d\0", _day, day(), _month, year());

	return buf;
}


/***** Private functions *********/

// Retrieve NTP time
time_t NTPTime::getNTPTime(){

  sendNTPpacket(_timeServer); // send an NTP packet to a time server

    // wait to see if a reply is available
  delay(1000);

  if (Udp.available()) {

    Udp.readPacket(_packetBuffer, NTP_PACKET_SIZE);  // read the packet into the buffer

    //the timestamp starts at byte 40 of the received packet and is four bytes,
    // or two words, long. First, esxtract the two words:
    unsigned long highWord = word(_packetBuffer[40], _packetBuffer[41]);
    unsigned long lowWord = word(_packetBuffer[42], _packetBuffer[43]);

    // combine the four bytes (two words) into a long integer
    // this is NTP time (seconds since Jan 1 1900):
    unsigned long secsSince1900 = highWord << 16 | lowWord;

    // Unix time starts on Jan 1 1970. In seconds, that's 2208988800:
    const unsigned long seventyYears = 2208988800UL;

    // subtract seventy years:
    unsigned long epoch = secsSince1900 - seventyYears;

    return epoch;
  }
  return 0;
}


// send an NTP request to the time server at the given address
void NTPTime::sendNTPpacket(byte *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.sendPacket(_packetBuffer, NTP_PACKET_SIZE,  address, 123); //NTP requests are to port 123
}

I can't see what's wrong either. You could try moving the definition of globalGetNTPTime into the .cpp file, i.e. in the .h file change its definition to:

  static time_t globalGetNTPTime();

and then in the main file add:

time_t NTPTime::globalGetNTPTime()
{
   return NTPTime::getTimeObject->getNTPTime();
}

I have just tried that too and still got the same undefined reference message NTPTime::getTimeOjbect :frowning: It seems weird, maybe it would work with a regular compiler?

I think a clue might be that every time the static variable getTimeObject is used there's an undefined reference error. Thanks so far thought! I think it's probably something obvious! :astonished:

NTPTime.cpp.o: In function `NTPTime::globalGetNTPTime()':
C:\DOCUME~1\ADMINI~1\LOCALS~1\Temp\build4958169755527007255.tmp/NTPTime.cpp:18: undefined reference to `NTPTime::getTimeObject'
C:\DOCUME~1\ADMINI~1\LOCALS~1\Temp\build4958169755527007255.tmp/NTPTime.cpp:18: undefined reference to `NTPTime::getTimeObject'
NTPTime.cpp.o: In function `NTPTime::retrieve()':
C:\DOCUME~1\ADMINI~1\LOCALS~1\Temp\build4958169755527007255.tmp/NTPTime.cpp:26: undefined reference to `NTPTime::getTimeObject'
C:\DOCUME~1\ADMINI~1\LOCALS~1\Temp\build4958169755527007255.tmp/NTPTime.cpp:26: undefined reference to `NTPTime::getTimeObject'

Maybe the Arduino IDE's preprocessing is getting in the way. Have you tried making getTimeObject a global variable (declared near the start of the main file) instead of a static class member?

I think that'll be the next thing to try after a sleep. :sleeping: If that doesn't work then I'll have to make all the members and variables shared. I'm hoping that'll work at the minimum otherwise it's all going into the main body of code and that would be a shame!

Cheers!

It appears that a sleep has brought me back to the project with a clear mind and I've got the code to work :slight_smile:
It turns out that all shared variables defined in the header file must be instanciated in the .cpp I tried this in my class to point to the object but it turns I was ever so slightly wrong

NTPTime NTPTime::*getTimeObject = 0;

It should have been

NTPTime* NTPTime::getTimeObject;

In English: Define static member variable getTimeObject as a pointer of type NTP time.

Great thanks to Mr dc42! :slight_smile:

Hi Supercrab

can you please share the ccp or h file to run setSyncProvider Correctly, i am having same trouble.

Thanks