Return to Answer

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

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

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 that one large one.

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

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

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 that one large one.

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 that one large one.

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