Arduino UNO timer1 interrupt

I've started working with interrupts on arduino UNO, but after few frustrated sketches, i've found that arduino timer1 is not working as i expected.
For checking this i tried this piece of code:

volatile long timer; //Timer to keep count of the ISR ocurred

void setup(){

cli(); //Disable global interrupts

TCCR1A = 0; //Register set to 0
TCCR1B = 0; //Register set to 0
TCNT1 = 0;

OCR1A = 15999; //Counter for 1KHz interrupt 16*10^6/1000-1 no prescaler
TCCR1B |= (1 << WGM12); //CTC mode
TCCR1B |= (1 << CS10); //No prescaler
TIMSK1 |= (1 << OCIE1A); //Compare interrupt mode

sei(); //Enable global interrupts

Serial.begin(9600);
}

ISR(TIMER1_COMPA_vect){
timer++; //Increment timer each ISR
}

void loop(){
Serial.println(timer); //Print timer value
delay(1000); //Send to serial port every second, so timer should be
//increasing by 1000, but this is the part that fails
}

when i open the serial monitor it should show the timer increasing by 1000, but thats not case as i see the timer increasing by 1000 at the beginning, but then starting to get higher than expected (34001, 48002...).

I have tried multiple prescalers and different values of counters, but always getting the same result.

Is this a normal behaviour of the interrupts? i don't know if is a bug in mode code (hope so) or if is something wrong with my arduino.

It is normal. It is not a bug in the Arduino software. Your Arduino board is okay.

You delay 1000ms using delay(). That is good enough.
However, you also use : Serial.println(timer);
That function needs execution time, and that is what you see when you print the number of interrupts.

So the interrupts are accurate, the TIMER1 and the crystal are okay. But you don't print it every second.
You can use millis() to keep in perfect pace with the seconds:

volatile long timer; //Timer to keep count of the ISR ocurred

void setup(){
 
  cli(); //Disable global interrupts
 
  TCCR1A = 0; //Register set to 0
  TCCR1B = 0; //Register set to 0
  TCNT1 = 0;
 
  OCR1A = 15999; //Counter for 1KHz interrupt 16*10^6/1000-1 no prescaler
  TCCR1B |= (1 << WGM12); //CTC mode
  TCCR1B |= (1 << CS10); //No prescaler
  TIMSK1 |= (1 << OCIE1A); //Compare interrupt mode
 
  sei(); //Enable global interrupts
 
  Serial.begin(9600);
}

ISR(TIMER1_COMPA_vect){
  timer++; //Increment timer each ISR
}

void loop(){
  // Use millis to do this loop once a second. That is exactly once a second.
  static unsigned long old_millis = 0UL;   // zero-unsigned-long
  
  if( millis() - old_millis >= 1000UL)
  {
    Serial.println(timer);   //Print timer value

    // advance to next event, regardless the time needed by Serial.println()
    old_millis += 1000UL;    
  }
}

The result is: 1000, 2000, 3000, 4000, ..... 300000 ..... and so on.
That is a perfect match, since TIMER0 (for millis) and TIMER1 both use the same crystal.

Paste your sketch between [code] ... [/code] tags.

thanks for the answer, i understand what was wrong with my code.

I have another question about timers: when i try this sketch to check my timer i was trying a simple pulse generator, and measuring output freq doesn't mach with expected values (only a minimal difference), does analogwrite o digital write take many processor cycles that effects are noticeable?

sorry about my english.

Yes.
The ATmega has an assembly instruction to change an output pin. That one takes 125 ns, but the normal digitalWrite() takes 3 to 4 us. The analogWrite() takes 10 to 12 us.

Peter_n:
Yes.
The ATmega has an assembly instruction to change an output pin. That one takes 125 ns, but the normal digitalWrite() takes 3 to 4 us. The analogWrite() takes 10 to 12 us.

time to play with lower level functions then.

thanks for the help

Use this : http://arduino.cc/en/Hacking/PinMapping168
And use the macros http://arduino.cc/en/Reference/BitSet and http://arduino.cc/en/Reference/BitClear

After you set the pin 12 as output: pinMode(12,OUTPUT);
You can do those fast instructions, like this: bitSet(PORTB,4); // pin 12 = PB4, set it HIGH
The compiler will translate that into that fast instruction of 125ns.

I was thinking about addressing the port directly instead of using pin mapping, which is more effective for fast requirements?

This is addressing the port directly with 125ns : bitSet(PORTB,4); // pin 12 = PB4, set it HIGH