A timer driver for Cortex-M0+... it rarely doesn't work

I don't know if it is an issue specific to Atmel SAMC21 Cortex-M0+ 
devices or not.

I wrote a simple timer driver: a 32-bits hw counter clocked at 875kHz 
(14MHz/16) that triggers an interrupt on overflow (every 1h 21').  In 
the interrupt I increment the 32-bits global variable _ticks_high.
The 64-bits number composed by _ticks_high (upper 32-bits) and the hw 
counter (lower 32-bits) is my system tick.  This 64-bits software 
counter, incremented at 875kHz, will never overflow during my life, so 
it's good :-)

In timer.h I have:
--- Start of timer.h ---
#include <stdint.h>
#include <io.h>  // Atmel specific

#define TIMER_FREQ    875000

typedef uint64_t Timer;
extern uint32_t _ticks_high;

#define volatileAccess(v) *((volatile typeof((v)) *) &(v))

static inline uint64_t ticks(void) {
   uint32_t h1 = volatileAccess(_ticks_high);
   TC0->COUNT32.CTRLBSET.reg =
        TC_CTRLBSET_CMD(TC_CTRLBSET_CMD_READSYNC_Val);
   uint32_t l1 = TC0->COUNT32.COUNT.reg;
   uint32_t h2 = volatileAccess(_ticks_high);
   if (h2 != h1) return ((uint64_t)h2 << 32) + 0;
   else          return ((uint64_t)h1 << 32) + l1;
}

static inline void TimerSet(Timer *tmr, uint64_t delay) {
   /* delay is in ms */
   *tmr = ticks() + delay * TIMER_FREQ / 1000;
}

static inline int TimerExpired(Timer *tmr) {
	return ticks() >= *tmr;
}
--- End of timer.h ---

In timer.c I have the ISR:
--- Start of timer.c ---
...
void TC0_Handler(void) {
   if (TC0->COUNT32.INTFLAG.reg & TC_INTFLAG_OVF) {
     ++_ticks_high;
     TC0->COUNT32.INTFLAG.reg = TC_INTFLAG_OVF;
   }
}
...
--- End of timer.c ---

The idea is simple (and stolen from a post appeared on this newsgroup). 
At first, the 64-bits software counter must be calculated disabling 
interrupts, because if a timer interrupt triggers during calculation, 
the overall software counter could be wrong.
By reading _ticks_high before and after reading hw counter, we can avoid 
to disable the interrupts.


Now I have a code that uses timers. It's a simple state-machine that 
manages communication over a bus.
--- Start of bus.c ---
...
int bus_task(void) {
   switch(bus.state) {
     case BUS_IDLE:
       if (TimerExpired(&bus.tmr_answer)) {
         /* Send new request on the bus */
         ...
         TimerSet(&bus.tmr_answer, timeout_answer);
         bus.state = BUS_WAITING_ANSWER;
       }
       break;

     case BUS_WAITING_ANSWER:
       if (TimerExpired(&bus.tmr_answer)) {
         /* No reply */
         bus.state = BUS_IDLE;
         TimerSet(&bus.tmr_answer, 0);
       } else {
         if (reply_received() == true)	{
           /* Analyze the reply */
           bus.state = BUS_IDLE;
           TimerSet(&bus.tmr_answer, 0);
         }
       }
       break;
   }
   return 0;
}
...
--- End of bus.c ---

I don't think I need to explain the code in bus.c. The only thing to 
specify is that bus_task() is called continuously in the main loop.

99% of the time this code works well. Unfortunately I have seen some 
strange events. Rarely (very rarely, one time in a week) the bus seems 
frozen for a time. After that it restarts the normal activity magically.
There's a thing that relates those strange events to driver of timers: 
the bus stall time lasts exactly 1h 21', the overflow period of hw counter.

I suspect there's a problem in my low-level driver and sometimes, maybe 
near the overflow, the code doesn't work as I expect.
Maybe the TimerSet() function sometimes sets a wrong value to the 
uint64_t timer, maybe a tick value that will happen only at the next 
overflow of the hw counter.

Do you see where is the problem?

On 2017-04-26, pozz <pozzugno@gmail.com> wrote:
> I don't know if it is an issue specific to Atmel SAMC21 Cortex-M0+ 
> devices or not.
>

[snip]

>
> 99% of the time this code works well. Unfortunately I have seen some 
> strange events. Rarely (very rarely, one time in a week) the bus seems 
> frozen for a time. After that it restarts the normal activity magically.
> There's a thing that relates those strange events to driver of timers: 
> the bus stall time lasts exactly 1h 21', the overflow period of hw counter.
>
> I suspect there's a problem in my low-level driver and sometimes, maybe 
> near the overflow, the code doesn't work as I expect.
> Maybe the TimerSet() function sometimes sets a wrong value to the 
> uint64_t timer, maybe a tick value that will happen only at the next 
> overflow of the hw counter.
>
> Do you see where is the problem?
>

Does changing the optimisation level change the problem ?

Does a review of the generated code using objdump match what you
would expect ?

Have you tried placing come kind of debug marker in TC0_Handler()
(maybe turning on an LED) to see if TC0_Handler() is called without
the interrupt flag being set ?

This last one is in case there's some kind of rare timing issue
which causes the overflow handler to be called without TC_INTFLAG_OVF
being set yet when INTFLAG is examined.

Simon.

-- 
Simon Clubley, clubley@remove_me.eisner.decus.org-Earth.UFP
Microsoft: Bringing you 1980s technology to a 21st century world

First you need to make ticks_high volatile.  It doesn't make any sense 
to me why you'd cast it volatile in one execution context but not the 
other.  If it's shared between two execution contexts, the compiler 
needs to know that.

It sounds like you have a concurrent access problem that you need to 
protect against.  If it was me, I'd disable interrupts instead of your 
trick.

JJS

Il 27/04/2017 01:24, Simon Clubley ha scritto:
> On 2017-04-26, pozz <pozzugno@gmail.com> wrote:
>> I don't know if it is an issue specific to Atmel SAMC21 Cortex-M0+
>> devices or not.
>>
>
> [snip]
>
>>
>> 99% of the time this code works well. Unfortunately I have seen some
>> strange events. Rarely (very rarely, one time in a week) the bus seems
>> frozen for a time. After that it restarts the normal activity magically.
>> There's a thing that relates those strange events to driver of timers:
>> the bus stall time lasts exactly 1h 21', the overflow period of hw counter.
>>
>> I suspect there's a problem in my low-level driver and sometimes, maybe
>> near the overflow, the code doesn't work as I expect.
>> Maybe the TimerSet() function sometimes sets a wrong value to the
>> uint64_t timer, maybe a tick value that will happen only at the next
>> overflow of the hw counter.
>>
>> Do you see where is the problem?
>>
>
> Does changing the optimisation level change the problem ?

As you can think, it's very difficult make a test and try, because I 
have to wait for the rare event that could happen after a week.

Anyway I think yes, disabling optimisation should solve, but it's not a 
solution.


> Does a review of the generated code using objdump match what you
> would expect ?

Do you mean reading the output listing with assembler instructions? I'm 
not an expert of ARM assembler, anyway I tried to read it and it seems 
correct.


> Have you tried placing come kind of debug marker in TC0_Handler()
> (maybe turning on an LED) to see if TC0_Handler() is called without
> the interrupt flag being set ?

No, it would be very strange. TC0_Handler() function address is stored 
only in the vector table, so it is called only when TC0 peripheral 
requests an interrupt.
Anyway, if the interrupt flag is not set, in TC0_Handler() there's a if 
and the increment of _ticks_high isn't done.

> This last one is in case there's some kind of rare timing issue
> which causes the overflow handler to be called without TC_INTFLAG_OVF
> being set yet when INTFLAG is examined.

There's an if and the increment of _ticks_high wouldn't be done.

Il 27/04/2017 02:17, John Speth ha scritto:
> First you need to make ticks_high volatile.  It doesn't make any sense
> to me why you'd cast it volatile in one execution context but not the
> other.  If it's shared between two execution contexts, the compiler
> needs to know that.

_ticks_high is accessed in TimerSet(), TimerExpired() and the ISR 
TC0_Handler().  Timerset() and TimerExpired() reads _ticks_high by 
calling ticks() function.  So _ticks_high is accessed only in two 
points: ticks() and TC0_Handler().

ticks() is called during normal background flow (not interrupt), so the 
access to _ticks_high must be volatile.
TC0_Handler() is the ISR, so I think a volatile access to _ticks_high is 
not necessary (the ISR can't be interrupted).


> It sounds like you have a concurrent access problem that you need to
> protect against.  If it was me, I'd disable interrupts instead of your
> trick.

I liked this trick because avoids disabling interrupts. TimerExpired() 
is often called.

I found the post (from Wouter van Ooijen) where this trick is suggested:
https://groups.google.com/d/msg/comp.arch.embedded/9d8I5FFbmX4/6yL33UR92F8J

Don Y suggested other improvements in the same thread.

On 27/04/17 00:55, pozz wrote:
> I don't know if it is an issue specific to Atmel SAMC21 Cortex-M0+
> devices or not.
> 
> I wrote a simple timer driver: a 32-bits hw counter clocked at 875kHz
> (14MHz/16) that triggers an interrupt on overflow (every 1h 21').  In
> the interrupt I increment the 32-bits global variable _ticks_high.
> The 64-bits number composed by _ticks_high (upper 32-bits) and the hw
> counter (lower 32-bits) is my system tick.  This 64-bits software
> counter, incremented at 875kHz, will never overflow during my life, so
> it's good :-)
> 
> In timer.h I have:
> --- Start of timer.h ---
> #include <stdint.h>
> #include <io.h>  // Atmel specific
> 
> #define TIMER_FREQ    875000
> 
> typedef uint64_t Timer;
> extern uint32_t _ticks_high;

Identifiers that begin with an underscore are reserved for file scope -
you are not supposed to use them for "extern" variables.  I would be
extremely surprised to find that a compiler treated them in any special
way, but them's the rules (&#4294967295;7.1.3 in C11 N1570.)

> 
> #define volatileAccess(v) *((volatile typeof((v)) *) &(v))

That looks familiar :-)

> 
> static inline uint64_t ticks(void) {
>   uint32_t h1 = volatileAccess(_ticks_high);
>   TC0->COUNT32.CTRLBSET.reg =
>        TC_CTRLBSET_CMD(TC_CTRLBSET_CMD_READSYNC_Val);
>   uint32_t l1 = TC0->COUNT32.COUNT.reg;
>   uint32_t h2 = volatileAccess(_ticks_high);
>   if (h2 != h1) return ((uint64_t)h2 << 32) + 0;
>   else          return ((uint64_t)h1 << 32) + l1;
> }

(I don't know what the CTRLBSET stuff is for - I am not familiar with
Atmel's chips here.)

If the low part of the counter rolls over and the interrupt has not run
(maybe interrupts are disabled, or you are already in a higher priority
interrupt, or interrupts take a few cycles to work through the system)
then this will be wrong - l1 will have rolled over, but h2 will not show
an updated value yet.

So if the high parts don't match, you need to re-read the low part and
re-check the high parts.  It is perhaps easiest to express in a loop:

static inline uint64_t ticks(void) {
	uint32_t h1 = volatileAccess(_ticks_high);
	while (true) {
		uint32_t l1 = TC0->COUNT32.COUNT.reg;
		uint32_t h2 = volatileAccess(_ticks_high);
		if (h1 == h2) return ((uint64_t) h2 << 32) | l1;
		h1 = h2;
	}
}

You can also reasonably note that this loop is not going to be re-run
more than once, unless you have interrupt functions that last for an
hour and a half, or something similarly bad - and then the failure of
the ticks() function is the least of your problems!  Then you can write:

static inline uint64_t ticks(void) {
	uint32_t h1 = volatileAccess(_ticks_high);
	uint32_t l1 = TC0->COUNT32.COUNT.reg;
	uint32_t h2 = volatileAccess(_ticks_high);
	if (h1 != h2) l1 = TC0->COUNT32.COUNT.reg;
	return ((uint64_t) h2 << 32) | l1;
}


> 
> static inline void TimerSet(Timer *tmr, uint64_t delay) {
>   /* delay is in ms */
>   *tmr = ticks() + delay * TIMER_FREQ / 1000;
> }
> 
> static inline int TimerExpired(Timer *tmr) {
>     return ticks() >= *tmr;
> }

When you want a boolean result, return "bool", not "int".  (This is not
your problem, of course - it's just good habit.)

An alternative method here is to just use the low 32-bit part (with
delays limited to 2^31 ticks), but be sure that you have considered
wraparound.  Keep everything in unsigned at first, to make overflows
work as modulo arithmetic:

static uint32_t ticks32(void) {
	return TC0->COUNT32.COUNT.reg;
}

static inline void TimerSet32(Timer32 *tmr, uint32_t delay) {
	*tmr = ticks32() + delay * TIMER_FREQ / 1000;
}

static inline bool TimerExpired32(Timer32 *tmr) {
	int32_t d = ticks32() - *tmr;
	return (d >= 0);
}

Note that doing the subtraction as unsigned, then converting to signed
int, gives you the wraparound behaviour you need.  Converting an
unsigned int to a signed int when the result is out of range is
implementation dependent (&#4294967295;6.3.1.3), but gcc and many other compilers
define it as modulo arithmetic:
<https://gcc.gnu.org/onlinedocs/gcc/Integers-implementation.html>




> --- End of timer.h ---
> 
> In timer.c I have the ISR:
> --- Start of timer.c ---
> ...
> void TC0_Handler(void) {
>   if (TC0->COUNT32.INTFLAG.reg & TC_INTFLAG_OVF) {
>     ++_ticks_high;
>     TC0->COUNT32.INTFLAG.reg = TC_INTFLAG_OVF;
>   }
> }
> ...
> --- End of timer.c ---
> 
> The idea is simple (and stolen from a post appeared on this newsgroup).

"Good artists copy, great artists steal", as someone famous once said.

> At first, the 64-bits software counter must be calculated disabling
> interrupts, because if a timer interrupt triggers during calculation,
> the overall software counter could be wrong.
> By reading _ticks_high before and after reading hw counter, we can avoid
> to disable the interrupts.
> 
> 
> Now I have a code that uses timers. It's a simple state-machine that
> manages communication over a bus.
> --- Start of bus.c ---
> ...
> int bus_task(void) {
>   switch(bus.state) {
>     case BUS_IDLE:
>       if (TimerExpired(&bus.tmr_answer)) {
>         /* Send new request on the bus */
>         ...
>         TimerSet(&bus.tmr_answer, timeout_answer);
>         bus.state = BUS_WAITING_ANSWER;
>       }
>       break;
> 
>     case BUS_WAITING_ANSWER:
>       if (TimerExpired(&bus.tmr_answer)) {
>         /* No reply */
>         bus.state = BUS_IDLE;
>         TimerSet(&bus.tmr_answer, 0);
>       } else {
>         if (reply_received() == true)    {
>           /* Analyze the reply */
>           bus.state = BUS_IDLE;
>           TimerSet(&bus.tmr_answer, 0);
>         }
>       }
>       break;
>   }
>   return 0;
> }
> ...
> --- End of bus.c ---
> 
> I don't think I need to explain the code in bus.c. The only thing to
> specify is that bus_task() is called continuously in the main loop.
> 
> 99% of the time this code works well. Unfortunately I have seen some
> strange events. Rarely (very rarely, one time in a week) the bus seems
> frozen for a time. After that it restarts the normal activity magically.
> There's a thing that relates those strange events to driver of timers:
> the bus stall time lasts exactly 1h 21', the overflow period of hw counter.

It sounds like you are missing a tick interrupt - maybe you are somehow
blocking the overflow interrupt on occasion.  Maybe the CTRLBSET line is
the problem (since I don't know what it does!)  But it could be the
problem I noted above in ticks().

Diagnosing problems like this are a real pain - and demonstrating that
you have fixed them is even worse.  The key is to find some way to speed
up the hardware timer so that you can provoke the problem regularly -
then you can be sure when you have fixed it.  Are you able to make this
timer overflow at a lower bit count - say, 12 bits rather than 32 bits?
 If not, then try this for your hardware interrupt function:

void TC0_Handler(void) {
	if (TC0->COUNT32.INTFLAG.reg & TC_INTFLAG_OVF) {
		++_ticks_high;
		TC0->COUNT32.INTFLAG.reg = TC_INTFLAG_OVF;
		TC0->COUNT32.COUNT.reg = 0xfffff000;
	}
}

That will force overflows and _ticks_high counts to run much faster.


> 
> I suspect there's a problem in my low-level driver and sometimes, maybe
> near the overflow, the code doesn't work as I expect.
> Maybe the TimerSet() function sometimes sets a wrong value to the
> uint64_t timer, maybe a tick value that will happen only at the next
> overflow of the hw counter.
> 
> Do you see where is the problem?
> 

On 27.4.2017 г. 01:55, pozz wrote:
> I don't know if it is an issue specific to Atmel SAMC21 Cortex-M0+
> devices or not.
>
> I wrote a simple timer driver: a 32-bits hw counter clocked at 875kHz
> (14MHz/16) that triggers an interrupt on overflow (every 1h 21').  In
> the interrupt I increment the 32-bits global variable _ticks_high.
> The 64-bits number composed by _ticks_high (upper 32-bits) and the hw
> counter (lower 32-bits) is my system tick.  This 64-bits software
> counter, incremented at 875kHz, will never overflow during my life, so
> it's good :-)
>
> In timer.h I have:
> --- Start of timer.h ---
> #include <stdint.h>
> #include <io.h>  // Atmel specific
>
> #define TIMER_FREQ    875000
>
> typedef uint64_t Timer;
> extern uint32_t _ticks_high;
>
> #define volatileAccess(v) *((volatile typeof((v)) *) &(v))
>
> static inline uint64_t ticks(void) {
>    uint32_t h1 = volatileAccess(_ticks_high);
>    TC0->COUNT32.CTRLBSET.reg =
>         TC_CTRLBSET_CMD(TC_CTRLBSET_CMD_READSYNC_Val);
>    uint32_t l1 = TC0->COUNT32.COUNT.reg;
>    uint32_t h2 = volatileAccess(_ticks_high);
>    if (h2 != h1) return ((uint64_t)h2 << 32) + 0;
>    else          return ((uint64_t)h1 << 32) + l1;
> }
>
> static inline void TimerSet(Timer *tmr, uint64_t delay) {
>    /* delay is in ms */
>    *tmr = ticks() + delay * TIMER_FREQ / 1000;
> }
>
> static inline int TimerExpired(Timer *tmr) {
>      return ticks() >= *tmr;
> }
> --- End of timer.h ---
>
> In timer.c I have the ISR:
> --- Start of timer.c ---
> ...
> void TC0_Handler(void) {
>    if (TC0->COUNT32.INTFLAG.reg & TC_INTFLAG_OVF) {
>      ++_ticks_high;
>      TC0->COUNT32.INTFLAG.reg = TC_INTFLAG_OVF;
>    }
> }
> ...
> --- End of timer.c ---
>
> The idea is simple (and stolen from a post appeared on this newsgroup).
> At first, the 64-bits software counter must be calculated disabling
> interrupts, because if a timer interrupt triggers during calculation,
> the overall software counter could be wrong.
> By reading _ticks_high before and after reading hw counter, we can avoid
> to disable the interrupts.
>
>
> Now I have a code that uses timers. It's a simple state-machine that
> manages communication over a bus.
> --- Start of bus.c ---
> ...
> int bus_task(void) {
>    switch(bus.state) {
>      case BUS_IDLE:
>        if (TimerExpired(&bus.tmr_answer)) {
>          /* Send new request on the bus */
>          ...
>          TimerSet(&bus.tmr_answer, timeout_answer);
>          bus.state = BUS_WAITING_ANSWER;
>        }
>        break;
>
>      case BUS_WAITING_ANSWER:
>        if (TimerExpired(&bus.tmr_answer)) {
>          /* No reply */
>          bus.state = BUS_IDLE;
>          TimerSet(&bus.tmr_answer, 0);
>        } else {
>          if (reply_received() == true)    {
>            /* Analyze the reply */
>            bus.state = BUS_IDLE;
>            TimerSet(&bus.tmr_answer, 0);
>          }
>        }
>        break;
>    }
>    return 0;
> }
> ...
> --- End of bus.c ---
>
> I don't think I need to explain the code in bus.c. The only thing to
> specify is that bus_task() is called continuously in the main loop.
>
> 99% of the time this code works well. Unfortunately I have seen some
> strange events. Rarely (very rarely, one time in a week) the bus seems
> frozen for a time. After that it restarts the normal activity magically.
> There's a thing that relates those strange events to driver of timers:
> the bus stall time lasts exactly 1h 21', the overflow period of hw counter.
>
> I suspect there's a problem in my low-level driver and sometimes, maybe
> near the overflow, the code doesn't work as I expect.
> Maybe the TimerSet() function sometimes sets a wrong value to the
> uint64_t timer, maybe a tick value that will happen only at the next
> overflow of the hw counter.
>
> Do you see where is the problem?
>

Hoping that the compiler will know how to fix this is not a very
efficient approach.
It is just a few lines of assembly, no matter how inconvenient the ARM
assembly may be it will still save you weeks (months) of blind trial
and error.
You just need to _know_ how this is done, typing blindly this or that
is unlikely to ever work.

Dimiter

------------------------------------------------------
Dimiter Popoff, TGI             http://www.tgi-sci.com
------------------------------------------------------
http://www.flickr.com/photos/didi_tgi/

On 27/04/17 09:37, pozz wrote:
> Il 27/04/2017 02:17, John Speth ha scritto:
>> First you need to make ticks_high volatile.  It doesn't make any sense
>> to me why you'd cast it volatile in one execution context but not the
>> other.  If it's shared between two execution contexts, the compiler
>> needs to know that.

"volatile" does not mean that a variable is shared between two execution
contexts - it is neither necessary nor sufficient to make such sharing work.

To get shared accesses right, you need to be sure of what is accessed at
what times - it is the /accesses/ to the variable that need to be
volatile.  Marking a variable "volatile" is simply a shortcut for saying
that /all/ accesses to it must be volatile accesses.

> 
> _ticks_high is accessed in TimerSet(), TimerExpired() and the ISR
> TC0_Handler().  Timerset() and TimerExpired() reads _ticks_high by
> calling ticks() function.  So _ticks_high is accessed only in two
> points: ticks() and TC0_Handler().
> 
> ticks() is called during normal background flow (not interrupt), so the
> access to _ticks_high must be volatile.
> TC0_Handler() is the ISR, so I think a volatile access to _ticks_high is
> not necessary (the ISR can't be interrupted).
> 

Yes, the volatile accesses here are fine.  Omitting the volatile for
_ticks_high does not give any benefits or disadvantages, because you are
only doing a single read and write of the variable anyway - there is
very little room for the compiler to optimise.  The compiler can move
the non-volatile accesses to _ticks_high to after the clearing of the
overflow flag, maybe saving a cycle or two if it is a Cortex M7 that can
benefit from more sophisticated instruction scheduling.  But otherwise,
it does not matter one way or the other.

> 
>> It sounds like you have a concurrent access problem that you need to
>> protect against.  If it was me, I'd disable interrupts instead of your
>> trick.
> 
> I liked this trick because avoids disabling interrupts. TimerExpired()
> is often called.
> 
> I found the post (from Wouter van Ooijen) where this trick is suggested:
> https://groups.google.com/d/msg/comp.arch.embedded/9d8I5FFbmX4/6yL33UR92F8J
> 
> Don Y suggested other improvements in the same thread.

Il giorno giovedì 27 aprile 2017 09:32:17 UTC+2, pozz ha scritto:

> As you can think, it's very difficult make a test and try, because I 
> have to wait for the rare event that could happen after a week.

change the timer clock so it overflows faster than 1.21h...

Bye Jack

On 27/04/17 10:01, David Brown wrote:
> On 27/04/17 00:55, pozz wrote:
>> I don't know if it is an issue specific to Atmel SAMC21 Cortex-M0+
>> devices or not.
>>
>> I wrote a simple timer driver: a 32-bits hw counter clocked at 875kHz
>> (14MHz/16) that triggers an interrupt on overflow (every 1h 21').  In
>> the interrupt I increment the 32-bits global variable _ticks_high.
>> The 64-bits number composed by _ticks_high (upper 32-bits) and the hw
>> counter (lower 32-bits) is my system tick.  This 64-bits software
>> counter, incremented at 875kHz, will never overflow during my life, so
>> it's good :-)
>>
>> In timer.h I have:
>> --- Start of timer.h ---
>> #include <stdint.h>
>> #include <io.h>  // Atmel specific
>>
>> #define TIMER_FREQ    875000
>>
>> typedef uint64_t Timer;
>> extern uint32_t _ticks_high;
> 
> Identifiers that begin with an underscore are reserved for file scope -
> you are not supposed to use them for "extern" variables.  I would be
> extremely surprised to find that a compiler treated them in any special
> way, but them's the rules (&#4294967295;7.1.3 in C11 N1570.)
> 
>>
>> #define volatileAccess(v) *((volatile typeof((v)) *) &(v))
> 
> That looks familiar :-)
> 
>>
>> static inline uint64_t ticks(void) {
>>   uint32_t h1 = volatileAccess(_ticks_high);
>>   TC0->COUNT32.CTRLBSET.reg =
>>        TC_CTRLBSET_CMD(TC_CTRLBSET_CMD_READSYNC_Val);
>>   uint32_t l1 = TC0->COUNT32.COUNT.reg;
>>   uint32_t h2 = volatileAccess(_ticks_high);
>>   if (h2 != h1) return ((uint64_t)h2 << 32) + 0;
>>   else          return ((uint64_t)h1 << 32) + l1;
>> }
> 
> (I don't know what the CTRLBSET stuff is for - I am not familiar with
> Atmel's chips here.)
> 
> If the low part of the counter rolls over and the interrupt has not run
> (maybe interrupts are disabled, or you are already in a higher priority
> interrupt, or interrupts take a few cycles to work through the system)
> then this will be wrong - l1 will have rolled over, but h2 will not show
> an updated value yet.
> 
> So if the high parts don't match, you need to re-read the low part and
> re-check the high parts.  It is perhaps easiest to express in a loop:
> 
> static inline uint64_t ticks(void) {
> 	uint32_t h1 = volatileAccess(_ticks_high);
> 	while (true) {
> 		uint32_t l1 = TC0->COUNT32.COUNT.reg;
> 		uint32_t h2 = volatileAccess(_ticks_high);
> 		if (h1 == h2) return ((uint64_t) h2 << 32) | l1;
> 		h1 = h2;
> 	}
> }
> 
> You can also reasonably note that this loop is not going to be re-run
> more than once, unless you have interrupt functions that last for an
> hour and a half, or something similarly bad - and then the failure of
> the ticks() function is the least of your problems!  Then you can write:
> 
> static inline uint64_t ticks(void) {
> 	uint32_t h1 = volatileAccess(_ticks_high);
> 	uint32_t l1 = TC0->COUNT32.COUNT.reg;
> 	uint32_t h2 = volatileAccess(_ticks_high);
> 	if (h1 != h2) l1 = TC0->COUNT32.COUNT.reg;
> 	return ((uint64_t) h2 << 32) | l1;
> }
> 

Looking over these again, I can see that they will have the same
potential problem.  They are fine for when the high part of the counter
is also in hardware - but /not/ if it is dependent on interrupts which
could be delayed.  If you have a situation like this:

_ticks_high is 1
timer reg is 0xffff'fff0
interrupts are disabled
timer reg rolls over, and is now 0x0000'0003
the ticks() function will return 0x0000'0001'0000'0003 instead of
0x0000'0002'0000'0003

You will need to check the hardware overflow flag to make this work.

static inline uint64_t ticks(void) {
	uint32_t h1 = volatileAccess(_ticks_high);
	uint32_t l1 = TC0->COUNT32.COUNT.reg;
	uint32_t h2 = volatileAccess(_ticks_high);
	if (h1 != h2) {
		// We just had an interrupt, so we know there is
		// no pending overflow
		l1 = TC0->COUNT32.COUNT.reg;
		// Or l1 = 0 for slightly faster code
		return ((uint64_t) h2 << 32) | l1;
	}
	if (TC0->COUNT32.INTFLAG.reg & TC_INTFLAG_OVF) {
		// There has been an overflow, and it was not handled
		l1 = TC0->COUNT32.COUNT.reg;
		return ((uint64_t) (h2 + 1) << 32) | l1;
	}
	// If there has been an overflow or an interrupt, it happened
	// after the first reads - so these are consistent and safe
	return ((uint64_t) h1 << 32) | l1;
}

If possible, change to the 32 bit version :-)  Failing that, if your
chip supports chaining of timers in hardware, use that.


Thanks for this thread, Pozz - it's a good question, makes people think,
and can hopefully help other developers.