Wandering GPS Timing???

Projects Networking, Protocols, and Devices
1

Hi all,

I am building a GPS synchronized clock and am confused by the behavior of the program. I'm looking for some help. The GPS sentence is used to update/sync a DS3234 RTC clock chip, update a MAX7219 driven 7 segment LED display, and write to an LCD.

I have written three libraries and modified (slightly) the Time.h library. Here's what they do:

The GPS lib is written for a USGlobalSat EM318-02. This is very similar to the 406, but uses an external antenna. I think the commands are the same, and obviously the sentences are to NMEA standard. The initialization call suppresses all sentences except $GPRMC, assures that it is transmitted every second, and resets the communication speed to 9600 baud. When a get time request is sent to the GPS, a TimeElements struct is passed by reference (it is a global variable in the sketch), the GPS sentence is parsed, and the TimeElements struct is updated only if the sentence is valid. I've written a bunch of overhead code to re-initialize variables at each call to make sure that I'm not dealing with old data-- probably more than I need to. I am confident, after innumerable Serial.println() calls, that the data is valid and the TimeElements struct is being filled in correctly.

The DS3234 RTC chip is a very sophisticated device that communicates by SPI with the arduino. My lib is based loosely on the Sparkfun demo sketch. A get time or set time call passes the global TimeElements struct by reference and either uses the struct to set the DS3234 time or get the DS3234 time. No bells and whistles used or accessed. The initialization routine sets the chip to output a 1 Hz square wave, which I use as the heartbeat; it communicates by an interrupt. Again, lots of Serial.println() calls, and I think the right info is being passed to/from the chip.

The MAX7219 lib is a simple library that accepts a TimeElements struct and updates the display on 8 7 segment LED displays. It also permits control over brightness. A silly but useful frill on the clock is a PIR motion detector with very focused directionality (put the detector in a toilet paper tube while prototyping!). As a bedside clock, just wave your hand over the clock and the display brightens!

The Time.h lib is modified only insofar as the the TimeElements struct has int, rather than byte members. This reduces a lot of typecasting in other libs, and the RAM in the Mega is so plentiful a few extra bytes doesn't matter. The only routines I use are makeTime and breakTime, and that is used to set the local time from the UTC time provided by the GPS sentence.

So, what's the quesion?

The sketch initiates fine, gets a GPS time, pops it into the DS3234. Every square wave rising edge I query the DS3234, get the time, and update the displays. For testing purposes, I have set up my update routine to query the GPS every minute and reset the DS3234. I am very confused by the results. The update clearly happens in less than a second, probably more along the line of ~350 ms (writing to an LCD is SLOWWW.). The update routine looks for when Time.Second == 0 and Time.Minute ==0, as output by the DS3234. I then query the GPS, and the GPS time is often several seconds off. Sometimes it's a few seconds ahead, sometimes it's 20 seconds behind. I have been round and round on this. I can understand that the times might not sync by a second or two, but it is beyond me why it varies so greatly and so randomly. Does anyone have any ideas?

BTW, I would be happy to post the code for the GPS library, DS3234 library, and MAX7219 library. Hopefully they would be of use to someone. All the MAXIM RTC chips have a similar basic time register structure, so modification of the library would be relatively simple (swap I2C calls for SPI calls, change a couple of specific registers, etc.) to adapt for other DS series devices.

Thanks.

Ed

2

Good questions, thanks.

I am using a separate hardware serial port (the project is based on a Seeeduino Mega) to talk to the GPS.

I have not done a direct device to device comparison. That's a good idea, and I'll try that tonight.

The data sheet indicates that as long as a write cycle is complete within one second, the device will be happy. The every minute write is only for debugging. In real use, it will drop to once a day, or once a week given the stability of the DS3234.

Ed

3

Sounds like you've got a lot of complicated code.

Write a simple sketch that does nothing more than report the time from the RTC at 1Hz, and copy data from the GPS at 1Hz. I'd probably do something like copy from the GPS a byte at a time (as it comes in), and as soon as I copy a newline character from the GPS sample the RTC and print that. Let this run for a while and see if there is discrepancy.

This tells you if it's your hardware or your code. (I suspect the code, 'cause like I said, it sounds complicated).

Many GPS units have a separate PPS (pulse per second) output for synchronizing more precisely than the NMEA standard 4800 baud sentences will allow. This may help with your synchronization when you start fine-tuning.

-j

4

Thanks. my guess is the code as well, but I've had a heck of a time isolating the problem. Clearly, I will need to start from scratch and isolate out just the GPS and the RTC. What is bothersome is that I am reusing what appeared to be very solid code for the GPS that was in another project.

5

but I've had a heck of a time isolating the problem.

Have you checked your RAM usage?

Bizarre behavior out of nowhere is one symptom of running out of RAM.

-j

6

I thought about it briefly, and then rejected the idea. I'm using a Seeduino Mega, which has 8Kb of RAM and 128K of flash. I would be VERY surprised if I were using more than 1 or at most 2K. Despite the relatively complexity of the operations, even the code is only 11.3K (I realize that doesn't correlate to RAM usage, just mentioning that.) I did wonder if I was trashing memory, writing past an array bound or otherwise corrupting my code and/or variables, and I will need to look at that very closely again. At the risk of inciting a ferocious partisan discussion, I will say that one of the reasons I always liked programming in Pascal when it was more common was because of very strong type checking and boundary checking which prevented my sloppy amateur mistakes. Arguably, an environment for hobbyists and non-programmers such as Arduino might benefit from such an option implemented as a compiler directive.

7

At the risk of inciting a ferocious partisan discussion, I will say that one of the reasons I always liked programming in Pascal when it was more common was because of very strong type checking and boundary checking which prevented my sloppy amateur mistakes.

Peer reviews are another good way to prevent amateur or sloppy mistakes. Psst. We're your peers. Post your code. We criticize it. I mean critique it.

8

Thanks. Here's the .pde, and the libraries and includes are in the zip file attached. This is far from polished, and there are some extra variables that I have not yet removed. Comments/critiques are welcome.

#include <DS3234.h>

#include <Time.h>

#include <GPSClass.h>

#include <MAXDriver.h>

#include <SPI.h>

#include <LiquidCrystal.h>


#define RequestGPS A2
#define LedPin A7

#define dataPin 4
#define clockPin 5
#define load 48
#define _CS 49


static TimeElements theTime;

time_t timeinsecs;
byte GPSRequested = false;
boolean ValidGPS = false;
boolean AutoUpdate = false;
volatile boolean triggerFlag = false;
volatile boolean updatedisplay = false;
long const EDT = -(60*60*4);
long counter = 0;
long other;
long secondcounter = 0;
boolean highoutput = false;
int a,b;

// invoke classes

LiquidCrystal lcd(A0,A1,A3,A4,A5,A6);
MAXDriver driver(dataPin,clockPin,load);
DS3234 rtc(_CS);
GPSClass em;


void setup () {
  Serial.begin(9600);
  em.GPSInit();
  rtc.DS3234Init();
  //get sensor input for changing screen brightness and LCD illumination
  attachInterrupt(0,trigger,HIGH);
  // get 1 PPS input
  attachInterrupt(1,UpdateDisplay,RISING);
  pinMode(A0,OUTPUT);
  pinMode(A1,OUTPUT);
  pinMode(RequestGPS,INPUT);
  pinMode(A3,OUTPUT);
  pinMode(A4,OUTPUT);
  pinMode(A5,OUTPUT);
  pinMode(A6,OUTPUT);
  pinMode(A7,OUTPUT);
  digitalWrite(A7,LOW);
  lcd.begin(4,20);
  lcd.noCursor();
  lcd.clear();
}

void loop () {

 
  if (highoutput){
    if (millis() - secondcounter > 10000){
      highoutput = false;
      driver.WriteARegister(0xA,0);
      // need to add code for LCD illumination
    }
  }
  if (triggerFlag) {
    secondcounter = millis();
    driver.WriteARegister(0xA,0xF);
    // need to add code for LCD illumination.
    highoutput = true;
    triggerFlag = false;
  }
  if (updatedisplay){
    lcd.setCursor(0,0);
    if (GPSRequested)
      ValidGPS = em.GetGPSTime(theTime);
    if (ValidGPS){
      rtc.setDS3234Time(theTime);
      lcd.setCursor(0,2);
      lcd.print("                    ");
      lcd.setCursor(0,2);
      lcd.print("UPDT ");
      if (AutoUpdate)
        lcd.print("AUT ");
      else
        lcd.print("REQ ");
      lcd.print((int)theTime.Hour);
      lcd.print(":");
      lcd.print((int)theTime.Minute);
      lcd.print(":");
      lcd.print((int)theTime.Second);
      GPSRequested = false;
      ValidGPS = false;
    }//valid gps
    else
      rtc.getDS3234Time(theTime);
    lcd.print("                    ");
    lcd.setCursor(0,0);
    lcd.print("U ");
    lcd.print((int)theTime.Month);
    lcd.print("/");
    lcd.print((int)theTime.Day);
    lcd.print("/");
    lcd.print((int)theTime.Year-30);
    lcd.print(" ");
    lcd.print((int)theTime.Hour);
    lcd.print(":");
    lcd.print((int)theTime.Minute);
    lcd.print(":");
    lcd.print((int)theTime.Second);
    timeinsecs = makeTime(theTime);
    timeinsecs = timeinsecs + EDT;
    breakTime(timeinsecs,theTime);
    digitalWrite(A7, HIGH);
    lcd.setCursor(0,1);
    lcd.print("                    ");
    lcd.setCursor(0,1);
    lcd.print("L ");
    lcd.print((int)theTime.Month);
    lcd.print("/");
    lcd.print((int)theTime.Day);
    lcd.print("/");
    lcd.print((int)theTime.Year-30);
    lcd.print(" ");
    lcd.print((int)theTime.Hour);
    lcd.print(":");
    lcd.print((int)theTime.Minute);
    lcd.print(":");
    lcd.print((int)theTime.Second);
    driver.Update(theTime);
    digitalWrite(A7,LOW);
    updatedisplay = false;
  }
    a = (int)theTime.Minute;
a = 0;
  b = (int)theTime.Second;
   if((digitalRead(RequestGPS)==LOW)||((a==0)&&(b==0))){
    if ((a==0)&&(b==0))
      AutoUpdate = true;
    else
      AutoUpdate = false;
    GPSRequested = true;
  }

}



void trigger(){
  triggerFlag=true;
}

void UpdateDisplay(){
  updatedisplay = true;
}

Ringel GPS DS3234 files.zip (16.4 KB)

9

Lots of unnecessary white space. Judicious use is good, but you've gone a bit overboard.

static TimeElements theTime;

Global variables are automatically static.

  //get sensor input for changing screen brightness and LCD illumination
  attachInterrupt(0,trigger,HIGH);

The need for an interrupt here completely escapes me. It is not like the button press results in an immediate reaction. When the button is pressed, the interrupt fires, and the need to change the brightness is recorded. That need is not checked until later, and not all that much later, so polling seems plenty fast/responsive enough.

    a = (int)theTime.Minute;
a = 0;
  b = (int)theTime.Second;
   if((digitalRead(RequestGPS)==LOW)||((a==0)&&(b==0))){

What is a contributing to this picture? What do the names a and b contribute to understanding what this code does?

Most of the code in loop() is for actually writing the date and time to the lcd. I'd recommend creating a function to write to the lcd.

In that function, I'd suggest that you put the { and } on separate lines, and indent code properly between them. As the code is now, you write the date and time to the lcd if you get a time from the GPS. Then, you write the date and time from the RTC to the lcd. It takes careful scrutiny to see that the dates and times are written in different places. Some comments and proper block alignment would make the code easier to follow.

10

Thanks for your comments. I admit the code is a bit sloppy; I didn't clean it up before posting. The interrupt is for another process. I have a PIR motion detector attached as well. When activated, it fires the trigger interrupt and brightens the LED display and turns on the backlight of the LCD for 10 seconds.

I agree that the LCD management should be encapsulated in a function. I will probably put it in its own file and call it as an include, with the .h file providing the function prototype.

The "a" and "b" variables were acts of desperation trying to figure out what was going wrong. They aren't good programming practice, and again I should have cleaned up the code before I posted it.

11

Can you really move so fast that you need an interrupt?

More importantly, can the PIR sensor react that fast?

12

Interesting comments; I'm actually a bit surprised. It had been my understanding that the way to use an interrupt was for an infrequent, random event. That way the main event loop didn't need to waste time polling for input that 99% of the time wasn't going to be there. My further understanding was to keep time in the interrupt routine at an absolute minimum because it interferes with internal timing functions.

My response was to treat a request for increased illumination (that is activation of the motion detector) as an interrupt. To keep time in the interrupt routine at a minimum, I simply have the routine set a flag. In the main event loop, processing of the flag is a simple true-false if statement: no calculations, no checking input states. Should be fast and minimally intrusive. Processing of the event only occurs if the flag is set, at which point it is correct to spend main event loop time on the request.

Where am I off base? Worrying too much?

Ed

13

Where am I off base? Worrying too much?

I think so. You've added a lot of complexity to replace this:

if(digitalRead(switchPin) == HIGH) // (or LOW)

with this:

if(interruptTrippedEarlier)

So what if the former takes a little longer? 99.9% of the time, loop() is going to do nothing useful anyway.

14

My comment is this:

    lcd.print((int)theTime.Month);
    lcd.print("/");
    lcd.print((int)theTime.Day);
    lcd.print("/");
    lcd.print((int)theTime.Year-30);
    lcd.print(" ");
    lcd.print((int)theTime.Hour);
    lcd.print(":");
    lcd.print((int)theTime.Minute);

Why all the "(int)" casts? Those fields are ints anyway:

typedef struct  { 
  int Second; 
  int Minute; 
  int Hour; 
  int Wday;   // day of week, sunday is day 1
  int Day;
  int Month; 
  int Year;   // offset from 1970; 
} 	tmElements_t, TimeElements, *tmElementsPtr_t;

Don't just cast for the hell of it, that can be hiding issues. Also it adds clutter.

And this:

                numberbuffer[0] = printbuffer[0];
		numberbuffer[1] = printbuffer[1];
		theTime.Day = atoi(numberbuffer);
		numberbuffer[0] = printbuffer[2];
		numberbuffer[1] = printbuffer[3];
		theTime.Month = atoi(numberbuffer);
		numberbuffer[0] = printbuffer[4];
		numberbuffer[1] = printbuffer[5];

Isn't this crying out for a helper function? One that takes two digits from printbuffer at a specified location and returns it converted to an int.

Now, as for your real problem :slight_smile: I am a bit worried about tests for equality:

if ((a==0)&&(b==0))

If for some reason you miss a second, then the whole test is delayed. Anyway, why use the heartbeat concept anyway? A simple delay of a second would achieve the same effect wouldn't it? It's a clock. If you update the display every second that should be enough. It doesn't have to be synchronized to an external pulse.

I can't see anything major wrong except for all the flags being set, being tested next time around the loop, being cleared later, and interrupts possibly resetting them unexpectedly. It's the confusion that is probably hiding the real problem.

Like, instead of setting a flag this time around the loop and acting on the flag the next time around, why not just:

if ( <some condition> )
  {
  < do the thing you want to do >
  }

... and do away with the flags, and clearing the flags, etc.

15

FWIW Ed, I agree with your use of interrupts here. The interrupt isn't triggered often enough to cause problems, and you are doing it exactly right: doing nothing but setting a flag in the ISR, which means you get in and get out, minimizing the amount of time spent in the ISR.

A human button press event is probably long-lived enough to simply sample as others suggest, but the ISR method looks OK to me.

-j

16

I agree with kg4wsv - the ISR routines do not seem to be the problem, per se.

17

Thanks for all the comments.

(Sigh), I promise, promise, promise the next time I post code I will clean it up first. (I have not posted code previously) I do know better, I was being sloppy and hadn't consolidated changes. I do like the idea of a helper function for parsing the GPS string. I had not thought of that previously, and it will clean things up.

Now, Nick, as you say, for the real problem. I thought a great deal about whether or not to use an external heartbeat. I decided to do so because I couldn't really think of a good way to keep the clock synchronized with a delay. Each bit of processing in the main event loop added up to about 200 milliseconds. If I delayed a second there would be an additive error which would periodically be corrected. It might make adding functionality such as an alarm more difficult. There's no way I could gauge exactly how much of a delay to put in (e.g. 200 ms processing + 800 ms delay), because I plan to have other code doing other things which may throw that off. Alternatively, I could test for 1000 milliseconds passing and use that as a trigger for an update:

if (millis() - old_time> 1000){

do display update;
old_time = millis();

} //end if millis()...

At that point, why not use the DS3234's square wave?

I thought (and this may be the crux of the problem) that I had written the code so that a delay in performing the test would not break the code. To my mind, missing an opportunity to do an update from the GPS is closely related to receiving an invalid string from the GPS: the DS3234 should simply still do its work and wait for the next opportunity to update itself.

I stand by my use of interrupt routines. I also plan to have a 4X4 keyboard as part of this project (ultimately.) I have not used this chip previously, but I plan to outsource some of the work to a 74C923. That chip monitors the keyboard and sends out a data available signal when a key is pressed. Then, all I need to do is read the state of 4 pins and I'm done. Perfect for an interrupt. However, that functionality is several weeks away. first we get updates correct.

With the weekend coming, more of an opportunity to pound away at this. I also promise that the next batch of code I put up will be properly scrubbed.

Again, thanks all.

Ed

18

edringel:
I decided to do so because I couldn't really think of a good way to keep the clock synchronized with a delay.

But why? At a given point in time X, if you get the GPS time and immediately set the clock to it, the clock will be right. Then you can read the clock every second, every 5 seconds, whatever. The reading interval only affects the resolution of the time being displayed.

Geoff Graham did a GPS synchronized clock here:

I have one hanging on my wall.

That only updates the internal time from the GPS every couple of days! And it doesn't use a clock chip at all. Basically it gets the GPS time, sets it "internal" time from it, and then just uses the processor timer to tick the clock every second for hours at end. After all the internal clock won't drift that much. Then next time it checks the time (a day or so later) it works out a delta - that is, the measured amount of CPU clock drift. Say for example when it checks the GPS a day later it finds its "internal" time was 5 seconds out. So then it averages out that 5 second difference over the next day to get a pretty accurate time next time.

Now you might want to update from the GPS more often than that - but every second or every minute sounds like overkill.

19

I think I understand this now, and have found a solution.

The point of the project was to create a portable GPS synchronized clock, something that could be used as a travel clock perhaps. The reason for a GPS altogether, rather than just depending upon the very accurate DS3234, is to automatically set local time. Although I have not knocked together the code for that part of the project, I have a pretty good idea for an algorithm that is feasible. (Hint: look at the geonames website.) GPS devices use a considerable amount of current and if the device is running from a battery, should be turned off when not needed. Navigating devices don't turn off the receiver, because they always need the GPS information. Likewise the uC uses a fair bit of power. When the clock was not in use, the idea was to power down the uC, shut off the GPS, and maintain time with the RTC chip. This is why the RTC chip was there in the first place. Although some of you may disagree, I chose to use the square wave output of the DS3234 as the synchronization source for getting GPS updates, refreshing the display, etc. That's fine. There are a bunch of ways to make this work, and it suits my purposes very well. We can agree to disagree. A few readers also questioned why I was updating the RTC from the GPS every minute. Testing purposes only; I didn't want to wait for an hour or 15 minutes to pass to see how the update fared. Real code will be perhaps once a day, if that.

I went back into my files and put in a lot of debugging code. What's going on here is fairly subtle, and you may question my results. The first critical point is that the RTC is a few hundred milliseconds different from the GPS. At my level of sophistication, that will be an inevitability. The implications of that are that the GPS sentence will not always be available when the square wave from the RTC requests it; the GPS really wants to set timing here, and gets very unhappy when it cannot, resulting in invalid sentences or no sentences. I added delay(100); every time there was an invalid sentence. This permitted the RTC and the GPS to synchronize fairly rapidly, usually within 15 seconds or so. What I then found, which is very puzzling to me ( the part that is somewhat unbelievable) is that the first couple of sentences after resynchronization sometimes had an "A" character in the validity field, implying valid data, but that the time differed from my WWVB based clock by sometimes as much as 30 or 40 seconds. This would resolve by the fourth consecutive valid GPS sentence. I put in code that would update the DS3234 chip with GPS data only after there had been 4 consecutive valid sentences. I have had no further wandering of the GPS time. I do not understand this behavior, and have not yet researched it.

Well, so that's the end of the story, at least from my end.

Lessons learned: Helper functions, clean up code before posting, accommodate phase differences between time keeping devices, make sure that data is valid when recovering from error conditions.

20

edringel:
What I then found, which is very puzzling to me ( the part that is somewhat unbelievable) is that the first couple of sentences after resynchronization sometimes had an "A" character in the validity field, implying valid data, but that the time differed from my WWVB based clock by sometimes as much as 30 or 40 seconds.

I think I can explain that. When I first turn on my GPS after it has been off for a while the clock may indeed be out by quite a bit. It must be using an internal clock chip to get the approximate time. The approximate time is better than no time at all, when it comes to working out which satellites are likely to be overhead. So, 40 seconds later, and using its "memory" of the time, it finds the satellites and the time becomes very accurate.

Mind you, your test for "A" would appear to be correct in testing that the data is valid, but nevertheless, I think that somehow it must not have the satellite lock. Are you testing for "A" or "not V"? And if so, maybe you are a byte out?