Problem with interrupts

Question

Alright. This is what I wanted to do:

Make a circuit and code the arduino such that there are 2 tactile switches, each with a predefined pwm value. Whenever any one button is pressed the led should run at that pwm value, until the button is released. There is also an ultrasonic sensor on the same arduino that triggers an interrupt at a threshold distance that stops the led. When no button is pressed or the ultrasonic isnt active, the led should run at another pwm value.

Here is the code:(updated)

const int trig = 9;
const int echo = 10;
const int redswitch = 2;
const int yellowswitch = 3;
int redled = 6;
int greenled = 5;
volatile int pwm = 50;

volatile bool rs = LOW;      //RS - RED SWITCH
volatile bool ys = LOW;      //YS - YELLOWSWITCH
bool last_rs = LOW;
bool last_ys =  LOW;
volatile long distance=0;
long duration=0;
long temp=0;

void setup(){
  pinMode(trig,OUTPUT);
  pinMode(echo,INPUT);
  pinMode(redled,OUTPUT);
  pinMode(greenled,OUTPUT);
  pinMode(redswitch,INPUT);
  pinMode(yellowswitch,INPUT);
  Serial.begin(9600);

  attachInterrupt(digitalPinToInterrupt(redswitch), red,CHANGE);
  attachInterrupt(digitalPinToInterrupt(yellowswitch), green,CHANGE);

  digitalWrite(greenled,LOW);
  digitalWrite(redled,LOW);
}

boolean debounce(boolean temp,int switchPin)
{
  boolean current = digitalRead(switchPin);
  if (temp != current)
  {
    delay(5);
    current = digitalRead(switchPin);
  }
  return current;
}

void loop(){


  //Ultrasonic sensor's distance calculation
  digitalWrite(trig,LOW);
  delayMicroseconds(5);

  digitalWrite(trig,HIGH);
  delayMicroseconds(10);
  digitalWrite(trig,LOW);

  duration = pulseIn(echo, HIGH);
  temp = duration * 0.0343 / 2;
  noInterrupts();
  distance = temp;
  rs = debounce(last_rs,redswitch);
  ys = debounce(last_ys,yellowswitch);
  last_rs = rs;
  last_ys = ys;
  if(distance>8)interrupts();

  Serial.println(distance);  
  analogWrite(redled,pwm);
  analogWrite(greenled,pwm);
}

void red(){

  if(rs == HIGH) pwm = 150;
  else pwm = 0;


}

void green(){

  if(ys == HIGH) pwm = 250;
  else pwm = 0;

}

Now, when the arduino is connected to my laptop, led runs at the a particular value. But whenever I press a button they go off. Can you help me find out what's wrong here?

Answer 1

Look at these lines:

distance = temp;
duration = pulseIn(echo,HIGH);
distance = duration * 0.0343 /2;

Here is what they do:

• The first line sets the variable distance to zero (that's the value of temp).
• The second line waits for the ultrasonic pulse to reach the target, bounce against it and come back, while at the same time timing the whole trip. This can take a long time (several milliseconds). Chances are your program spends most of its time executing this line.
• The third line sets the variable distance to something sensible.

This means distance is zero most of the time (while the second line executes). When the interrupt fires, there is a very high probability for the interrupt handler to find a zero distance, and thus to set pwm to zero.

Simple fix: never set distance to an unreasonable value. Leave it's value alone until you have a new distance measurement ready, and only then update that variable. In other words, just remove the first of those three lines.

But there is a catch now. The variable is four bytes long, and updating it takes four instructions. There is a small probability that the interrupt fire in between these instructions. When that happens, the interrupt handler finds the distance variable half-updated, with a mix of bytes from the previous and the current distance measurements. This is called a “race condition”, and is one of the worst kind of bugs you can meet, because it is very hard to systematically reproduce.

The fix to the race condition is to update the variable with interrupts disabled. The wrong way to do it is:

duration = pulseIn(echo, HIGH);
noInterrupts();
distance = duration * 0.0343 / 2;
interrupts();

This is bad because interrupts are blocked during the distance calculation, which is too much. The right way is this:

duration = pulseIn(echo, HIGH);
temp = duration * 0.0343 / 2;
noInterrupts();
distance = temp;
interrupts();

Now interrupts are blocked only while writing the distance variable.

Personally I don't think it is such a great idea to read the buttons with interrupts. Buttons are extremely slow peripherals. If you make your code non-blocking, your loop() will always be fast enough to deal with them. Handling the buttons in loop() means you don't have to worry about all the bugs that can easily sneak into interrupt-driven code, like the above mentioned race condition.

---

Edit: In the original version of your code, interrupts were used to read the switches. In the edited version, you added a debouncer, and you don't need interrupts anymore. In fact, the way this version uses interrupts makes no sense at all. The most problematic bit is the fact that this code can disable interrupts for arbitrarily long times. This is not good, and you should be aware that the Arduino's timekeeping functions rely on interrupts to perform their job.

I suggest you just remove from this code every single line where you see the word “interrupt” (singular or plural), and you replace

if(distance>8)interrupts();

by

if (distance > 8) {
    red();
    green();
}

or, to streamline things a little bit, by

if (distance > 8) {
    if (ys == HIGH) pwm = 250;
    else if (rs == HIGH) pwm = 150;
    else pwm = 0;
}