How to properly count timer overflows to convert a 32-bit high-resolution timer into a 64-bit high-resolution timer

Question

I have a 32-bit core timer on a PIC32MZ microcontroller (datasheet) counting at 100 MHz.

The system is running FreeRTOS with multiple tasks (threads) and this code should be able to get called from any task at any time.

It rolls over every 2^32 ticks / 100000000 ticks/sec = 42.9 seconds. I'd like to safely convert it into a 64-bit counter.

There is an ISR which handles counting overflows. It looks something like this:

static volatile uint32_t rollover_count = 0;

void isr_core_timer_overflow()
{
    rollover_count++;
}

This concept would also work on 8-bit microcontrollers to convert from 8-bit counters to 16-bit or 32-bit counters.

Here is my proposed solution:

// For 32-bit mcus such as PIC32
uint64_t get_counts()
{
    uint32_t count1 = ReadCoreTimer();

    // No critical section needed because we are reading a 32-bit value, and
    // turning off interrupts doesn't stop the core timer from counting
    // anyway.
    uint32_t rollover_count_copy = rollover_count;

    uint32_t count2 = ReadCoreTimer();

    if (count2 < count1)
    {
        // rollover just occurred between reading count1 and count2, so manually
        // account for it; note: rollover_count will be incremented
        // by the rollover ISR, so I don't need to update it
        rollover_count_copy++;
    }

    uint64_t total_count = ((uint64_t)rollover_count_copy << 32) | count2;

    return total_count;
}

Part of me thinks I do need a critical section, but I can't wrap my head around why. It has something to do with the fact that when the core timer overflows, the overflow ISR would run, and maybe I would need to momentarily stop that.

If I had interrupt guards to protect a critical section, it might look like this:

// For 32-bit mcus such as PIC32
uint64_t get_counts()
{
    uint32_t count1 = ReadCoreTimer();

    // back up the interrupt state
    // - Note: the `__builtin_*` functions are part of the XC32 compiler, and 
    //   are documented in DS50002799C (MPLAB® XC32 C/C++ Compiler User's Guide
    //   for PIC32M MCUs; XC32 Compiler for PIC32M) p195, p265 (the assembly 
    //   code output for the functions is around here), etc.
    unsigned int isr_state = __builtin_get_isr_state();
    __builtin_disable_interrupts();  // critical section start

    uint32_t rollover_count_copy = rollover_count;
    uint32_t count2 = ReadCoreTimer();

    // restore the interrupt state
    __builtin_set_isr_state(isr_state); // critical section end

    if (count2 < count1)
    {
        // rollover just occurred between reading count1 and count2, so manually
        // account for it
        rollover_count_copy++;
    }

    uint64_t total_count = ((uint64_t)rollover_count_copy << 32) | count2;

    return total_count;
}

Or maybe move the count1 read into the critical section too to stop the rollover_count from incrementing in its ISR during that time too:

// For 32-bit mcus such as PIC32
uint64_t get_counts()
{

    unsigned int isr_state = __builtin_get_isr_state();
    __builtin_disable_interrupts();  // critical section start

    uint32_t count1 = ReadCoreTimer();
    uint32_t rollover_count_copy = rollover_count;
    uint32_t count2 = ReadCoreTimer();

    __builtin_set_isr_state(isr_state); // critical section end

    if (count2 < count1)
    {
        rollover_count_copy++;
    }

    uint64_t total_count = ((uint64_t)rollover_count_copy << 32) | count2;

    return total_count;
}

Which is correct? What solution would you propose? Are there any problems with my first approach above?

References

1. My 8-bit AVR solution that has been working flawlessly for a decade is in the top of this issue here, but I feel like a rewrite following the design above would run faster and be simpler, and now I need to write this for a PIC32 anyway, which I've never done before. The if (count2 < count1) check is to try to counter this race condition identified here.

2. The ReadCoreTimer() function is provided here inside the document titled PIC32 Peripheral Libraries for MPLAB C32 Compiler - also contains UpdateCoreTimer(), WriteCoreTimer(), etc.

3. PIC32MZ datasheet

4. https://www.microchip.com/en-us/tools-resources/develop/mplab-xc-compilers/xc32 --> scroll down to "MPLAB XC32 Compiler Documentation" and download "MPLAB XC32 C/C++ Compiler User's Guide for PIC32M MCUs".

   1. Direct link: MPLAB® XC32 C/C++ Compiler User's Guide for PIC32M MCUs--XC32 Compiler for PIC32M--DS50002799C.pdf - contains the documentation for the __builtin_* functions. From section 18.8 p195:

      unsigned int __builtin_get_isr_state(void)
      void __builtin_set_isr_state(unsigned int)
      unsigned int __builtin_disable_interrupts(void)
      void __builtin_enable_interrupts(void)
      

Related

1. Timer rollover handling - related, but not a duplicate

Notes to self:

1. Add a previous_count variable. Update it to the last returned value after each get_counts() call, and update it to 0 after each overflow ISR run. Use it to check for overflows.

2. For AVR, enable nested interrupts inside the overflow ISR. Ensure that the interrupt that interrupts that interrupt does not also allow nested interrupts.

   Nevermind: the interrupting interrupts could be pin change interrupts and they need to able to read timestamps to measure incoming PWM signals.

Answer 1

I've done exactly this for a similar system in real production code.

A few issues ...

1. You increment rollover_count_copy but never advance rollover_count
2. The change to rollover_count must be done in the critical section (i.e. interrupts disabled)
3. You don't want to read the timer twice. You want to have a global that keeps track of the "old" timer value.
4. For PIC*, it doesn't do much instruction reordering, so using volatile on globals under the critical section is sufficient (vs. using barriers).
5. Assuming that once you enter an ISR, the system is "single level" and can't be stacked (i.e. a second interrupt can not interrupt an ISR) ...
6. ... If you add a global that is set when within an ISR, you can speed up get_counts by not changing the interrupt flag(s) when called from an ISR. (i.e.) In an ISR, interrupts are already disabled.
7. The save/restore of interrupt context needs to be done only if called from base/task level.

---

Here is a slight rewrite of your function. It works when called from within an ISR and at task level. It is annotated:

#include <stdint.h>

// fake definitions
#ifdef __linux__
uint32_t ReadCoreTimer(void);
uint32_t __builtin_get_isr_state(void);
void __builtin_set_isr_state(uint32_t);
void __builtin_disable_interrupts(void);
#endif

volatile int in_isr = 0;                // 1=within ISR
volatile uint32_t old_timer = 0;        // previous timer value
volatile uint32_t rollover_count = 0;   // rollover count

// set this according to the system's interrupt architecture
#define CONFIG_INTERRUPTS_CANT_BE_STACKED   1

// For 32-bit mcus such as PIC32
uint64_t
get_counts()
{
    unsigned int isr_state;
    int isrnow = CONFIG_INTERRUPTS_CANT_BE_STACKED && in_isr;

    // back up the interrupt state
    // - Note: the `__builtin_*` functions are part of the XC32 compiler, and
    // are documented in DS50002799C (MPLAB® XC32 C/C++ Compiler User's Guide
    // for PIC32M MCUs; XC32 Compiler for PIC32M) p195, p265 (the assembly
    // code output for the functions is around here), etc.
    if (! isrnow) {
        isr_state = __builtin_get_isr_state();
        __builtin_disable_interrupts();     // critical section start
    }

    uint32_t rollover_count_copy = rollover_count;
    uint32_t curcount = ReadCoreTimer();

    if (curcount < old_timer) {
        // rollover just occurred between reading count1 and count2, so manually
        // account for it
        rollover_count = ++rollover_count_copy;
    }

    // save the current timer value for rollover detection on the _next_ call
    old_timer = curcount;

    // restore the interrupt state
    if (! isrnow)
        __builtin_set_isr_state(isr_state); // critical section end

    uint64_t total_count = ((uint64_t) rollover_count_copy << 32) | curcount;

    return total_count;
}

// some_ISR -- example ISR routine
void
some_ISR(void)
{

    in_isr = CONFIG_INTERRUPTS_CANT_BE_STACKED;

    get_counts();

    in_isr = 0;
}

// main -- example of task level function
int
main(void)
{

    get_counts();

    return 0;
}

Answer 2

Simply repeat the count generation if rollover changes:

uint64_t get_counts()
{
    uint32_t count_lo = 0u ;
    uint32_t count_hi = 0u ;

    do
    {
        count_hi = rollover_count ;
        count_lo = ReadCoreTimer() ;

    } while( count_hi != rollover_count ) ;
   
    return ((uint64_t)count_hi << 32) | count_lo ;
}

The loop will execute no more than twice, and generally once. The race condition exists as you say, but here it is detected and the result recalculated. Critically, with no disabling of interrupts.