Hello all! I began to use Arduino little time ago and I am very much obliged to all it's developers for such convenient device!
I have one question and possibly suggestion:
In one my program used such code:
void setup()
{
pinMode(2, INPUT);
attachInterrupt(0, Handler, RISING);
}
loop()
{
cli();
//... some lines that need to be executed without interrupts
sei();
//other code
}
void Handler()
{
unsigned long newSyncTime = micros();
//......
}
the ISR Handler() executed by external interrupt (signal from sensor) and must use micros() func to syncronize all program. The possible trouble can occur because register TCNT0 of timer0 can overflow while executing code between cli() and sei() and micros() give result with error about millisecond. In it's code provided protection from such problem:
unsigned long micros() {
unsigned long m, t;
uint8_t oldSREG = SREG;
cli();
t = TCNT0;
#ifdef TIFR0
if ((TIFR0 & _BV(TOV0)) && (t == 0))
t = 256;
#else
if ((TIFR & _BV(TOV0)) && (t == 0))
t = 256;
#endif
m = timer0_overflow_count;
SREG = oldSREG;
return ((m << 8) + t) * (64 / clockCyclesPerMicrosecond());
}
so far as i understand the code "if ((TIFR0 & _BV(TOV0)) && (t == 0))" checks for overflow the counter after disabling interrupts - if set bit TOV0 in register TIFR0 than occurred overflow but I don't understand what for second condition "t == 0" ? I see possible problem with it - if while executing code between cli() and sei() TCNT0 will be incremented after overflow it will have value more than zero, and when will be enabled interrupts firstly will run Handler(). In Handler() code will be called micros() func but it can return wrong result because TCNT0 can have value more than zero although TOV0 bit will be set. I think this problem can be solved by changing code of micros():
unsigned long micros() {
unsigned long m, t;
uint8_t oldSREG = SREG;
cli();
t = TCNT0;
m = timer0_overflow_count;//Save value here to increment it if needed
#ifdef TIFR0
if (TIFR0 & _BV(TOV0))//If occured overflow while interrupts was disabled increment m by one as it would be if interrupts wasn't disabled
m++;
#else
if (TIFR & _BV(TOV0))
m++;
#endif
SREG = oldSREG;
return ((m << 8) + t) * (64 / clockCyclesPerMicrosecond());
}
I think this code will give more precise results if not disable interrupts for more than millisecond.
What gurus of this forum will say about it? I want to know your opinion.
PS sorry for my english :-[
Many people in the Arduino community operate at a much higher level of abstraction than your code uses. You might have better luck asking your question in a community that is probably closer to the hardware coding level, such as http://www.avrfreaks.net/.
I just recently looked at the source for micros, but did not spot the issue you pointed out - however I think you're absolutely right.
In the AtMega datasheet it is stated that the TOV0 bit of TIFR0 will automatically be cleared when the overflow interrupt is triggered. Meaning also that it will remain set until the interrupt is processed and irrespective of the value in TCNT0. In other words we should account for the overflow for as long as TOV0 is set and irrespective of TCNT0.
This is indeed a BUG and also I think your proposed change is correct. The same could also be achieved as follows:
unsigned long micros() {
unsigned long m, t;
uint8_t oldSREG = SREG;
cli();
t = TCNT0;
// check if overflow is pending
#ifdef TIFR0
if (TIFR0 & _BV(TOV0))
t+=256;
#else
if (TIFR & _BV(TOV0))
t+=256;
#endif
m = timer0_overflow_count;
SREG = oldSREG;
return ((m << 8) + t) * (64 / clockCyclesPerMicrosecond());
}
JetC spotted an issue with micros() when called from within an ISR. The error would be up towards 1024 micro seconds. In the proposed fix however we failed to account for the possibility that overflow may occur in between reading TCNT0 and checking for the overflow. E.g. as in the following:
overflow is false
t = TCNT0 // value is 255
overflow occurs
test for overflow is now true and we incorrectly add 256 to t
The original code avoided this scenario with the "(t == 0)" test and this would be ok unless micros() was called from within an ISR. The following fix however should take care of all the identified scenarios:
unsigned long micros() {
unsigned long m;
uint8_t oldSREG = SREG, t;
cli();
m = timer0_overflow_count;
t = TCNT0;
#ifdef TIFR0
if ((TIFR0 & _BV(TOV0)) && (t < 255))
m++;
#else
if ((TIFR & _BV(TOV0)) && (t < 255))
m++;
#endif
SREG = oldSREG;
return ((m << 8) + t) * (64 / clockCyclesPerMicrosecond());
}
In the proposed fix however we failed to account for the possibility that overflow may occur in between reading TCNT0 and checking for the overflow
Ah! I didn't think about that.
But if just to write so:
unsigned long micros() {
unsigned long m;
uint8_t oldSREG = SREG;
cli();
m = timer0_overflow_count;//Save value here to increment it if needed
#ifdef TIFR0
if (TIFR0 & _BV(TOV0))//If occured overflow while interrupts was disabled increment m by one as it would be if interrupts wasn't disabled
m++;
#else
if (TIFR & _BV(TOV0))
m++;
#endif
SREG = oldSREG;
return ((m << 8) + TCNT0) * (64 / clockCyclesPerMicrosecond());
}
