Atomic reading of uint16_t ?

Projects Programming
1

I have a periodic timer ISR that once in a while increments an error counter (up to 10 increments per ISR invocation). This is the only interrupt in my program, I disabled everything else to reduce interrupt jitter. There are at least 200 16Mhz clock cycles interval between successive interrupts. I would like to read this counter from the main program without disabling interrupts (again, to reduce interrupt jitter).

uint16_t reads are not atomic, right? If so, what is a reasonable way to do so without disabling interrupts? For example, is this safe and reasonable?

volatile uint16 counter;   // incremented by ISR

uint16 read_counter() {
  uint16 previous = counter;
  for(;;) {
    uint16 current = counter;
    if (current == previous) {
      return current;
    }
    previous = current;
  }
}
2

uint16_t reads are not atomic, right?

Correct.

If so, what is a reasonable way to do so without disabling interrupts?

There isn't ANY way, reasonable or not.

3

@zapta, what price do you pay if read_counter fails to return the correct value of counter?

I have a periodic timer ISR that once in a while increments an error counter...

What else writes to counter? Is it ever reset to zero?

4

The price will be that reasoning on the correctness of the program will be more difficult and error pone. The spec of the read_count() function for example will say 'may or may not return the correct value' and this uncertainty will propagate to the rest of the program, adding complexity which I want to avoid.

These are the accesses to the counter:

  1. Reset during initialization in setup(). I can wrap this one with interrupts off. No interrupt jitter issues during initialization.

  2. Timer 2 ISR may increment this a few times at most. The ISR is triggered at about 20Khz.

  3. Main program (that is, from loop()) reads the counter, not more than once per loop() iteration. This is the access I want to have with interrupts enabled. It will compute diff from previous reading so should handle counter overflow with no problem.

5

No matter how you try and do it, an atomic read cannot be done in less clock cycles than a normal read, since the ATmegas are only 8-bit processors. That means that, for a short period of time, access to the variable must be disabled so that only one thing can change it at once. This means that there is a period of time where the interrupt cannot access the variable, and must be disabled. This will cause interrupt jitter, and is unavoidable.

What are your trying to do that is so jitter-sensitive?

6

zapta:

[quote author=Coding Badly link=topic=208149.msg1530731#msg1530731 date=1388714295]
@zapta, what price do you pay if read_counter fails to return the correct value of counter?

The price will be that reasoning on the correctness of the program will be more difficult and error pone. The spec of the read_count() function for example will say 'may or may not return the correct value' and this uncertainty will propagate to the rest of the program, adding complexity which I want to avoid.[/quote]

Right. And when that uncertainty propagates to the rest of the program what happens? Wrong value displayed on an LCD? Thermonuclear reaction?

7

Does this counter really need to have more than 8 bits?

If this counter increases by no more than 10 each time through loop(), then why not just have the counter be 8 bits, and have loop() check for overflow?

8

odometer:
Does this counter really need to have more than 8 bits?

If this counter increases by no more than 10 each time through loop(), then why not just have the counter be 8 bits, and have loop() check for overflow?

I am trying to relax the timing requirements loop() max iteration time.

9

Much worse, I will never trust it.

How is the function in my original post above? Is it safe and correct? Am I missing something?

10

zapta:
Much worse, I will never trust it.

Twice I have tried to get you to answer a very basic question about your application. Twice you have answered, instead, about your state of mind. If you won't provide a reasonable answer then I have no choice but to give up. You will have to take PaulS's advice and disable interrupts or odometer's advice and use a byte-sized datatype.

11

Can you make the counter in to two 8 bit values thus:

volatile uint8 counterLow;  // incremented by ISR 
volatile uint8 counterHigh; // incremented by ISR

uint16 read_counter() {
  uint8 high = counterHigh;
  uint8  low = counterLow;

  if( high != counterHigh )
  {
    // wraparound happened so re-read values
    high = counterHigh;
    low = counterLow;
  }
  return (uint16)  ((high<<8) | low) ; 
}

You don't need a for loop either as we assume the low byte is not going to wraparound a second time while in this code.

Of course that is not necessarily true if you happen to have some other interrupt that takes a very long time.

How long does counterLow take to wrap round?

12

solderspot:
Can you make the counter in to two 8 bit values thus:

  uint8 high = counterHigh;

uint8  low = counterLow;

(snip)

return (uint16)  ((high<<8) | low) ;

Um... wrong. You need to cast high before shifting it so far left.

13

Badly, with all the respect, you are avoiding answering my original question, how to transfer correctly a counter value from the ISR to main, without disabling interrupts. I even posted a proposed code snippet and asked for critique.

Instead you keep asking me to analyze what will happen in any future program that will use the library I am writing if this API function will return incorrect values, That's a different topic. My question is about how to have correct code.

Any comment on the code snippet I posted? Is there any hole I am missing?

15

Or if you really need atomic 16-bit support, go up to an ARM processor like the Teensy 3.0/3.1, Arduino Due, DigiX, etc. The AVR chips only process data 8-bits at a time.

16

Um... wrong. You need to cast high before shifting it so far left.

Actually the code works as is. Most processors are not internally 8 bit and compilers will, by default, upcast to the native word size for speed reasons. But to be pedantic then yes we should cast first, or we could define high and low as uint16 and handle casting in the comparison operators.

17

Delta_G:
... The problem is that to do what you want to do you will have to live with the possibility of some error.

Will the code snippet I asked about in my original post give any error?

If so, can you explain the scenario that will cause an error?

Delta_G:
It's really hard to fix code when you don't know the specifics of what it has to do.

I think the original post gives all the information. It has to read a 16 bit value that is updated by an ISR, without disabling interrupts.

18

solderspot:
Can you make the counter in to two 8 bit values thus:

There is at least one fatal flaw in the code.

19

There is at least one fatal flaw in the code.

And that is...?

20

zapta:
Badly, with all the respect, you are avoiding answering my original question...

Bullshit...

uint16_t reads are not atomic, right?

PaulS gave you the correct direct answer. There is nothing more I can offer.

If so, what is a reasonable way to do so without disabling interrupts?

"Reasonable" is a subjective term. What is reasonable to PaulS may be different than what is reasonable to me which may be different than what is reasonable to you. The only way to answer that question is for you to define "reasonable". Something I was trying to help you do.

For example, is this safe and reasonable?

At this point the answer is clearly "no".

21

zapta:
Instead you keep asking me to analyze what will happen in any future program...

Do I? I thought my questions were about the application you described in your first post.