Go Down

Topic: Atomic reading of uint16_t ? (Read 5494 times) previous topic - next topic

zapta

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?
Code: [Select]


volatile uint16 counter;   // incremented by ISR

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

PaulS

Quote
uint16_t reads are not atomic, right?

Correct.

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

There isn't ANY way, reasonable or not.
The art of getting good answers lies in asking good questions.

Coding Badly


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

Quote
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?

zapta


@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.


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


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.

Jiggy-Ninja

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?
Hackaday: https://hackaday.io/MarkRD
Advanced C++ Techniques: https://forum.arduino.cc/index.php?topic=493075.0

Coding Badly



@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.


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

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?

zapta


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.

zapta


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


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?

Coding Badly

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.

solderspot

#10
Jan 03, 2014, 11:31 am Last Edit: Jan 03, 2014, 11:52 am by solderspot Reason: 1
Can you make the counter in to two 8 bit values thus:

Code: [Select]

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?

odometer


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

Code: [Select]

  uint8 high = counterHigh;
  uint8  low = counterLow;

(snip)

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


Um... wrong. You need to cast [font=Courier]high[/font] before shifting it so far left.

zapta


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.


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?

Delta_G

He's really trying to help.  You're not listening to the advise  you're getting.  The problem is that to do what you want to do you will have to live with the possibility of some error.  Which error is better?  Is it worse to get a wrong value or to miss a read?  There are several possible solutions, but which is best depends entirely on what happens when it is wrong.  Maybe if you got the chip off your shoulder and helped those who are trying to help you then you will get your problem solved.  It's really hard to fix code when you don't know the specifics of what it has to do. 
|| | ||| | || | ||  ~Woodstock

Please do not PM with technical questions or comments.  Keep Arduino stuff out on the boards where it belongs.

MichaelMeissner

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.

Go Up