How to read a 32-bits hw counter on 8-bits microcontroller

pozzugno schreef op 15-Oct-14 9:33 AM:

I think the reading of an 8-bit register on an 8-bit chip is always atomic.

To make ( h1 + 1 ) valid you must increment counter_high in the ISR by

For an 8-bit chip various optimizations could be made (which might require using assembler), for instance h1 == h2 needs to check only the lowest-but-one byte, and the counter_high could be handled as a 3-byte integer.

I am not a language lawyer, but substracting two uinit32_t where the result could be negative does not feel good to me.

When I use this method I don't think of using timers, but of handling moments in time. So I would write something like

time_t message_timeout = now() + 100 * ms; while( ... ){ . . . if( now() > message_timeout ){ . . . } }

Or, for a busy delay

void wait_busy( time_t duration ){ time_t end = now() + duration; while( now() < end ){} }

That would be the C version. I mostly work in C++, where I use an ADT for time_t, and I distinguish between absolute time (returned by now()) and relative time (constexpr ns, us, ms, s, etc).

But maybe I should thing in timers. That matches better with the way I use time in threads. I'll give it a try.

I used to work with PICs a lot, but afew years ago I switched to Cortex. It is a relieve not having to worry about 8-bit and 16-bit integers and the speed penalty of using 32-bits any more. And C++ is so much more fun than C.

Wouter van Ooijen

I have two points here. The first is simple - you should be adding

The second issue is about code efficiency - size and speed. Using "volatile" imposes restraints on the compiler, and should be used when necessary but /only/ when necessary. It is commonly the case that you need the "volatile" access when using a variable like counter_high from outside the ISR, but that "volatile" is unnecessary inside the ISR. In this case, without the "volatile" the compiler could skip the read and write of the lowest byte, and it could also do the addition with fewer registers which would mean less pushing and popping for the ISR. (I don't know if current avr-gcc does such optimisation, but it /could/ do them.) It will certainly help if you use counter_high for anything else in the same ISR.

So personally, I would write:

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

static uint32_t counter_high = 0; // No "volatile" Timer/Counter ISR: counter_high += 0x0100;

But when reading counter_high in read_counter, I would use: uint32_t h1 = volatileAccess(counter_high);

This also helps make it clear that it is /accesses/ that are volatile, not the variable itself. Keep counter_high "static" to avoid it being misused by code in other files.

uint32_t read_counter(void) { uint32_t h1 = volatileAccess(counter_high); uint8_t l = COUNTER_REGISTER; /* Atomic operation */ uint32_t h2 = volatileAccess(counter_high); return (h1 == h2) ? (h1 + l) : h2; }

You are using an AVR - don't use pointers unless you /really/ have to, as they are very inefficient. Even on a processor with good address registers, there is no point in using pointers here - it makes code less clear and may reduce optimisation flexibility. In particular, functions like these can often be inlined to give reduced code size (as well as faster code), and that works better without an extra pointer layer.

typedef uint32_t Timer; Timer TimerSet(uint32_t delay) { return read_counter() + delay; } bool TimerExpired(Timer tmr) { return ((int32_t) (read_counter() - tmr)) >= 0; }

Converting the time difference to a signed int lets you compare it to 0 instead of a fixed unsigned number. It is easier to write, easier to read, easier to modify if you change everything to 64-bit, and may give better code.

Yes, apart from your 0x10 / 0x0100 typo, your code was correct.

I am not a language lawyer either, but I listen to others on comp.lang.c

Overflow and wrapping is fully defined for unsigned arithmetic - so subtracting two uint32_t values will work as expected even if you subtract a large value from a small value. But you won't get a negative result - you will get an unsigned result greater than 0x7fff'ffff.

In my post, I suggested casting the result of the subtraction to a signed int, and checking for the sign of the result:

return ((int32_t) (read_counter() - tmr)) >= 0;

That actually has more issues, since the standard does not define what happens when an unsigned value greater than 0x7fff'ffff is converted into an int32_t - it is "implementation defined". I am confident that avr-gcc will do the expected thing here - but in theory a compiler could decide that the conversion gave the absolute value of the two's complement negative number. This would then let the compiler optimise that return statement to "return true;".

However, since such conversions are "implementation defined" the compiler has to be consistent, and anything other than the "obvious" behaviour would usually lead to worse code - so the compiler must /always/ do the "obvious" thing. If the conversion had been "undefined behaviour" then it would be a different matter - the compiler could behave differently at different times.

Il 15/10/2014 10:03, Wouter van Ooijen ha scritto:

I think it too.

Yes, of course. It was a typo.

Yes, here I'm more interested in the general aspect.

As David Brown wrote, it is well defined in C standard.

I think it's very similar to my approach. Your

time_t message_timeout = now() + 100 * ms;

is the same as my

Timer tmr; TimerSet(&tmr, 100 * ms);

And your

if (now() > message_timeout) {

is the same as my

if (TimerExpired(&tmr)) {

And your comparison doesn't care about wrap-around!!

It's the same as:

void wait_busy(Timer duration) { Timer end; TimerSet(&end, duration); while(!TimerExpired(&end){} }

I am exactly in this situation, except I started with AVRs (and only a few PICs).

How you are right.

Yes? Why?

Yes, it was a typo.

Good suggestion, thank you very much :-)

Yes, I know, but here I wanted to stress just the tecnique to access a

Anyway I would implement TimerSet and TimerExpired as inline functions (or macro), so the compiler is able to convert pointer arithmetic to direct assignment in most cases.

IMHO the API with pointers are more generic if, in the future, I'll change Timer to a complex structure.

As you observed in your other post, it brings to an "implementation defined" behaviour.

Prefer "static inline" functions to macros whenever possible.

Usually pointers can be converted to direct assignment when inlining, but not always - you are more likely to have situations where the data can "leak" to other modules (or that the compiler cannot easily prove that there is no leak), and thus limit optimisation.

That can sometimes be the case, but not here - the code must change if the Timer type changes.

As Wouter suggests, you can always change to C++ - then you could make your Timer's into a class and write generic code that works for timers of different sizes. It can be more fun - but don't forget that generalisation is wasted if you only ever need the one size.

Yes - but you have a fixed implementation, so implementation defined behaviour is fine if it does what you want. But you may need to consider such things in the future. (In this particular case, any sane compiler will have the same implementation-defined behaviour on sane processors. You only need to worry if you have one's complement arithmetic or weird sized integers.)

pozzugno schreef op 15-Oct-14 12:19 PM:

No need to, I work with 64 bit time_t only, and I won't be around when that wraps around.

Yes, it boils down to the same effect, but the it looks (and reads) different in the source. Which I do care about.

Better architecture, worse manufacturer. But you do have GCC! (But I would still drop those 8-bitter and switch to Cortex if I were you.)

Better abstraction mechanisms. For one thing, try to implement the absolute_time_t versus time_duration_t distinction (including operators) in C.

For some more fun:

Wouter

Op Wed, 15 Oct 2014 17:32:38 +0200 schreef Wouter van Ooijen :

Why be so fatalistic? The nanobots are coming!