ATmega AVR frequency measurement with timer/counter

Question

I've been rehashing some existing libraries I found on some obscure forums to measure a frequency on digital pin 5, using the ATmega328P/ATmega2560 Timer/Counter modules and an interrupt based approach in order to be able to measure frequencies up to 4MHz(-ish). However, using the code described below, the results are off by a factor of exactly 100. I used a frequency generator to accomplish these readouts. Here is an example of what I've been getting back when generating a 5000Hz square wave (50% duty cycle):

enter image description here

Can someone please point out what might be wrong. I've read the ATmega328P datasheet multiple times by now and it should just work! This is very strange.

Edit: As it turns out the AWG I use has a blantant software bug which causes it to generate a wave with a period that's 100 times too long (see picture below). Nevertheless as Edgar pointed out I made some pretty big mistakes by not taking into account race conditions. Therefore the code below has been updated. Also, the test program has been updated to generate a 50% duty cycle square wave @5000Hz for testing purposes as suggested by Edgar.

AWG software bug

This is the header file:

#ifndef FreqCounter_h
#define FreqCounter_h
#include <avr/interrupt.h>
#if defined(ARDUINO) && ARDUINO >= 100
#include "Arduino.h"
#else
#include "WProgram.h"
#endif
namespace FreqCounter 
{
 extern volatile unsigned long f_freq;
 extern volatile unsigned char f_ready;
 extern volatile unsigned char f_mlt;
 extern volatile unsigned int f_tics;
 extern volatile unsigned int f_period;
 void start(int ms);
}
#endif

This is the source file:

#include <FreqCounter.h>
volatile unsigned long FreqCounter::f_freq;
volatile unsigned char FreqCounter::f_ready;
volatile unsigned char FreqCounter::f_mlt;
volatile unsigned int FreqCounter::f_tics;
volatile unsigned int FreqCounter::f_period;
void FreqCounter::start(int ms) {
 f_period = ms;
 GTCCR = (1<<TSM) | (1<<PSRASY) | (1<<PSRSYNC); //halt all timers
 //set TC1 to normal mode => TOV1 flag set at TOP = 0xFFFF
 TCCR1A &= 0x00; //reset TC1 control register A
 TCCR1B &= 0x00; //reset TC1 control register B
 //set TC1 to use external clock source, clocked on rising edge
 TCCR1B |= (1<<CS10);
 TCCR1B |= (1<<CS11);
 TCCR1B |= (1<<CS12);
 //reset TC2
 TCCR2A &= 0x00; //reset TC2 control register A
 TCCR2B &= 0x00; //reset TC2 control register B
 //set TC2 to CTC Mode with TOP = OCR2A => counter cleared on reaching TOP
 TCCR2A |= (1<<WGM21);
 //TC2 is in CTC mode => set value of OCR2A = TOP
 OCR2A = 124; //TOP is reached every 125 counts (starting from 0)
 //set TC2 to use a prescaler of 1 => 16MHz / 128 = 125kHz
 TCCR2B |= (1<<CS20);
 TCCR2B |= (1<<CS22);
 f_ready = 0; //reset ready flag
 f_tics = 0; //reset TC2 interrupt counter
 //synchronize the value of both timers
 TCNT2 = 0;
 TCNT1 = 0;
 //clear interrupt flags
 TIFR2 |= (1<<OCF2A);
 TIFR1 |= (1<<TOV1);
 TIMSK2 |= (1<<OCIE2A); //enable TC2 Output Compare A interrupt => triggered on reaching TOP = OCR2A
 TIMSK1 |= (1<<TOIE1); //enable TC1 overflow interrupt => triggered on reaching TOP = MAX = 0xFFFF
 GTCCR &= 0x00; //restart all timers => start counting
}
/*
 * This ISR is triggered by TC2 every 1ms because
 * TC2 increments with a frequency of 16MHz / 128 = 125KHz
 * and reaches TOP every 125 counts 
 * => trigger frequency = 1kHz
 * => trigger period = 1ms
 * 
 * This ISR handles the Gate Time generation
 */
ISR(TIMER2_COMPA_vect) 
{
 //increment number of TC2 interrupts (triggers when reaching TOP = OCR2A)
 FreqCounter::f_tics++;
 /*
 * If branch evaluates as true if given gate time (f_period) equals 
 * amount of milliseconds TC2 has counted (= f_tics)
 */
 if (FreqCounter::f_tics >= FreqCounter::f_period) 
 {
 //end of gate time => measurement ready
 TCCR1B &= ~0x07; //set TC1 to no clk source => TC1 stops counting
 TIMSK2 &= ~(1<<OCIE2A); //disable TC2 Output Compare A interrupt
 TIMSK1 &= ~(1<<TOIE1); //disable TC1 overflow interrupt
 FreqCounter::f_ready = 1; //set software flag for end of gating period
 //increment on overflow of TCNT1
 if (bit_is_set(TIFR1, TOV1))
 FreqCounter::f_mlt++;
 //calculate frequency
 FreqCounter::f_freq = 65536 * FreqCounter::f_mlt;
 FreqCounter::f_freq += TCNT1; //add value of TC1
 FreqCounter::f_freq = (long)(FreqCounter::f_freq * 1000.0 / (double)FreqCounter::f_tics);
 FreqCounter::f_mlt = 0;
 }
}
ISR(TIMER1_OVF_vect)
{
 FreqCounter::f_mlt++;
}

This is the test program:

#include <FreqCounter.h>
long frq;
void setup()
{
 //generate a 5 kHz square wave on OC0B = digital pin 5 = T1
 DDRD |= _BV(PD5); //PD5 = OC0B as output
 TCCR0B = 0; //stop timer
 TIMSK0 = 0; //disable overflow interrupt
 OCR0A = 50 - 1; //period = 50 * 64 cycles
 OCR0B = 50 / 2 - 1; //50% duty cycle
 TCCR0A = _BV(COM0B1) //non-inverting PWM on OC0B
 | _BV(WGM00) //fast PWM, TOP = OCR0A
 | _BV(WGM01); //...ditto
 TCCR0B = _BV(WGM02) //...ditto
 | _BV(CS00) //clock at F_CPU / 64
 | _BV(CS01); //...ditto
 Serial.begin(57600);
 Serial.println("Frequency Counter");
}
void loop()
{
 FreqCounter::start(1000);
 while (FreqCounter::f_ready == 0);
 frq = FreqCounter::f_freq;
 Serial.println(frq);
}

PS: This is my first ever post on this forum. Feel free to comment on the contents of my question. Tips as to how I might improve future questions are always welcome. Have a great day!

• Gerben: Try swapping the order of operations to FreqCounter::f_freq * 1000 / FreqCounter::f_tics
• Robbe Elsas: @Gerben unfortunately changing that didn't make a difference and if it did that would've been completely irrational. Thanks anyway!
• Gerben: Not completely irrational. The variables are integers, but you want the calculation to be performed as floating point. Swapping some parts around will negate the need for floats all together, as demonstrated. Too bad that didn't help here. Have you tried measuring different frequencies? Are the all of by a factor of 100?
• Robbe Elsas: @Gerben you're right indeed. And to answer your question: yes I have. All other frequency measurements are also off by a factor 100. That's what's strange to me actually. This leads me to believe there might be some default prescaler weirdness I'm not aware off. I'm used to working with STM's and have worked with XMEGA devices in the past but since an Arduino has some pretty annoying default settings I've been bumping into these sorts off things all the time lately. I'm commited to using the Arduino platform for this project unfortunately.
• Gerben: The code looks okay to me. Very weird indeed. What happens if you change the period (e.g. FreqCounter::start(100);)

Answer 1

I am posting this as an answer, just because it doesn't fit as a comment. But more than an answer it is intended to be a debugging help.

First, I would like to point out that you can use the same Arduino to both generate the test signal and read it. This can be done by configuring Timer 0 (the last one you have left...) to output the 5 kHz square wave on OC0B (digital pin 5), and wire this signal to pin T1 which is... also digital 5. No wiring needed then! You can do this by adding the following to your test program's setup():

// Generate a 5 kHz square wave on OC0B = digital pin 5.
DDRD |= _BV(PD5); // PD5 = OC0B as output
TCCR0B = 0; // stop timer
TIMSK0 = 0; // disable overflow interrupt
OCR0A = 50 - 1; // period = 50 * 64 cycles
OCR0B = 50/2 - 1; // 50% duty cycle
TCCR0A = _BV(COM0B1) // non-inverting PWM on OC0B
 | _BV(WGM00) // fast PWM, TOP = OCR0A
 | _BV(WGM01); // ...ditto
TCCR0B = _BV(WGM02) // ...ditto
 | _BV(CS00) // clock at F_CPU / 64
 | _BV(CS01); // ...ditto

Then I would like to point out a couple of bugs in the library you are using. First, there is an off-by-one error: the program is counting one millisecond too much. This can be fixed by moving the lines

//increment number of TC2 interrupts (triggers when reaching TOP = OCR2A)
FreqCounter::f_tics++;

from the bottom to the top if ISR(TIMER2_COMPA_vect).

Next, there is a race condition between the ISRs. Race conditions are nasty bugs because they are hard to reproduce and most of the time you don't see them. And then the program just fails at a random time after many tests showed it works fine.

Here it works as follows: let's say that after the required time has elapsed, TIMER2_COMPA fires and right after that TIMER1_OVF fires also. This second interrupt is put on hold (it's a pending interrupt) while TIMER2_COMPA_vect runs through its prologue. Then you compute the frequency based on FreqCounter::f_mlt (which has not been updated to account for the overflow) and TCNT1 (which has already overflown to zero). Thus you get an incorrect result. Not only that, the overflow flag remains set, so the ISR will run right away as soon as the interrupt is enabled again by your next call to FreqCounter::start(). And again you get an incorrect result.

The fix is two fold: first you have to detect the pending interrupt condition in ISR(TIMER2_COMPA_vect): after stopping the timers and before computing FreqCounter::f_freq, you need this:

// Did we miss an overflow?
if (bit_is_set(TIFR1, TOV1))
 FreqCounter::f_mlt++;

Next, you have to clear the interrupt flag. This is customarily done right before enabling the interrupt, and just to be safe you do it for every interrupt you enable. Thus, in FreqCounter::start(), right before touching TIMSK2 and TIMSK1, you would:

// Clear interrupt flags.
TIFR2 = _BV(OCF2A);
TIFR1 = _BV(TOV1);

There are a couple of other minor issues, like the statement TCCR2A &= ~(1<<WGM22); which makes no sense (the bit belongs to another register). But given that this, like a few other statements, has no effect whatsoever, it should not affect the working of the program. It does reveal sloppy programming though.

• Robbe Elsas: Wow I can't believe the level of noobishness I displayed by neglecting to even think about race conditions. If my former professor in computer sciences knew about this he would strangle me with his bare hands. Also, I should really check my code before running off to forum threads. I will make the suggested corrections at once and report back to you. If it's fixed I will edit my post. Thanks in advance.