Return to Answer

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: 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.

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.

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.

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: 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.