Issue using Arduino Mega Timer2 with PinChange Interrupt

There are quite a few issues with this program. The most obvious is the use of floating point calculations in interrupt context. Interrupts should be served as fast as possible, and floating point is really slow on the AVR.

Some other issues:

• Interrupt flags should be cleared by writing a logic 1 to them, however silly it may sound.

• A couple if & operators in ISR(PCINT2_vect) are probably meant to be &&.

• The logic for detecting rising and falling edges is all wrong.

• There is no need to store so much data in the Pulse objects.

• ch1.width should be read with interrupts disabled in order to avoid a data race.

• There is a subtle race condition when reading ovfCount and TCNT2: if the timer overflows right when ISR(PCINT2_vect) starts running, that overflow will not be counted.

Here is a version of the program that attempts to fix all these points. I also changed it for running on an Uno (with pin 13 = PB5 = PCINT5), so that I could test it:

/*
 * Pulse width via PCINT.
 *
 * https://arduino.stackexchange.com/questions/81688
 */

volatile uint32_t ovfCount = 0;

struct Pulse {
    uint32_t last_toggle;
    uint32_t width;
    bool stateHigh;
    uint32_t get_width() {
        noInterrupts();
        uint32_t width_copy = width;
        interrupts();
        return width_copy;
    }
};

Pulse ch1, ch2, ch3, ch4, ch5, ch6;

void setup() {
    Serial.begin(115200);
    pinMode(13, OUTPUT);

    /* Setup Pin Change Interrupt for detecting pulses. */
    PCMSK0 = _BV(PCINT5);  // sense change on pin 13 = PB5 = PCINT5
    PCIFR  = _BV(PCIF0);   // reset interrupt flag
    PCICR  = _BV(PCIE0);   // enable PCINT0_vect: pins PCINT[7:0]

    /* Setup Timer 2 for counting. */
    TCCR2B = 0;           // stop
    TCCR2A = 0;           // normal mode
    TCNT2  = 0;           // reset timer
    TIFR2  = _BV(TOV2);   // reset interrupt flag
    TIMSK2 = _BV(TOIE2);  // enable timer overflow interrupt
    TCCR2B = _BV(CS20);   // count at F_CPU/1
}

ISR(TIMER2_OVF_vect) {
    ovfCount++;
}

ISR(PCINT0_vect) {
    uint8_t tcnt2 = TCNT2;
    uint32_t ovf_count = ovfCount;

    // Was there an overflow that has not yet been accounted for?
    if (bit_is_set(TIFR2, TOV2) && tcnt2 < 128) {
        ovf_count++;
    }

    uint32_t time = ovf_count << 8 | tcnt2;

    bool input_is_high = bit_is_set(PINB, PB5);
    if (input_is_high && !ch1.stateHigh) {  // low -> high
        ch1.stateHigh = true;
    } else if (!input_is_high && ch1.stateHigh) {  // high -> low
        ch1.width = time - ch1.last_toggle;
        ch1.stateHigh = false;
    }
    ch1.last_toggle = time;
}

void loop() {
    digitalWrite(13, HIGH);
    delay(1000);
    digitalWrite(13, LOW);
    delay(200);
    Serial.println(ch1.get_width() / 16e3, 5);
}

And here is the output:

1000.02349
1000.01959
1000.01916
1000.02496
1000.01959
1000.02032
1000.01947
1000.02795
1000.02642
1000.01855
1000.01916
1000.02105
1000.02490
1000.01782
1000.03033
1000.01898
...

The printed values are larger than 1000 because of the time needed to execute digitalWrite().

Also note that an 8-bit timer running at the full CPU speed overflows every 16 µs. That's quite a lot of pressure on the CPU, and many operations will take longer that expected because of these interrupts. If I wanted to time something with single-cycle resolution, I would reach for a 16-bit timer. And I would prefer using input capture, although sadly only two input capture channels are available on the Mega (on pins 48 and 49).

Edit: Answering the questions in the comments.

please have a look at my question edits and please advice.

I did not test it, but it seems to me this code should work. I would prefer, however, having a different interrupt for each channel. Otherwise, the successive calls to pinChangeFunction() can make the execution of this ISR quite long.

Also note that Pulse::input_is_high serves no useful purpose. Storing this value in the object slows down the ISR, as it has to access memory, whereas a local variable would be kept in a (much faster) internal CPU register.

what will we do if we have 8 channels as in the radio receiver?

You could use 6 external interrupts and 2 pin change interrupts.

Why did you take tcnt2 < 128 when tcnt2 max is 256?

This one is tricky. I'll try to explain the whole logic of the test.

Consider the following naive version of the ISR:

ISR(PCINT0_vect) {
    // ← A
    uint8_t tcnt2 = TCNT2;
    uint32_t ovf_count = ovfCount;
    uint32_t time = ovf_count << 8 | tcnt2;
    // ...
}

There is a small chance that Timer 2 overflows at point A, i.e. while this ISR is executing its prologue and has not yet read TCNT2. If this happens, the TIMER2_OVF ISR will not run immediately, because we are already executing an ISR, and ISRs do not nest. Then, ovfCount is not updated. If this happens, we then read a value of TCNT2 affected by the overflow, and a value of ovfCount that does not account for that overflow. We then combine together those inconsistent values into a time value that misses 256 timer ticks.

We can detect this situation by reading the TOV2 flag in TIFR2. This flag is set only when there is a pending interrupt request for TIMER2_OVF, i.e. if the counter has overflowed and that overflow has not yet been accounted for in ovfCount. Thus the “fixed” ISR looks like this:

ISR(PCINT0_vect) {
    // ← A
    uint8_t tcnt2 = TCNT2;
    // ← B
    uint32_t ovf_count = ovfCount;
    // ← B
    if (bit_is_set(TIFR2, TOV2)) {
        ovf_count++;
    }
    uint32_t time = ovf_count << 8 | tcnt2;
    // ...
}

With this, if the overflow happens at point A, we manage to compute the proper time. But consider now what would happen if the timer overflows at either of the points marked B, i.e. after reading TCNT2 but before reading TOV2. If this happens, we read the pre-interrupt values of both TCNT2 and ovfCount. No correction is thus needed for computing a consistent time. Yet, TOV2 is raised when we test it: we then apply an unneeded correction, and we end up with a time that is too large by 256 ticks.

In order to fix this problem, we have to make sure the correction is applied if the overflow happens at point A, but not if it happens at point B. How can we tell apart those two situations? The answer is: by looking at the value recorded in tcnt2. In case A, the counter was read right after the overflow, so it had a very small value (close to 0) when it was copied to tcnt2. In case B, the counter was read when it was just about to reach the overflow, so it had a very large value (close to 255). If TOV2 is set when we read it, this means an overflow happened during the very beginning of the current ISR, very close to the point where we read TCNT2. So the value recorded in tcnt2 has to be either very small or very large. It cannot be in the middle (close to 128).

So we have to decide whether tcnt2 is very small or very large. There are many possible values that could be used as a threshold. The most natural is the mid range: 128. It is also the most efficient to compute, as the compiler can optimize the comparison into a simple test of the most significant bit. Thus the final version of the test:

    if (bit_is_set(TIFR2, TOV2) && tcnt2 < 128) {
        ovf_count++;
    }