Go Down

Topic: micros() function (Read 8094 times) previous topic - next topic

JetC

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:

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

JetC

Can anyone answer my question about "t == 0" condition? please

TBAr

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

JetC

Thanks for reply but still hope to get answer here because my question pertains to Arduino library

borref

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:

Code: [Select]

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

Ok, then solution for me is to change code in my local copy of wiring.c until this bug will be fixed "officially" (if it will). Thanks for reply!  :)

borref

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:

Code: [Select]

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:

Code: [Select]

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());
}



mellis


JetC

Quote
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:
Code: [Select]
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());
}


Really what for save TCNT0 in t now?  ;)

borref

Code: [Select]
Really what for save TCNT0 in t now?

If we consider the following scenario:

Code: [Select]

TCNT0 is at 255
Test for overflow is false
TCNT0 overflows to 0 in between the test and where we calculate elapsed microseconds


- again we would be off by close to 1024 micro seconds.

Agree?

JetC

Yes, agree - your proposed solution is best

Go Up