Timer1 stops firing

Question

I am trying to use interrupts to play notes on a speaker. Timer2 works fine, but after about 15 seconds, Timer1 stops firing for about 5 seconds, and then it comes back on. If I keep running the sketch, it will stop and resume every now and then.

Here is my code:

#define SPEAKER1 8
#define SPEAKER2 7
volatile bool swap1;
volatile bool swap2;

ISR(TIMER1_COMPA_vect)
{
    digitalWrite(SPEAKER1, swap1);
    swap1 = !swap1;
}

ISR(TIMER2_COMPA_vect)
{
    digitalWrite(SPEAKER2, swap2);
    swap2 = !swap2;
}

void playNote1(unsigned note) { //Play a note on Timer1.
    uint8_t oldSREG = SREG;
    cli();
    bitSet(TCCR1B, CS10);
    bitSet(TCCR1B, CS12);
    unsigned ticks = (16000000 / 1024) / (note);
    ticks /= 2;
    OCR1A = (ticks <= 65535 ? ticks : 65535);
    SREG = oldSREG;
}

void playNote2(unsigned note) { //Play a note on Timer2.
    uint8_t oldSREG = SREG;
    cli();
    bitSet(TCCR2B, CS20);
    bitSet(TCCR2B, CS21);
    bitSet(TCCR2B, CS22);
    unsigned ticks = (16000000 / 1024) / (note);
    ticks /= 2;
    OCR2A = (ticks <= 255 ? ticks : 255);
    SREG = oldSREG;
}

void stopNote1() {  //Stop playing a note on Timer1.
    uint8_t oldSREG = SREG;
    cli();
    bitClear(TCCR1B, CS10);
    bitClear(TCCR1B, CS12);
    SREG = oldSREG;
}

void stopNote2() {  //Stop playing a note on Timer2.
    uint8_t oldSREG = SREG;
    cli();
    bitClear(TCCR2B, CS20);
    bitClear(TCCR2B, CS21);
    bitClear(TCCR2B, CS22);
    SREG = oldSREG;
}


void setup() {
    pinMode(SPEAKER1, OUTPUT);
    pinMode(SPEAKER2, OUTPUT);
    pinMode(13, OUTPUT);
    // initialize timers
    cli();          // disable global interrupts
    TCCR1A = 0;     // set entire TCCR1A register to 0
    TCCR1B = 0;     // same for TCCR1B
    // turn on CTC mode:
    bitSet(TCCR1B, WGM12);
    bitSet(TIMSK1, OCIE1A);

    TCCR2A = 0;     // set entire TCCR2A register to 0
    TCCR2B = 0;     // same for TCCR2B
    // turn on CTC mode:
    bitSet(TCCR2A, WGM21);
    bitSet(TIMSK2, OCIE2A);
    sei();          // enable global interrupts
}

void loop() {
    playNote1(523);
    delay(1000);
    stopNote1();
    delay(500);
    playNote1(131);
    delay(2000);
    stopNote1();
    delay(500);
}

Answer 1

The problem you have is that you never reset Timer1 counter (TCNT1) when you stop a note and then play another one.

Also, changing the clock divider selection to "no clock" by clearing bits CS10 and CS12 as you do is not the most effective way to stop the timer; in fact, rather than stopping it, you can let it run but just disable its interrupts.

I have modified your program and this worked correctly:

#define SPEAKER1 8
volatile bool swap1;

ISR(TIMER1_COMPA_vect)
{
    digitalWrite(SPEAKER1, swap1);
    swap1 = !swap1;
}

void playNote1(unsigned note) { //Play a note on Timer1.
    uint8_t oldSREG = SREG;
    cli();
    unsigned ticks = (16000000 / 1024) / (note);
    ticks /= 2;
    OCR1A = (ticks <= 65535 ? ticks : 65535);
    TCNT1 = 0;
    bitSet(TIMSK1, OCIE1A);
    SREG = oldSREG;
}

void stopNote1() {  //Stop playing a note on Timer1.
    uint8_t oldSREG = SREG;
    cli();
    bitClear(TIMSK1, OCIE1A);
    SREG = oldSREG;
}

void setup() {
    pinMode(SPEAKER1, OUTPUT);
    pinMode(13, OUTPUT);
    // initialize timers
    cli();          // disable global interrupts
    TCCR1A = 0;     // set entire TCCR1A register to 0
    TCCR1B = 0;     // same for TCCR1B
    TCCR1C = 0;     // same for TCCR1C
    // Set clock divider
    bitSet(TCCR1B, CS10);
    bitSet(TCCR1B, CS12);
    // turn on CTC mode:
    bitSet(TCCR1B, WGM12);
    bitClear(TIMSK1, OCIE1A);
    sei();          // enable global interrupts
}

void loop() {
    playNote1(523);
    delay(1000);
    stopNote1();
    delay(500);
    playNote1(131);
    delay(2000);
    stopNote1();
    delay(500);
}

Note that I have removed all code for Timer2 as it does not play any role in the initial problem.

The important changes are:

1. setup() now performs Timer1 setup completely (CTC mode, clock divider); note that I also included TCCR1C to ensure full setup (it is preferrable as Arduino libraries may have initialized it differently before setup() is called); it also ensures Timer1 interrupts are initially disabled.
2. playNote1 now clears TCNT1 and then enable Timer1 interrupt for TIMER1_COMPA_vect
3. stopNote1() just disables Timer1 interrupt for TIMER1_COMPA_vect

To be complete, you would also want to limit useless consumption on output pin 8, thus stopping a note should also force its output level to LOW (or HIGH depending on your actual circuit behind, but LOW is more likely):

void stopNote1() {  //Stop playing a note on Timer1.
    uint8_t oldSREG = SREG;
    cli();
    bitClear(TIMSK1, OCIE1A);
    swap1 = false;
    SREG = oldSREG;
    digitalWrite(SPEAKER1, LOW);
}

Now what about your code for Timer2, the code is the same, so why does it work?

Actually it doesn't :-)

But you can't hear the difference between Timer2 is an 8-bit counter so if you don't reset it, then it won't take long until it reaches 255 and resets to 0, so only the first wave phase will be wrong but your hear won't notice that...

So you should also modify the code for Timer2 to make it correct.