I am having an issue with trying to time an external 1PPS pulse. My goal is to measure the microcontroller's clock drift against a known good external source (in this case, a GPS module), so that when GPS lock is lost I have a quick and dirty way to keep somewhat accurate time until the next GPS lock (without having to buy an RTC).
For the most part, my solution appears to work on the bench. My test setup includes two nanos: one driving a 1PPS signal (just a mock up of an external source) and one running my measurement code. My issue is that every once in a while, I get a spurious update that gets a measured elapsed time of -20ms. When this happens, I end up getting an extra interrupt and measuring about ~1 second short, skewing my correction factor.
I have attached complete source to this post. The "ExternalClockSource" sketch is the ~1PPS output nano, "ClockCorrection" is the nano that is doing the measurement and emitting the above logs. ClockCorrection.zip (3.1 KB)
Apologies, ignore the comment in ClockCorrection::update() that says "This check needed to counter correction of timer0". I forgot to clean up that comment when I reverted to a simple != check.
It will tell you how to post code correctly using code tabs.
Many members use mobile devices and they will not cope with a .zip file and what it contains. So by posting code properly there more people who will be able to help you.
The solution to my problem is that the variable "gClockCorrectionLastMillis" (type unsigned long, aka 4 bytes) was getting overwritten during the copy into isrMillis. Take a look at what is happening on the instruction level, if we assume the value of gClockCorrectionLastMillis to be 0xAABBCCDD, then the instruction:
isrMillis = gClockCorrectionLastMillis;
is equivalent to:
char *dest = (char *)isrMillis;
char *src = (char *)gClockCorrectionLastMillis;
dest[0] = src[0]; // "0xAA" Natively, the arduino is copying one byte at a time
// The interrupt can happen here
dest[1] = src[1]; // "0xBB"
// The interrupt can happen here
dest[2] = src[2]; // "0xCC"
// The interrupt can happen here
dest[3] = src[3]; // "0xDD"
So if the interrupt fires during this line and sets to gClockCorrectionLastMillis to 0xFFFFFFFF, isrMillis can end up any of the values 0xAABBCCFF, 0xAABBFFFF, or 0xAAFFFFFF.
The moral of the story is, never assume that "X = Y" is an atomic operation!
I think you still have the issue that millis() increments by two sometimes. That's because it actually runs a bit slow. But you can replace the default millis() with your own millis timer0 interrupt that increments at exactly 1ms intervals.
/*
This is a replacement for the ISR(TIMER0_OVF_vect)
interrupt that drives millis(). It disables the OVF
interrupt, enables the COMPA interrupt, sets OCR0A to 249,
and changes Timer0 to CTC mode. This results in an
interrupt every 250 timer clock cycles, which is exactly 1ms
for a 16MHz crystal, or 2ms for an 8MHz crystal. The new ISR
increments millis, but since the interrupt rate is exactly
correct, no periodic double increment of millis is needed.
Using this code probably means you can't do any analog
writes that use Timer0, which would include pins 5 and 6.
*/
extern volatile unsigned long timer0_millis; //these defined in wiring.c
extern volatile unsigned long timer0_overflow_count;
volatile unsigned long MILLIS_INCB = (64 * 250) / (F_CPU / 1000); // ms between timer overflows
const int cycleTime = 500; // flash LED every second
int LEDcount = cycleTime;
unsigned long oldMillis = millis();
unsigned long newMillis = 0;
void setup() { //Set up alternate interrupt
// at 249 on timer0
cli(); // disable interrupts while doing this
TCCR0A = 0; // set entire TCCR0A register to 0
TCCR0B = 0; // same for TCCR0B
TCNT0 = 0; // initialize timer0 count to 0
OCR0A = 249; // set top of counter
TIMSK0 &= ~bit(TOIE0); // disable overflow interrupt
TCCR0A |= bit(WGM01); // turn on CTC mode
TCCR0B |= (bit(CS01)+bit(CS00)); // Set CS00&CS01 bits for prescaler = 64
TIMSK0 |= bit(OCIE0A); // enable timer compare interrupt
sei(); // enable interrupts
pinMode(13,OUTPUT);
digitalWrite(13,HIGH);
}
void loop() { // flashes LED for 1/2 second
// every second
newMillis = millis();
if ((newMillis - oldMillis) > 0) {
oldMillis = newMillis;
LEDcount -= 1;
if (LEDcount == 0) {
digitalWrite(13,!digitalRead(13)); // invert pin 13 state
LEDcount = cycleTime;
}
}
}
ISR(TIMER0_COMPA_vect) { // this is the new ISR - much
// simpler than original
timer0_millis += MILLIS_INCB;
timer0_overflow_count++;
}
Thanks for the code, but the point of this code is that the long-term drift of the CPU clock is accounted for. It won't be perfect, but it should be better than just using millis() to manage a local clock during loss of gps sync. It assumes that ambient temperature (and therefore clock drift factor) is reasonably stable, and is not meant for weeks or months of gps loss. This code is going into an NTP server.