Suspected Bug - Micros or Interrupts

Hi,
The following sketch reproduces a bug whereby interrupts are reporting the time as measured using micros to be either 1,000 micros too soon or 1,000 micros too late.

The code is a lightly modified version of a pattern used man times to measure RC Channels on the AVR Platforms. I have stripped the sketch down, changed the data types and added some additional tracking variables to assist with debug.

The sketch implements a loop back where a servo signal is output on pin 8 and read back in on pin 2. Over a period of time, the signal read back in will alternate between periods of being correct, 1,000 micros too short and 1,000 micros too long, each of these conditions can be seen repeatedly for short periods while the sketch is running - sample output below -
The columns are

Pulse Width, start time in micros, end time in micros, pulse width, Time since last rising edge, time since last falling edge

1102	12823887	12824989	1102	20001	20002
1102	12843888	12844990	1102	20001	20001
1102	12863889	12864991	1102	20001	20001
1102	12883890	12884992	1102	20001	20001
1102	12903891	12904993	1102	20001	20001
1102	12923892	12924994	1102	20001	20001
1101	12943894	12944995	1101	20002	20001
1102	12963895	12964997	1102	20001	20002
1102	12983896	12984998	1102	20001	20001
1102	13003897	13004999	1102	20001	20001
102	13023898	13024000	102	20001	19001    // we lost 1,000 micros on the falling edge here ?
102	13043899	13044001	102	20001	20001
102	13063900	13064002	102	20001	20001
101	13083902	13084003	101	20002	20001
102	13103903	13104005	102	20001	20002
102	13123904	13124006	102	20001	20001
102	13143905	13144007	102	20001	20001
102	13163906	13164008	102	20001	20001
102	13183907	13184009	102	20001	20001
102	13203908	13204010	102	20001	20001
101	13223910	13224011	101	20002	20001
102	13243911	13244013	102	20001	20002
102	13263912	13264014	102	20001	20001
1102	13283913	13285015	1102	20001	21001 // we gained 1,000 micros on the rising edge here ?
1102	13303914	13305016	1102	20001	20001
1102	13323915	13325017	1102	20001	20001

This pattern continues with the measured pulse being reported as 100,1000 or 2000

Anyone seen something similar with interrupts or micros ?

Loop back test code -

//
// rcarduino.blogspot.com
//
// A simple approach for reading RC Channels using interrupts
//
// See related posts -
// http://rcarduino.blogspot.co.uk/2012/01/how-to-read-rc-receiver-with.html
// http://rcarduino.blogspot.co.uk/2012/03/need-more-interrupts-to-read-more.html
// http://rcarduino.blogspot.co.uk/2012/01/can-i-control-more-than-x-servos-with.html
//
// rcarduino.blogspot.com
// 

#include "Servo.h"

// Assign your channel in pins
#define THROTTLE_IN_PIN 2

// Assign your channel out pins
#define THROTTLE_OUT_PIN 8

// Servo objects generate the signals expected by Electronic Speed Controllers and Servos
// We will use the objects to output the signals we read in
// this example code provides a straight pass through of the signal with no custom processing
Servo servoThrottle;

// These bit flags are set in bUpdateFlagsShared to indicate which
// channels have new signals
#define THROTTLE_FLAG 1

// holds the update flags defined above
volatile uint32_t bUpdateFlagsShared;

// shared variables are updated by the ISR and read by loop.
// In loop we immediatley take local copies so that the ISR can keep ownership of the
// shared ones. To access these in loop
// we first turn interrupts off with noInterrupts
// we take a copy to use in loop and the turn interrupts back on
// as quickly as possible, this ensures that we are always able to receive new signals
volatile uint32_t ulThrottleStartShared;
volatile uint32_t ulThrottleEndShared;
volatile uint32_t ulThrottleInShared;

void setup()
{
  Serial.begin(115200);
 
  Serial.println("multiChannels");

  // attach servo objects, these will generate the correct
  // pulses for driving Electronic speed controllers, servos or other devices
  // designed to interface directly with RC Receivers 
  servoThrottle.attach(THROTTLE_OUT_PIN);

  // using the PinChangeInt library, attach the interrupts
  // used to read the channels
  attachInterrupt(THROTTLE_IN_PIN, calcThrottle,CHANGE);

  // for loop back test only, lets set each channel to a known value
  servoThrottle.writeMicroseconds(1500);
}

void loop()
{
  // create local variables to hold a local copies of the channel inputs
  // these are declared static so that thier values will be retained
  // between calls to loop.
  static uint32_t ulThrottleIn;
  static uint32_t ulThrottleStart;
  static uint32_t ulThrottleEnd;

  // local copy of update flags
  static uint32_t bUpdateFlags;

  // check shared update flags to see if any channels have a new signal
  if(bUpdateFlagsShared)
  {
    noInterrupts(); // turn interrupts off quickly while we take local copies of the shared variables

    // take a local copy of which channels were updated in case we need to use this in the rest of loop
    bUpdateFlags = bUpdateFlagsShared;
   
    // in the current code, the shared values are always populated
    // so we could copy them without testing the flags
    // however in the future this could change, so lets
    // only copy when the flags tell us we can.
   
    if(bUpdateFlags & THROTTLE_FLAG)
    {
      ulThrottleIn = ulThrottleInShared;
      ulThrottleStart = ulThrottleStartShared;
      ulThrottleEnd = ulThrottleEndShared;
    }

    bUpdateFlagsShared = 0;
   
    interrupts(); // we have local copies of the inputs, so now we can turn interrupts back on
    // as soon as interrupts are back on, we can no longer use the shared copies, the interrupt
    // service routines own these and could update them at any time. During the update, the
    // shared copies may contain junk. Luckily we have our local copies to work with :-)
  }
  
  // do any processing from here onwards
  // only use the local values unAuxIn, unThrottleIn and unSteeringIn, the shared
  // variables unAuxInShared, unThrottleInShared, unSteeringInShared are always owned by
  // the interrupt routines and should not be used in loop

  if(bUpdateFlags & THROTTLE_FLAG)
  {
    Serial.print(ulThrottleIn);
    Serial.print(",");
    Serial.print(ulThrottleStart);
    Serial.print(",");
    Serial.println(ulThrottleEnd);
  }
  bUpdateFlags = 0;
}


// simple interrupt service routine
void calcThrottle()
{
  // if the pin is high, its a rising edge of the signal pulse, so lets record its value
  if(digitalRead(THROTTLE_IN_PIN))
  {
    ulThrottleStartShared = micros();
  }
  else
  {
    // else it must be a falling edge, so lets get the time and subtract the time of the rising edge
    // this gives use the time between the rising and falling edges i.e. the pulse duration.
    ulThrottleEndShared = micros();
    ulThrottleInShared = ulThrottleEndShared - ulThrottleStartShared;
    // use set the throttle flag to indicate that a new throttle signal has been received
    bUpdateFlagsShared |= THROTTLE_FLAG;
  }
}

Duane B

rcarduino.blogspot.com

Thanks for the detailed test case, it really helps a lot - I meant to look at this particular bug a couple of months ago when others noticed it but didn't because there wasn't full code to reproduce the bug. Nevertheless it's still taken hours of hard work and I haven't completely fixed it, although I think I've improved things.

Basically there is a very complicated race condition going on. There are SysTick interrups every millisecond which micros() uses to calculate the thousands of microseconds in the result, and also your pin change interrupt. Bad things can happen if the SysTick interrupt triggers while you are in the middle of the pin change - it has to wait for your interrupt to finish before it can update the millis counter which messes up the timing.

Please try the following: In hardware/arduino/sam/cores/arduino/wiring.c change the function micros() to this:

uint32_t micros( void )
{
volatile    uint32_t ticks, ticks2;
volatile        uint32_t pend, pend2;
volatile     uint32_t count, count2;

        ticks2  = SysTick->VAL & 0xffffff;
        pend2   = ((SCB->ICSR & SCB_ICSR_PENDSTSET_Msk)||((SCB->SHCSR & SCB_SHCSR_SYSTICKACT_Msk)))  ? 1 : 0;
        count2  = GetTickCount();

do {
        ticks=ticks2;
        pend=pend2;
        count=count2;
        ticks2  = SysTick->VAL & 0xffffff;
        pend2   = ((SCB->ICSR & SCB_ICSR_PENDSTSET_Msk)||((SCB->SHCSR & SCB_SHCSR_SYSTICKACT_Msk)))  ? 1 : 0;
        count2  = GetTickCount();
} while ((pend != pend2) || (count != count2) || (ticks < ticks2));

    volatile uint32_t r=((count+pend) * 1000) + ((84000  - ticks) / 84) ;

        return r;
}

It is complicated to describe but what it does is repeatedly read the timer values and detects pending timer ticks until it thinks it has sensible readings, then it calculates micros from those. I tried it on your code for 5 minutes and it was OK and I've tested it for monotonicity for 5 minutes too, but there's so much code out there that uses micros() I can't guarantee it works on everything.

This fix appears to be better than I first thought :slight_smile: I have now given it a couple of hours testing without error.

This is my current version of the fix, it adds some optimizations for speed. It probably isn't as fast as it could be (a call takes about 1.8us).
hardware/arduino/sam/cores/arduino/wiring.c :

uint32_t micros( void )
{
    uint32_t ticks, ticks2;
    uint32_t pend, pend2;
    uint32_t count, count2;

    ticks2  = SysTick->VAL;
    pend2   = !!((SCB->ICSR & SCB_ICSR_PENDSTSET_Msk)||((SCB->SHCSR & SCB_SHCSR_SYSTICKACT_Msk)))  ;
    count2  = GetTickCount();

    do {
        ticks=ticks2;
        pend=pend2;  
        count=count2;
        ticks2  = SysTick->VAL;
        pend2   = !!((SCB->ICSR & SCB_ICSR_PENDSTSET_Msk)||((SCB->SHCSR & SCB_SHCSR_SYSTICKACT_Msk)))  ;
        count2  = GetTickCount();
    } while ((pend != pend2) || (count != count2) || (ticks < ticks2));

    return ((count+pend) * 1000) + (((SysTick->LOAD  - ticks)*(1048576/(F_CPU/1000000)))>>20) ;
}
1 Like

hi,

I've tested with my 4 channel RC reading. After replacing the micros(), it does fix the jittering in RC reading. My test was successful for continuous reading of 15min.

Thank you guys!

Thanks for testing it, I have given it many hours of testing myself too and have had no errors. I've opened a pull request on github: Changed micros() to work in interrupts by stimmer · Pull Request #1388 · arduino/Arduino · GitHub

Hi,
Thanks Stimmer for your work on the confirmation and resolution.

Duane B

rcarduino.blogspot.com

Is this 'bug' still existing in the current Due implementation of the micros() function?

Do you know how to read the Github pull request? It says "merged" which means that it has been incorporated in the Arduino code.

This bug was solved in 2013.

No, I didn't until now. Thank you!