Using TimerCounter2 as clock

With the timercounter2, I want to make a clock/timer with hours:minutes:seconds passed.
So the Arduino works on 16 MHz (16 Million ticks/second).
I use a prescaler of 8 so the timercounter2 has to tick 2 Million ticks/second
TimerCounter2 is an 8bit counter so it counts up to 255 then reset. That's why I set my compare register of the counter to 250 (easy to calculate with) and I use the interrupt that comes with it.
2 Million ticks / 250 = 8000, so after 8000 times getting the interrupt of comparing, I should have 1 second in my opinion.
But somethings seems wrong with my logic/code and I can't figure it out what is wrong...

I hope you could help me

Thanks in advance

My code:

#ifndef F_CPU
#define F_CPU           16000000UL  // 16 MHz
#endif

#include                <stdlib.h>
#include                <avr/io.h>
#include                <avr/interrupt.h> 
int counter1 = 0;
int seconds = 0;

void setup(){
  Serial.begin(115200);
  cli();
  
  // select CTC mode
  TCCR2A |= (1<<WGM21);
  //prescaler 8
  TCCR2B |= (1<<CS21);
  // enable compare interrupt
  TIMSK2 |= (1<<OCIE2A);
  //set compare register to 250
  OCR2A = 250;
  sei();
}

void loop()
{
  Serial.println(seconds);
  delay(500);
}

ISR(TIMER2_COMPA_vect)
{
  counter1++;
  if(counter1 == 8000)
  {
     seconds++;
     counter1 = 0;
  }
  TCNT2 = 0;
}

That's why I set my compare register of the counter to 250 (easy to calculate with) and I use the interrupt that comes with it.

Easy for who? Not the Arduino. It likes multiples of 2 a lot better.

But somethings seems wrong with my logic

The millis() function already returns the number of milliseconds that elapsed since the Arduino was reset. Why not use that, like everyone else does?

All variables used in ISRs and in other functions need to be volatile. Yours are not.

PaulS:
Easy for who? Not the Arduino. It likes multiples of 2 a lot better.

When you calculate that you need 8000 times the interupt when reaching the number 250 on the counter, you become 2 million (multiply that with 8 and you become the 16 million ticks / second)

PaulS:
The millis() function already returns the number of milliseconds that elapsed since the Arduino was reset. Why not use that, like everyone else does?

Because I need to make a clock using the timer

Thnx for answer, I hope you could still help me out

I need to make a clock using the timer

Do you need to make a clock or do you need to make a clock using the timer ?

Make the ISR variables volatile then try printing the value of counter1 and counter2 each time through loop(). How fast is counter2 incrementing ?