Important FIX to core library - millis(), micros(), digitalWrite(), pinMode()

This is very importante !

In wiring lib, usualy a global interrupt disabled is issued when not realy needed.
It’s very bad for timed interrupt sensitive designs. Eg.: energy meter projects like openenergymonitor.org.

The global interrupt disable in millis() and micros() functions (also on digitalWrite() and pinMode()) on arduino core library causes erratic occurrence of other interrupts that really dont impact the primary reason it’s disabled in this functions.
Disabling global interrupts can lead to delaying some important interrupts (other timers, pins, adc, etc) used by other libs or part of the code.
Since the objective is to disable timer0 interrupt on overflow, i highly recomend to change code so that only this specific interrupt is disabled.
Doing the way i show bellow also frees 1byte of program code per function changed :

unsigned long millis()
{
unsigned long m;
- uint8_t oldSREG = SREG;
// disable interrupts while we read timer0_millis or we might get an
// inconsistent value (e.g. in the middle of a write to timer0_millis)
- cli();

  • TIMSK0 &= ~(1 << TOIE0); // Clear Overflow Interrupt Enable
    m = timer0_millis;
    - SREG = oldSREG;
  • TIMSK0 |= 1 << TOIE0; // Sets Overflow Interrupt Enable
    return m;
    }

unsigned long micros() {
unsigned long m;
- uint8_t oldSREG = SREG, t;

  • uint8_t t;
    - cli();
  • TIMSK0 &= ~(1 << TOIE0); // Clear Overflow Interrupt Enable
    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;

  • TIMSK0 |= 1 << TOIE0; // Sets Overflow Interrupt Enable

return ((m << 8) + t) * (64 / clockCyclesPerMicrosecond());
}

Unfortunately, your patch also makes it not safe to call if you are multitasking. I'll keep the erratic behavior. ;-)

ajk_:
Unfortunately, your patch also makes it not safe to call if you are multitasking.

How so?

@chaveiro, thank you for the post. Please do not cross-post.

[quote author=Coding Badly link=topic=168586.msg1257582#msg1257582 date=1369813311]

ajk_: Unfortunately, your patch also makes it not safe to call if you are multitasking.

How so?

[/quote]

due to you blindly switching the interrupt flag back on....

you should take a copy, clear the Timer 0 overflow enabled flag ( if needed ), and then afterwards restore the copy to remain multi tasking friendly.

imagine you had two tasks running, and they are switched by lets say timer 2, between turning off and back on, if the second task completes before its turned the overflow enable ISR back on.... but the first task hasnt finished, when the multi task returns to it, its data would/could/will be corrupted ( ie. the millis micros count )

darryl: imagine you had two tasks running

Why would I imagine that? The Arduino core does not support multitasking.

I can imagine these functions being used in an interrupt routine. That's why the existing functions save and restore SREG. This would cause the interrupt enable bit to be set on return from interrupt.

There are now many people using RTOS libraries and these users also call these functions in tasks so it could happen if two tasks both called micros() near a context switch.

So is this really an "important FIX’ or not?

Lefty

chaveiro: It's very bad for timed interrupt sensitive designs. Eg.: energy meter projects like openenergymonitor.org.

How is it bad?

So is this really an "important FIX’ or not?

It’s not a fix since it doesn’t work correctly when the functions are called in an interrupt routine and in non-interrupt code.

The fix won’t work if an interrupt occurs between

TIMSK0 &= ~(1 << TOIE0); // Clear Overflow Interrupt Enable

and

TIMSK0 |= 1 << TOIE0; // Sets Overflow Interrupt Enable

and the interrupt function calls one of the “fixed functions”.

The proposed fix for micros() will not work because the code relies on certain instructions being executed consecutively:

//#defines removed
   t = TCNT0;
   if ((TIFR0 & _BV(TOV0)) && (t < 255))      m++;

If an interrupt was allowed to occur between these lines, there could be a delay between reading TCNT0 and TIFR0 and m could be incremented erroneously causing an error of 1000us. Therefore it is necessary to disable all interrupts here, not just the timer.

As for timed interrupt sensitivity the jitter here will be small compared to the jitter caused by the millis timer interrupt itself and interrupts can be up to 3 ticks late anyway, you would be better dejittering the interrupt than altering micros.

It's not a fix

I have to argue with the "very important" part as well.

It's very bad for timed interrupt sensitive designs

It's difficult for me to believe that the original code is leaving interrupts turned off for long enough to have much effect on anything but the most poorly written code (or code that is really so extremely sensitive that using ANY external libraries will break it.) I count about 15 instructions in micros() and only 4 in millis() - that's much less latency introduced for other interrupts than would occur due to servicing either a uart or timer0 (clocktick) interrupt, for example...

Is there an actual example in the openenergymonitor.org code that is being blamed on this?

darryl: [quote author=Coding Badly link=topic=168586.msg1257582#msg1257582 date=1369813311]

ajk_: Unfortunately, your patch also makes it not safe to call if you are multitasking.

How so?

due to you blindly switching the interrupt flag back on....

you should take a copy, clear the Timer 0 overflow enabled flag ( if needed ), and then afterwards restore the copy to remain multi tasking friendly.

imagine you had two tasks running, and they are switched by lets say timer 2, between turning off and back on, if the second task completes before its turned the overflow enable ISR back on.... but the first task hasnt finished, when the multi task returns to it, its data would/could/will be corrupted ( ie. the millis micros count )

[/quote]

Exactly. If you want to make it still safe without globally disabling ISRs then use a semaphore, or other lock, and spin until free to go on.

My guess is that the cli here is causing troubles with some other code that you wrote and is causing jitter.

The proper thing to do in this case is to cli(); your time sensitive section of code; sei();

westfw: Is there an actual example in the openenergymonitor.org code that is being blamed on this?

Hi The history that relates to this issue discovery was the creation of an energy metering lib that relies on precise timing on timer1 ints : http://openenergymonitor.org/emon/node/2406 Imagine you are implementing a frequency counter, or soft PLL or some sort of time sensitive code that uses TIMER1 interrupt. If your interrupt in TIMER1 occours frequently and you use millis() on main code frequently also, you will get many delayed TIMER1 ints (due to being globally disabled in millis) that affects time sensitive calculations.

Who wrote millis() or micros() disabled interrupts with one pourpouse that is:

-Prevent timer0_millis and timer0_overflow_count vars from being written on timer0 overflow in the middle of the read of that vars.

But did it the lazy way that is to disable global int. That is bad programming and must be avoided for the reasons being said.

I'm almost surprised no one never discovered this issue. But maybe few arduino people get out to AvrStudio and simulate there sketches interrupt timings.

I assume you are referring to the timing of the TIMER1_COMPA_vect interrupt service routine. In which case the correct solution is to use the analog-to-digital converter auto-trigger.

This...

Serial.println("$");

...in an interrupt service routine is a very bad idea.

Still not seeing it. The time that millis() or micros() turns off interrupts is MUCH smaller than the time interrupts are disabled by the occurrence of any interrupt service routine.

My comments are essentially identical to what "dBC" said on your link:

Surely there'd have to be two interrupts from the same source during the disabled window before you'd lose anything? Otherwise the interrupt just gets delayed until they're globally re-enabled. Looking at millis() for example, it disables them for about 9 cycles, or 562.5 nsecs at 16MHz. So worst case, if an interrupt fired just as they were disabled, that event would get delayed by 562.5 nsecs. Is the PLL code really that sensitive that it can't cope with 1/2 usec of jitter in the timer interrupt?

Calling millis() or micros() "very frequently" in the main loop would be an example of the "badly coded" comment I threw out. Although, it's not clear that there is a great alternative. I think you'd be better off asking for a delay() or whatever that doesn't call millis/micros. You haven't found a bug, you've found a requirement of your code that the arduino environment doesn't meet. Since people who should know seem to define a "hard real time" OS as one with less than 100us of jitter in interrupt response time (http://www.lisha.ufsc.br/wso/wso2009/papers/st04_03.pdf ), even on cpus that are much faster than the AVR, you're likely to have a hard time finding anything better.

Coding Badly is right, the only way to insure low noise due to ADC sample time jitter is to auto trigger the ADC with Timer/Counter1 Compare Match B.

At the rate chaveiro is sampling, 1600 samples per second, sub microsecond jitter is needed to avoid jitter noise in the AVR 10 bit ADC. The jitter SNR must be greater than 62 dB to get 10 bit ADC accuracy.

The formula for ADC jitter noise is

SNR = 20*log(1/(2*pi*f*t))

where f is the sample frequency and t is the jitter time.

with 0.1 usec jitter the SNR is about 60, just about good enough.

SNR = 20*log10(1/(6.28*1600*1e-7)) = 59.96

Here is a calculator for ADC jitter http://www.maximintegrated.com/design/tools/calculators/general-engineering/jitter.cfm

It gives 9.71e-8 seconds max jitter for full accuracy of a 10-bit ADC at 1600 samples per second.