Problem with the Frequency meter code

Question

I am trying to build a frequency meter using Interrupts and Timer1, which works as if the system receives an Interrupt it would get the timer's data and would calculate

Signal's Frequency = 1 / (Counter * Timer's Period) => F_s = F_t/TCNT

but the result is way off, and it does not even make sense. I have had altered my code to look that if the timer's data is not valid or sth, but the number that the timer counter has counted is sth that I expect.

Here is my code:

int f=0;
char msg[50];

void setup()
{
  Serial.begin(115200);
  noInterrupts();           // disable all interrupts
  pinMode(13, OUTPUT);
  digitalWrite(13, LOW);
  TCCR1A = 0;
  TCCR1B = 0;
  OCR1A = 0;
  OCR1B = 0;
  TCCR1B |= (0<<CS12)|(1<<CS11)|(0<<CS10);    // Clock 2MHZ 
  TCNT1  = 0;  
  TIMSK1 = 0; // disable timer overflow interrupt 
  interrupts(); 
  attachInterrupt(1,pin_ISR,RISING); 
}

void pin_ISR(){
  //digitalWrite(13,!digitalRead(13));
  f = 2000000/TCNT1;
  TCNT1=0;
}

void loop()
{
  sprintf(msg,"Humidity:%d \n",f); 
  Serial.print(msg);
}

Answer 1

As stated in my comment, your code is kind of fine. Id mostly works, but it looses a few microseconds on each iteration. If you are looking at multi-kHz frequencies, you probably want to avoid loosing microseconds. The solution is simple: never reset the counter. I changed you ISR as follows:

uint16_t previous_counter;

void pin_ISR(){
    uint16_t counter = TCNT1;
    f = 2000000/(counter - previous_counter);
    previous_counter = counter;
}

and now it gives the correct frequency... on average. There are some variations though, dues mostly to delays caused by other interrupts.

Edit 1: If you want to avoid the fluctuations caused by other interrupts, your best bet is probably to use the input capture feature of Timer 1.

Edit 2: Here is why your measurement was somewhat off. Your original ISR essentially does something like this:

void pin_ISR() {
    uint16_t temporary_value = TCNT1;
    loose_some_time();
    TCNT1 = 0;
}

The time you loose in the ISR is the part of the signal period you are not measuring, which means you are underestimating the period by that much. In the ISR you posted, this is mostly the time taken by a 32-bit division, which is quite significant. A simple improvement would be to clear TCNT1 right after reading it:

void pin_ISR() {
    uint16_t temporary_value = TCNT1;
    TCNT1 = 0;
    loose_some_time();
}

and now the average period you measure is almost right. It is not perfect though, as the whole reading and writing of TCNT1 takes 8 CPU cycles.

The alternative I am suggesting, where you never clear the timer, is the correct way of measuring a period with a timer interrupt: you don't loose a single CPU cycle. The best way is to use input capture: you get rid of the software-induced jitter.

Note: You should also take care of the very valid points raised in Jot's answer, about making f volatile and reading it with interrupts disabled.

Answer 2

Your approach is basically a reciprocal counter and it can work, if you start and stop the timer in the isr. The logic is essentially this:

If timer is on stop the timer and save the timer count. Else initialize the timer and start counting.

For counting a wide range of frequencies, you may need to consider timer over flow.

Edit: any solution that reads a multi byte counter while it is running will produce the wrong results. Google atomicity to understand why.

Answer 3

Did you find an example that works that way ? Is it, for example, allowed that the TCNT1 is cleared in an interrupt routine? The TCCR1A is cleared, but not set to a value ?
[edit] Clearing TCCR1A is okay. Clearing TCNT1 is also okay, but can be improved as @EdgarBonet shows.

The calculation uses a long of 2MHz, but the others are 16 bits. Could you force that calculation with long ? f = int( 2000000L / long( TCNT1));
[edit] It is not needed to force the compiler into a 'long' calculation, it already does so.

The 'f' variable is used in an interrupt and outside that interrupt. It must be made 'volatile': volatile int f=0;

The 'f' is used in the loop, but the interrupt could happen when 'f' is only half read or half written. The ATmega328P is a 8-bit microcontroller and 'f' is a 16-bits variable. I suggest to disable the interrupts and make a copy.

noInterrupts();
int fcopy = f;
interrupts();
sprintf(msg,"Humidity:%d \n",fcopy);

What is the range that you want to measure ?

Do you know that there are good libraries to measure a frequency ? They are the FreqMeasure Library and the FreqCount Library.

[ADDED]
Try to slow down the loop with a delay. If you fill the transmit buffer of the Arduino Uno TX, then the Serial.println wait for each character if there is a place in the transmit buffer.

Answer 4

So I have had asked this question on the Arduino forum and some one has fixed my code which gives me the accuracy that I expect. The problem was that the division was better not to be in the ISR routine but in the main loop. Here's the link to that answer because that guy should be credited as he helped me big time. https://forum.arduino.cc/index.php?topic=486573.0

Thanks to all who replied to me, I have learned many things about ISR's and their limitations.