Square wave generator generates a shorter pulse from time to time

Question

I am using an arduino Micro to generate 8 square waves on 8 pins. The idea is as follows: on pin 13 is the main square wave. It represents a certain BPM (beats per minute). The other 7 pins should give multiplications or divisions of that BPM.

It uses timer1 to trigger an interrupt at BPM/192 (this number is choosen to enable easy multiplications). It counts those interrupts and changes the states of the outputs when needed.

The codes seems to run fine for most divisions and most multiplications. However in certain cases like /2, there seem to be shorter pulses from time to time that throw the whole timing off.

Adding a short delay at the end of the loop seems to make the system more stable but still the short pulses appear from time to time.

Does anyone have an idea what the problem might be.

int counter[8] = {0,0,0,0,0,0,0,0}; //storing number of interrupts since last pulse
int trip[8] = {191,383,767,1535,3071,6143,12287,24575}; //number of interrupts needed for div/mult
int outputPin[8] = {13,12,11,10,9,8,7,6};

void setup() {

  for (int x = 0; x < 8; x++){

    pinMode(outputPin[x], OUTPUT);
    digitalWrite(outputPin[x], HIGH);

  }

  cli();
  TCCR1A = 0;
  TCCR1B = 0;
  TCNT1 = 0;
  OCR1A =2602; // 16000000 / (x * 8) -1
  TCCR1B |= (1 << WGM12);
  TCCR1B |= (0 << CS12) | (1 << CS11) | (0 << CS10);//prescaler of 8
  TIMSK1 |= (1 << OCIE1A);
  sei();

}

void pulse(){

  for (int count = 0; count < 8; count++){

    counter[count]++;

  }

}

ISR(TIMER1_COMPA_vect){

  pulse();
}

void loop() {

  for (int y = 0; y < 8; y++){

    if(counter[y] > trip[y]){

      digitalWrite(outputPin[y],!digitalRead(outputPin[y]));
      counter[y] = 0;

    }

  }

  delayMicroseconds(100);

}

Answer 1

There are two issues here:

1. There is a data race on counter[]: it can be modified in interrupt context while it is being read (or even modified) by the main program.

2. Clearing counter[y] will make you loose ticks if for some reason (say some interrupt) you do it a little bit late. You should instead decrement it by trip[channel].

Here is a safe way of managing the counters:

volatile int counter[8] = {...};  // don't forget `volatile`

// Return whether this channel has tripped, and update the count.
bool tripped(int channel) {
    noInterrupts();
    bool did_trip = counter[channel] >= trip[channel];
    if (did_trip) counter[channel] -= trip[channel];
    interrupts();
    return did_trip;
}

void loop() {
    for (int y = 0; y < 8; y++)
        if (tripped(y))
            digitalWrite(outputPin[y], !digitalRead(outputPin[y]));
}

Notice that the critical section (the section with interrupts disabled) lives within the for loop, rather than around it. It is better to have multiple small critical sections rather than one large one.

Edit 1: You don't need the delay. It makes the system “more stable” because it reduces the likelihood that, on any given TIMER1_COMPA interrupt, the main program is accessing the counters (as it will spend most of its time delaying). Adding this delay is in a way treating the symptoms rather than the root cause of the data race.

Edit 2: Answering a comment. If you want the squares waves to stay in sync, their frequency ratios should be integers. In your program, this is almost, but not quite the case. For example, if you look at the ratio between the first two channels:

channel 0: 191 interrupts per signal toggle
channel 1: 383 interrupts per signal toggle
ratio: 383 ÷ 191 ≈ 2.0052

The easy fix is to add 1 to every element of the trip array (384 ÷ 192 is exactly 2).