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(){

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


boolean debounce(boolean temp,int switchPin)
  boolean current = digitalRead(switchPin);
  if (temp != current)
    current = digitalRead(switchPin);
  return current;

void loop(){

  //Ultrasonic sensor's distance calculation


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


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?

  • remove the constrain(pwm,0,255);. it does nothing. function constrain returns first parameter if the value is in constraints or one if the constraints if the value is outside – Juraj Nov 1 '18 at 13:29
  • 2
    you code has too many problems. you use one variable for both outputs. you don't debounce the buttons. you reset PWM in every loop (you don't let it run). and I would say the whole concept of reading the buttons with interrupt is wrong – Juraj Nov 1 '18 at 13:37
  • @Juraj. I use one variable for both outputs because I want both leds to have same pwm value. About deboucing, I forgot that completely. Sorry. And I dont understand this - " you reset pwm in every loop". where do I do that? i am not resetting pwm in loop(). Reading buttons with interrupt - I was following an online tutorial. I'm totally new to arduino. It would be immensely helpful if you could be a bit more clear and precise. – Kishore Nov 1 '18 at 17:31
  • but you set pwm to 0 if one of the buttons is not pushed. PWM runs once started. it should do high, low, high,low,high... but if you start it again and again, it does high, and again high and high. interrupts are good for very short events. people hold the buttons long – Juraj Nov 1 '18 at 17:41
  • I think that interrupts are the the correct way to go. It enables you to do all sorts of slow stuff in your main loop, while you buttons still works. Do not do any read or write in your interrupt functions - you already know the button has been pushed!, and the write happens in your main loop. Do nothing else than change the state of volatile variables. Add debounce. – hpekristiansen Nov 1 '18 at 17:54

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);
distance = duration * 0.0343 / 2;

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;
distance = temp;

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) {

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;
  • I updated the code in the question. Please check it. There is a great improvement. But right now, the problem seems to be that the code does exactly opposite to what i want. The led glows at a particular pwm value when no button is pressed. When it is pressed, it turns off. i want it to happen vice versa. And for this part There is also an ultrasonic sensor on the same arduino that triggers an interrupt at a threshold distance that stops the led - I use if(distance>8)interrupts(). Is this the right way to go? – Kishore Nov 2 '18 at 6:42
  • @Kishore: see amended answer. – Edgar Bonet Nov 2 '18 at 16:51
  • the thing is I'm learning arduino right now. this is like a task that has been given to me. I have to use interrupts in this code. That is compulsory. I myself have another version of the code where i don't use interrupts and it still does the job. But I have to do the same thing with the use of interrupts. So, I can't remove them. – Kishore Nov 5 '18 at 11:29
  • @Kishore: Then forget about my edit, and follow the path given in the first part of my answer. You will have to remove your debounce() function and find a algorithm suitable for interrupt-driven debouncing. An interesting problem in itself. – Edgar Bonet Nov 5 '18 at 13:16

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.