Bug in micros()?

I just had a look at the micros() Funktion of arduino-0022 in file wiring.c and I suppose there is an error in the atomic read sequence.

unsigned long micros() {
	unsigned long m;
	uint8_t oldSREG = SREG, t;
	
	cli();
	m = timer0_overflow_count;
#if defined(TCNT0)
	t = TCNT0;
#elif defined(TCNT0L)
	t = TCNT0L;
#else
	#error TIMER 0 not defined
#endif

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

Timer0 is configured to Fast PWM Mode. In this mode the Timer/Counter Overflow Flag (TOV0) is set each time the counter reaches 255.
Assume that TCNT0 is 254, so that t is assigned 254. Also assume that with the next cycle TCNT0 is incremented to 255 and the TOV0 flag is set. This means that (TIFR0 && _BV(TOV0)) is not zero, therefore m will be incremented => wrong result.
Either "t < 255" has to be replaced by "t < 254" or TIFR0 has to be read before reading TCNT0.

Note: This is just a theoretical case. I don't know the exact timing behaviour of AVR timer overflow interrupt generation.

EDIT: After having a second look at this topic, I assume that it is a mistake to use Fast PWM Mode instead of Normal Mode. In Fast PWM Mode the case should be handled that TCNT0 is still 255 after TOV0 ISR was serviced (i.e. an m-- would be needed in micros() for that case). But in Normal Mode everything should work fine.

The current implementation is based on the assumption that TOV0 is set when TCNT0 change from 255 to zero. For fast PWM mode (default configuration as set in init()) this appears not to be the case and so I think you're right.

Here's a go at a fix that should address both of your issues when in fast PWM mode:

 unsigned long micros() {
	unsigned long m;
	uint8_t oldSREG = SREG, t;
	
	cli();
	m = timer0_overflow_count;
#if defined(TCNT0)
	t = TCNT0;
#elif defined(TCNT0L)
	t = TCNT0L;
#else
	#error TIMER 0 not defined
#endif
        t++;

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

Would you agree?

I agree. (But to make it clear: this code is only applicable for Fast PWM Mode!)

I prefer using Normal Mode instead.

BenF:
The current implementation is based on the assumption that TOV0 is set when TCNT0 change from 255 to zero. For fast PWM mode (default configuration as set in init()) this appears not to be the case and so I think you're right.

I ran some additional tests and it appears that the current implementation is correct as is and no patch is required for fast PWM mode (Arduino default for timer0).

Timer overflow is set at 255 when in PWM mode 3, but at the trailing edge of the timer pulse (see AtMega328 datasheet figure 15-6). This coincides with TCNT0 incrementing from 255 to zero and so TCNT0 will be zero when TOVF0 is set (not 255 as suggested by matthias_g). Tests confirmed this behavior and also revealed issues with delay when patched as suggested above.

In short - what we have in 022 is good as is.

I also ran some tests now (ATmega32U4): External clock source on T0 pin. Clock on rising edge. T0 used as output, toggled via debugger. Result is, that in fast PWM mode TOV0 is set to 1 when TCNT0 changes from 255 to 0, on the rising edge of the clock.
This means arduino-022 is correct whereas datasheet statement "The Timer/Counter Overflow Flag (TOV0) is set each time the counter reaches TOP." is wrong. TOV0 is set when the counter reaches 0. Thanks for support.