4
\$\begingroup\$

Initialization of timer 1, the 16 bit timer on the ATmega328:

TCCR1A = 0; // normal operation
TCCR1B = bit(CS10); // no prescaling
OCR1A = 0;
OCR1B = 0;
TIMSK1 |= bit(TOIE1); // Timer/Counter 1, Overflow Interrupt Enable

16 bit overflows increment an overflow counter:

ISR(TIMER1_OVF_vect) {
    timer1OverflowCount ++;
}

In a loop it is checked whether the two 16 bit counters increment correctly:

void loop() {
  static uint16_t lastOc = 0, lastC = 0;
  uint16_t oc, c;

  ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
    oc = timer1OverflowCount;
    c = TCNT1;
  }

  if (c < lastC && oc == lastOc) {
    print(oc, c, lastOc, lastC);
  }

  lastOc = oc;
  lastC = c;
}

Sample output to the serial console:

Bad overflow: oc = 31, c = 49, lastOc = 31, lastC = 65440
Bad overflow: oc = 58, c = 49, lastOc = 58, lastC = 65440
Bad overflow: oc = 66, c = 49, lastOc = 66, lastC = 65440
Bad overflow: oc = 118, c = 49, lastOc = 118, lastC = 65440
Bad overflow: oc = 127, c = 49, lastOc = 127, lastC = 65440

Why does the overflow counter sometimes not get incremented?

I understand that the loop accesses the overflow counter a lot. During an access, interrupts are turned of with ATOMIC_BLOCK(ATOMIC_RESTORESTATE). However, the access is fast, and I expect the overflow interrupt to get queued so that it is never missed.

Full code for the Arduino Pro Mini ATmega328P (5V, 16MHz), compatible with the Arduino IDE 1.8.1:

#include <util/atomic.h>

volatile uint16_t timer1OverflowCount = 0;

ISR(TIMER1_OVF_vect) {
  timer1OverflowCount ++;
}

void setup() {
  TCCR1A = 0; // normal operation
  TCCR1B = bit(CS10); // no prescaling
  OCR1A = 0;
  OCR1B = 0;
  TIMSK1 |= bit(TOIE1); // Timer/Counter 1, Overflow Interrupt Enable

  Serial.begin(9600);
}

void print(uint16_t oc, uint16_t c, uint16_t lastOc, uint16_t lastC) {
  Serial.print("Bad overflow: ");
  Serial.print("oc = ");
  Serial.print(oc);
  Serial.print(", c = ");
  Serial.print(c);
  Serial.print(", lastOc = ");
  Serial.print(lastOc);
  Serial.print(", lastC = ");
  Serial.println(lastC);
}

void loop() {
  static uint16_t lastOc = 0, lastC = 0;
  uint16_t oc, c;

  ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
    oc = timer1OverflowCount;
    c = TCNT1;
  }

  if (c < lastC && oc == lastOc) {
    print(oc, c, lastOc, lastC);
  }

  lastOc = oc;
  lastC = c;
}
\$\endgroup\$
  • \$\begingroup\$ What assembly code is emitted by the ATOMIC_BLOCK(..){..} statement? \$\endgroup\$ – Spehro Pefhany May 4 '18 at 9:17
  • 1
    \$\begingroup\$ @MITURAJ Look at the example output. oc = 31, c = 49, lastOc = 31, lastC = 65440 means that TCNT1 did overflow (65440→49) but the overflow counter stayed the same (31). \$\endgroup\$ – feklee May 4 '18 at 9:38
  • 1
    \$\begingroup\$ An asm listing after replacing the print call with some NOP-s would be interesting to see the timing. \$\endgroup\$ – Dorian May 4 '18 at 11:25
  • 1
    \$\begingroup\$ Couple of things to try: toggle a pin in the ISR, toggle a pin in when you read TCNT1, bring out the hardware overflow signal on a pin, and connect all of that to an oscilloscope or logic analyzer. That should give you a decent picture of what's happening internally. Also, grab the interrupt flag at the end of your atomic block and print that out to see if an interrupt was requested inside the atomic block, as that is probably the case here. \$\endgroup\$ – alex.forencich May 4 '18 at 12:38
  • 2
    \$\begingroup\$ @Dorian Yes, but it doesn't work. \$\endgroup\$ – feklee May 6 '18 at 9:32
1
\$\begingroup\$

Unless interrupts are disabled all the way from the time a value is read until the time it is used, one should generally design around the idea that an attempt to read a value from something that is changing may yield any value that thing held at any time during the attempt, without worrying about precisely when it is sampled.

A simple general-purpose way of handling a 32-bit read is to start by reading a value twice into n1 and n2 (read the bytes in any order, provided that the last read for n1 precedes the first read for n2) and don't worry about disabling interrupts), and then saying:

if ((uint8_t)((n1 >> 24) ^ (n2 >> 24)) // MSB has changed
  n2 &= 0xFF000000;
else if ((uint8_t)((n1 >> 16) ^ (n2 >> 16))
  n2 &= 0xFFFF0000;
else if ((uint8_t)((n1 >> 8) ^ (n2 >> 8))
  n2 &= 0xFFFFFF00;

If the upper byte of the timer changes from xx to yy, that implies that when the upper byte was xx, the timer's value was at most xxFFFFFF, and when the value was yy, the timer's value was at least yy000000. Thus, at some time between when xx was read and yy was read, its value must have been yy000000. Similar logic applies to the other bytes of the timer.

Note that the amount of time required to run this code will be bounded, even in the presence of heavy interrupt loading, provided only that the loading is less than 100%. Note that the code makes no attempt to distinguish between e.g. the case where the timer was 0x01FF0000 before the code and 0x02000000 after [having spent 65,000+ cycles in interrupts] or 0x01FFFFFF before and 0x02010000 after. It will report 0x02000000 in either case, but that would represent a value the timer held at some point during the execution of that code.

\$\endgroup\$
  • \$\begingroup\$ Neat trick, and independent of the MCU used! I successfully tested it with the implementation that I added to my answer. \$\endgroup\$ – feklee May 8 '18 at 18:30
8
\$\begingroup\$

The problem is that you are not eliminating the actual issue with the ATOMIC statement- the counter is incrementing in hardware and there will be times when an interrupt is pending but not completed. It's worse because the C int operations take a lot of cycles on an 8-bit processor, but it would show up even in tight asm code.

If you don't have any other interrupts of much duration going on you don't even have to kill interrupts, you can simply correct the count.

The general approach is as follows:

  1. read timer1OverflowCount, first timer1OverflowCount sample
  2. read the hardware counter TCNT1 <-- right here is when you are sampling with high resolution
  3. read timer1OverflowCount again, second timer1OverflowCount sample

If timer1OverflowCount has changed (incremented), then look at the sample from TCNT1. If it has MSB = 0 then use the later timer1OverflowCount. If it has MSB=1 then use the earlier timer1OverflowCount.

There is no need to turn interrupts off unless there are other interrupts with slow ISRs going on that could cause the total time for the above to approach the time for the hardware counter to reach 1/2 of full count. It's almost always undesirable to turn off interrupts unless absolutely necessary.

Edit: In your case you have an int for the overflow so you need to make the read of the variable changed within the ISR atomic, as well as dealing with the issue above.

\$\endgroup\$
  • \$\begingroup\$ This seems to do the trick, thanks! But shouldn't I disable interrupts when reading timer1OverflowCount? I assume it is possible that timer1OverflowCount is incremented right in the middle of reading it, i.e. right between several LDS instructions. \$\endgroup\$ – feklee May 4 '18 at 13:20
  • 1
    \$\begingroup\$ Because it's not necessary (the reading will be corrected in any case). Turning off interrupts globally can increase latency to higher priority interrupts in processors that support such; it's just bad form if you don't need to. \$\endgroup\$ – Spehro Pefhany May 4 '18 at 13:28
  • \$\begingroup\$ Example: (a) timer1OverflowCount is a 16 bit value of: 11111111 00000000 (b) The first byte is read (LDS): 11111111 (c) The overflow interrupt increments 11111111 0000000000000000 00000001. (little endian) (d) The second byte is read (LDS): 00000001. The total value read is 0xb111111111 = 760495542545, instead of 0xb100000000 = 760209211392. This is wrong. Or am I wrong? I don’t see how your proposal corrects for this race condition. \$\endgroup\$ – feklee May 4 '18 at 13:54
  • \$\begingroup\$ Ah, okay yes you should in that case if it is a 16-bit variable on an 8-bit processor, you can also play a similar trick with double reads if you need to keep interrupts on. \$\endgroup\$ – Spehro Pefhany May 4 '18 at 19:36
5
\$\begingroup\$

The ATOMIC_BLOCK is executed without interruption but the instructions inside are not executed in the same time.

From "oc = timer1OverflowCount;" to "c = TCNT1;" TCNT1 is incremented and is not the same as when reading oc.

The error shows when the overflow occurs inside the ATOMIC block and timer1OverflowCount cannot be updated and even the interrupts were disabled TCNT1 will show a later value than timer1OverflowCount.

Switch the two instructions, place each in it's own ATOMIC block and see the result. It is a "false positive". I assumed you just that you just want to check that your TCNT1 overflow interrupt it's working well, It works but the code used is showing errors where they aren't.

At OP request I add the explanation for reading TCNT1 in it's own atomic block.

Reading the 16 bit TCNT1 register is safe regarding it's incrementation between two 8 bit LSB and MSB register readings, when reading LSB the MSB register is buffered and all further readings will show the same value at the moment when LSB was read.

But having an interrupt between the two readings that also reads TCNT1 will update the value of the buffered MSB with the new value that might be different.

I don't know if applies here but other 16bit register might share the same buffer as TCNT1.

You can watch overflow count for a fixed time to see if has any delay.

The code is fast but also executed without any break between loops.

In the loop you have more or less five instructions, two transfers in the atomic block, and three outside. Can be added a jump and enable disable interrupts. The chance for this overflow to occur while executing the atomic block and to raise the error is pretty high.

So let's say the processor spend 2us for the ATOMIC block and 4us for the rest of the loop then again 2us in the ATOMIC block and again 4 outside.

If the overflow occurs while in the ATOMIC block (which is 1/3 chance) while the interrupts are disabled and timer1OverflowCount cannot be incremented you have a pretty good chance to raise a false error.

The chance is much lower because the compiler adds some code for the loop and also inside the ATOMIC block the overflow must occur before reading TCNT1 LSB.

This is the working code posted by OP in the comment:

ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { c = TCNT1; } 
ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { oc = timer1OverflowCount; }

I would like to add that this code is working using the conditions from the IF section

if (c < lastC && oc == lastOc)

But if you check

if (c >= lastC && oc != lastOc)

meaning changing overflow count without an overflow yo will also have false errors.

In this case, if it isn't just a code check then you should use something like Spehro's answer reading the TCNT1 again ( also in it's ATOMIC block) and use the lowest value of TCNT1 in case of timer count change.

But if you need to do something when timer count is updating then check only the timer count (thread safe). It's working well.

\$\endgroup\$
  • \$\begingroup\$ Thanks, this works: ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { c = TCNT1; } ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { oc = timer1OverflowCount; } One may wonder: Why is it necessary to place c = TCNT1; into its own atomic block? After all, the 16 bit read access to TCNT1 happens with the help of the TEMP register. See datasheet version DS40001984A, pages 154 and 155. Interestingly, it is explicitly stated: “The following code examples show how to access the 16-bit timer registers assuming that no interrupts updates the temporary register” Perhaps ISR(TIMER1_OVF_vect) fires during the read of TCNT1. \$\endgroup\$ – feklee May 8 '18 at 8:20
  • \$\begingroup\$ Could you add these considerations to the answer, to make it more thorough? Also it would be nice if you explain what you mean by “false positive”. I don’t understand the second half of your answer. \$\endgroup\$ – feklee May 8 '18 at 8:21
3
\$\begingroup\$

Looking at the 'bad' printouts we see lastC = 65440 and c = 49, which indicates that 96 counter ticks occurred between each timer read in the main loop. An overflow interrupt should have occurred at 65536, which is about half way through that time.

But you are also reading timer1OverflowCount about half way through that time, so depending the exact timing of the interrupt it may or may not have occurred when you read the overflow count. When you read the timer the interrupt will have occurred and timer1OverflowCount will have incremented, but you are using the historical overflow count that may be before the timer overflowed.

To get a better idea of what is happening you could do another print out immediately after each 'bad' one, then you should see that timer1OverflowCount has 'mysteriously' incremented before the next interrupt!

To fix the problem, don't read timer1OverflowCount until c < lastC. Then you will know that an overflow interrupt should have just occurred (and the next one is ~65000 ticks away) so you can reliably test the overflow count.

\$\endgroup\$
  • \$\begingroup\$ Very simple solution, thanks! I added an implementation to my answer. \$\endgroup\$ – feklee May 8 '18 at 19:11
2
\$\begingroup\$

By combining bits and pieces from answers with information from the ATmega328 data sheet version DS40001984A I found the following solution, which I believe to be robust:

ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
  c = TCNT1; // TCNT1 increases all the time during the following instructions
  bool timerDidOverflow = TIFR1 & 1; // TOV1 Timer/Counter 1, Overflow Flag
  byte msb = c >> 15;
  if (msb == 0 && timerDidOverflow) {
    timer1OverflowCount ++;
    TIFR1 &= 1; // Write to TOV1 to clear it and prevent triggering TIMER1_OVF
  }
  oc = timer1OverflowCount;
}

The above lines replace:

ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
  oc = timer1OverflowCount;
  c = TCNT1;
}

As there has been concern about micro-optimization in the comments, here is the assembly code per output by avr-objdump:

   c = TCNT1; // TCNT1 increases all the time during the following instructions
606:    c0 91 84 00     lds    r28, 0x0084    ; 0x800084 <__stack+0x7ff785>
60a:    d0 91 85 00     lds    r29, 0x0085    ; 0x800085 <__stack+0x7ff786>
   bool timerDidOverflow = TIFR1 & 1; // TOV1 Timer/Counter 1, Overflow Flag
60e:    86 b3           in    r24, 0x16    ; 22
610:    81 70           andi    r24, 0x01    ; 1
   byte msb = c >> 15;
   if (msb == 0 && timerDidOverflow) {
612:    d7 fd           sbrc    r29, 7
614:    0e c0           rjmp    .+28         ; 0x632 <main+0x144>
616:    88 23           and    r24, r24
618:    61 f0           breq    .+24         ; 0x632 <main+0x144>
     timer1OverflowCount ++;
61a:    80 91 4c 01     lds    r24, 0x014C    ; 0x80014c <timer1OverflowCount>
61e:    90 91 4d 01     lds    r25, 0x014D    ; 0x80014d <timer1OverflowCount+0x1>
622:    01 96           adiw    r24, 0x01    ; 1
624:    90 93 4d 01     sts    0x014D, r25    ; 0x80014d <timer1OverflowCount+0x1>
628:    80 93 4c 01     sts    0x014C, r24    ; 0x80014c <timer1OverflowCount>
     TIFR1 &= 1; // Write to TOV1 to clear it and prevent triggering TIMER1_OVF
62c:    86 b3           in    r24, 0x16    ; 22
62e:    81 70           andi    r24, 0x01    ; 1
630:    86 bb           out    0x16, r24    ; 22
   }
   oc = timer1OverflowCount;
632:    e0 90 4c 01     lds    r14, 0x014C    ; 0x80014c <timer1OverflowCount>
636:    f0 90 4d 01     lds    r15, 0x014D    ; 0x80014d <timer1OverflowCount+0x1>

Side note: The 16 bit read access to TCNT1 happens with the help of the TEMP register. The access is guaranteed to yield a consistent value. See pages pages 154 and 155 in datasheet version DS40001984A. Without the threat of interrupts accessing TCNT1 it is not necessary to put read/write access to TCNT1 into an atomic block.

For comparison, here is an implementation of the solution from the answer by @supercat, which does not require atomicity during reading at all:

uint16_t c0 = TCNT1, oc0 = timer1OverflowCount;
uint16_t c = TCNT1, oc = timer1OverflowCount;

if ((oc0 >> 8) ^ (oc >> 8)) {
  oc &= 0xff00;
  c = 0;
} else if ((uint8_t)(oc0 ^ oc)) {
  c = 0;
} else if ((c0 >> 8) ^ (c >> 8)) {
  c &= 0xff00;
}

Terse is the solution by @Bruce Abbot, which I successfully tested with the following implementation:

static uint16_t oc = 0;
uint16_t c;
ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
  c = TCNT1;
}

bool timerDidOverflow = c < lastC;
if (timerDidOverflow) {
  ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
    oc = timer1OverflowCount;
  }
}
\$\endgroup\$
  • \$\begingroup\$ Voted as an option but actually the code is worse that way. The interrupt latency is much higher. For TCNT1 interrupt from "bool..." line to the end and for other interrupt all the block. For three small atomic blocks the latency is as high as the longest one. \$\endgroup\$ – Dorian May 8 '18 at 15:49
  • \$\begingroup\$ At least replace "msb = c >> 15" which is done in more than 32 steps or 7 if the compiler make some optimization ignoring the lower byte with "msb = c & 0x1000" or just skip it and replace "if(msb==0 &&..." with "if(c<0x1000 &&..." that takes less processing time. There is no real gain to use only one block. \$\endgroup\$ – Dorian May 8 '18 at 15:58
  • \$\begingroup\$ By the way, why do you buffer the overflow flag to since it doesn't matter when it's sampled? You can use "if ((c<0x1000)&&(TIFR1 & 1)){..." \$\endgroup\$ – Dorian May 8 '18 at 16:05
  • 1
    \$\begingroup\$ @feklee: That's true, but many compilers have optional warnings about using certain combinations of operators without parenthesis, and adding parentheses will both make the code code clearer and ensure that it will compile without complaints even when such options are enabled. \$\endgroup\$ – supercat May 8 '18 at 19:43
  • 1
    \$\begingroup\$ @feklee: Perhaps this particular case doesn't give a warning, but it seems most combinations of left/right shift with other operators do give warnings, and good code has the property that it is not only does what the programmer intends, but leaves a human reader with no doubt but that it will do so. Adding parentheses helps with the latter. \$\endgroup\$ – supercat May 9 '18 at 14:49
1
\$\begingroup\$

This is only my though. You are checking it occasionally not at a fixed interval.

It takes time to process the loop and send characters over UART ! Let's do a assumption:

You are using the baud rate of 9600, your Arduino sends about 57 characters each loop so the minmum time to finish each loop is :

               57 * 8 / 9600  =  47.5 ms  ( Rough approximation)

How much time does Timer1 overflow ? (Based on your setting)

               65440 / 16000000 ≈ 4 ms

You see, It cannot catch every single overflow.

You should modify your code and use another timer to created a fixed gating interval to count how much timer1OverflowCount in a fixed time.

I haven't analyze your code thoroughly as I'm exhausted now (and lazy too). This is probably software error rather than hardware defection.

\$\endgroup\$
  • \$\begingroup\$ He uses uart only after the missed udate is found so it doesn't count to loop time. \$\endgroup\$ – Dorian May 4 '18 at 10:03
  • \$\begingroup\$ As I said :" It cannot catch every single overflow. ". \$\endgroup\$ – Long Pham May 4 '18 at 10:05
  • \$\begingroup\$ This means that the issue is a bit worse. But even if the print instruction is not buffered he miss checking only one overflow after one error found. \$\endgroup\$ – Dorian May 4 '18 at 10:23
  • \$\begingroup\$ Sorry, I've edited. 47.5 ms not 6 ms. \$\endgroup\$ – Long Pham May 4 '18 at 10:36
  • \$\begingroup\$ Just noticed the error bauds vs bytes. Still your answer is missing the answer itself. It just shows that the issue is there but does not explain the cause. Didn't downvoted but it should be rather a comment. \$\endgroup\$ – Dorian May 4 '18 at 10:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.