First post! Can't get arduino to read past two conditions inside an ISR

Hello everyone,

I am new to the forum and just as new to Arduino (2 months). I have a technical background but nothing regarding programming.

I look forward to being able to contribute to this forum but for the immediate future I am afraid I will be more of a drain, so please bear with me!

So… I should start by describing the part of my circuit involved by the question.

I have a +5v source connected to A1 and digital 2 (interrupt). I have connected both through a voltage divider so that the voltage sensed at both pins will change from +5v while no buttons are pressed, to anywhere below that when a particular button was pressed (buttons ground out PS1 - PS4 on the terminal block). See below.

I have created an ISR that is “falling” triggered. here is the main code. Obviously I am missing something about this and I have been trying very hard to solve it on my own.

The objective is to have TWO counters set up using 3 buttons:

counter one is "buttonCount’ and uses 2 buttons, one to increase and one to decrease.
counter two is “idleCount” and it is only going to be 1 or 0 so I just use ++ and code to go back to zero.

Here is the code.

#include <PID_v1.h>

#include <Servo.h> // include servo library



#include <LiquidCrystal.h>
LiquidCrystal lcd(9, 8, 7, 6, 5, 4); // Set to GENboardV1 pins

volatile float buttonCount = 0; // store buttonCount in RAM, decimal format

volatile int idleCount = 0; // store idle setting in RAM, whole numbers

volatile int manualSpeed = 0; // store manual setting in RAM, whole numbers



float speedSetting = 0;

Servo myservo;

double Setpoint, Input, Output; // defines input types

PID myPID(&Input, &Output, &Setpoint, 2, 5, 1, DIRECT);

void setup()

{
  
  pinMode(10, Output); // sets output of servo to pin 10
  
  pinMode(3, INPUT); // sets digital pin 3 to paddlewheel input
  
  myservo.attach(10);
  
  attachInterrupt(0, switchISR, FALLING); // digital pin 2. interrupt waits for voltage drop
  
  pinMode(2, INPUT); //interrupt pin
  
 // pinMode(A3, INPUT); // manual mode button
  
 // digitalWrite(A3, HIGH); // enable pull-up resistor for manual button
  
  myPID.SetOutputLimits(0, 22); //Limits output to match servo limits (0- 90 degrees)
  
  myPID.SetMode(AUTOMATIC); // turns on PID controls
  
  Serial.begin(9600);
  
  lcd.begin(16, 2);
}

void loop()

{
  
  
  speedSetting = buttonCount / 10;
  
  //int manualButton = A3; // button to activate manual setting
  
  int paddlewheel = 3; // sets 3 to paddlewheel input
  
  int servodrive = 10; // sets digital pin 10 to servo output
  
  int paddlesignal = (((1 / ((pulseIn(3,LOW) * .000001) / 2) / 5.7) * 1.15)); // sets input to read paddlewheel
  
  /*while(idleCount == 1) {   // if idlecount is at 1, servo stays at idle)
    myservo.write(0); //Servo to 0 degrees
    lcd.setCursor(0, 0);
    lcd.print("IDLE");
    
    
  }
  */
  
    
 // manualSpeed = 0;
  
  //while(digitalRead(manualButton) == LOW){
   // myservo.write(manualSpeed);
//}
    
    
  Setpoint = speedSetting;
  
  Input = paddlesignal; // makes the PID read the signal from the paddlewheel
  lcd.clear();
  lcd.setCursor(0, 0);
  lcd.print("Setting:");
   lcd.setCursor(10, 0);
  lcd.print(speedSetting);
  Serial.println(paddlesignal);
  lcd.setCursor(0, 1);
  lcd.print("Actual:");
    lcd.setCursor(10, 1);
  lcd.print(paddlesignal);
  Serial.println(Setpoint);
  Serial.println(idleCount);
 
  myPID.Compute(); // computes
  
  myservo.write(Output); // output is written to the servo (in degrees)
  
  Serial.println(Output);
  
}

  void switchISR(){
  
 static unsigned long last_interrupt_time = 0;
 unsigned long interrupt_time = millis();
 // If interrupts come faster than 200ms, assume it's a bounce and ignore
 if (interrupt_time - last_interrupt_time > 200);
 { 
  
   int buttonState = map((analogRead(A1)), 1083, 0, 0, 4);
   
  switch (buttonState){ 
   
   case 1:
   idleCount++;
   break;
    
   case 2:
   buttonCount--;
   break;
   
    case 3:
   buttonCount++;
   break;
   
   
 /*if (buttonState == 3) buttonCount++;
 
 //if(manualSpeed > ) manualSpeed = 100; };
   
 if (buttonState == 2) buttonCount--;
  
 //if (buttonState == 1) idleCount++;

 //if (analogRead(A1) > 120 && analogRead(A1) < 135) buttonCount++;
 
 // {if(manualSpeed > 1) manualSpeed = 1;}
 
  /*if(speedSetting > 5){
   speedSetting = 5;
   lcd.setCursor(0, 0);
   lcd.println("5 / MAX SPD");
 }*/
 

 
  /*if(speedSetting < 0.5){
   speedSetting = 0.5;
   lcd.setCursor(0, 0);
   lcd.println("0.5 / MIN SPD");
  }
  */
 
  if(idleCount > 1) idleCount = 0;
 
 
 
  last_interrupt_time = interrupt_time; 
  
  
 }
  }
  }

The first counter, “buttonCount”, works, it increments up and down. The second one “idleCount” will not.

This is trying switch/ case, and I tried a bunch of if statements previously,

(ie: "if (analogRead(A1) > 800) && (analogRead(A1) < 850) buttonCount++;)

but had similar results, I can get the first counter to increment but not the second.

So then I tried to make a simple code to test the switch/ case setup, incase I screwed it up so I wrote simple code with the switch/ case set up out of my ISR and it works just fine, increments both counters perfectly. So the issue seems to be with how I have designed the ISR…

Can anyone help me?

Roto.

Rule #1 - put as little code inside an interrupt routine as possible.

What this means is set a variable to some value - what used to be called setting a “flag” - and doing the processing in loop() if at all possible. Printing to the LCD is probably a bad thing to put inside an interrupt. Analog Read may also not be the best thing to put inside a interrupt routine. Also there needs to be some time for the analogread to complete - yu press the button, the voltage drops, and the analog read needs some time to get a good reading.

What might work better for you is to have a variable that keeps track of the last button state and that is reset every time through loop. What you then look for is if the last button state and the current state are different then you do whatever is appropriate. This is called Polling. Unless the event (the button press) is very fast polling should do the job just fine.

   int buttonState = map((analogRead(A1)), 1083, 0, 0, 4);

The range of values that analogRead can return is 0 to 1023. Why are you using 1083 in the map() call?

If the interrupt is triggered by the falling edge, the edge has fallen below 3V, so the analogRead() is going to return a value between 0 and about 600. These will be mapped to the high end of the scale, since your mapping is backwards. That is, buttonState will never be 0 in the ISR.

PaulS:    int buttonState = map((analogRead(A1)), 1083, 0, 0, 4);

The range of values that analogRead can return is 0 to 1023. Why are you using 1083 in the map() call?

If the interrupt is triggered by the falling edge, the edge has fallen below 3V, so the analogRead() is going to return a value between 0 and about 600. These will be mapped to the high end of the scale, since your mapping is backwards. That is, buttonState will never be 0 in the ISR.

Hi PaulS,

I made a boo boo on that one, but that isn't the problem as it still works fine outside the ISR for the exact circuit it is attached to. I will change that right now!

kf2qd: Rule #1 - put as little code inside an interrupt routine as possible.

What this means is set a variable to some value - what used to be called setting a "flag" - and doing the processing in loop() if at all possible. Printing to the LCD is probably a bad thing to put inside an interrupt. Analog Read may also not be the best thing to put inside a interrupt routine. Also there needs to be some time for the analogread to complete - yu press the button, the voltage drops, and the analog read needs some time to get a good reading.

What might work better for you is to have a variable that keeps track of the last button state and that is reset every time through loop. What you then look for is if the last button state and the current state are different then you do whatever is appropriate. This is called Polling. Unless the event (the button press) is very fast polling should do the job just fine.

I figured that may have been the case and tried to enter a delay(50) to get it to stabilize, obviously delay doesn't work inside an ISR, duh!.

Could you give a tiny example of what you are suggesting? Are you saying to remove the interrupt? isn't there a chance it will miss the button press if there is no interrupt?

if the interrupt just changes a variable according to the state of the button, then the switch/ case stuff is called in the loop according to the state (like "if (state != lastState) perform switch/ case stuff here" ??), won't the button potentially not be pressed anymore by the time it gets there to perform analogRead?

Thank you guys for your time!

isn't there a chance it will miss the button press if there is no interrupt?

What is "pressing the button"? If it is a human, then polling is plenty fast (as long as you do not use delay()).

won't the button potentially not be pressed anymore by the time it gets there to perform analogRead?

Depends on what the code is doing. If it is reasonably written (no delay()s or other blocking code), then no.

If I understand correctly, though, you have a variable voltage being applied to the analog pin. You want to measure that voltage when it triggers an interrupt. In order for that variable voltage to trigger a falling interrupt, the voltage needs to drop below 60% of VREF (typically 5V, dropping to about 3.0V). The interrupt is not triggered as soon as the voltage begins to drop.

The use of an ISR in this case is not really feasible.

PaulS:
If I understand correctly, though, you have a variable voltage being applied to the analog pin. You want to measure that voltage when it triggers an interrupt. In order for that variable voltage to trigger a falling interrupt, the voltage needs to drop below 60% of VREF (typically 5V, dropping to about 3.0V). The interrupt is not triggered as soon as the voltage begins to drop.

The use of an ISR in this case is not really feasible.

Well FML…

There is the piece of info I was lacking… I even designed (and had manufactured) a circuit board around my flawed understanding of interrupts.

However, couldn’t I just adjust the resistor values in the divider so that depending on what button was pressed the result was always lower than 3v ??? Then it would work fine wouldnt it???

I will however try as you suggested, without the ISR all together. But to be clear, when you say “polling is plenty fast”, by polling you just mean when the loop goes over that particular line of code, looking for a change right? So the speed of the “polling” is completely dependant on the speed of the entire loop? This code is also running a PID and LCD library so I hope that doesn’t bog it down too much.

Thanks again!

So the speed of the "polling" is completely dependant on the speed of the entire loop?

Yes.

This code is also running a PID

So? That should be plenty fast.

and LCD library so I hope that doesn't bog it down too much.

Writing to the LCD is not particularly fast. Judicious writes are the key (only write when there is really new data, etc.).

Ditch the ISR, for the following reasons.

1 - you are doing waaay too much in the ISR. ISR time is not “free”, it takes cycles away from the main loop. It will probably mess up your PID code because the timing will be off.

2 - you should not hook an un-debounced button to an interrupt. It could generate many, many spurious interrupts as it moves from one state to another, giving you unreliable results and bogging down your main loop. The forum and the playground have lots of articles about debouncing.

However, couldn’t I just adjust the resistor values in the divider so that depending on what button was pressed the result was always lower than 3v ??? Then it would work fine wouldnt it???

3 - I think (not entirely sure) the way you’ve got the circuit wired, there will be some voltage leakage out the interrupt pin. This will make the voltage at the analog pin different from what you expect. So even with different resistor values, it’s not going to act like you want.

Fortunately, you are not the first person to hook up multiple buttons to an analog pin. You can search the forum for similar topics and get lots of advice about how to do that. There is even some code in the playground http://arduino.cc/playground/Code/AnalogButtons that might be of some help. It may not be usable as-is, but you can certainly copy pieces and ideas from it.

HTH,

PaulS:

This code is also running a PID

So? That should be plenty fast.

and LCD library so I hope that doesn't bog it down too much.

Writing to the LCD is not particularly fast. Judicious writes are the key (only write when there is really new data, etc.).

Hi Paul,

I am not questioning you, I am just trying to learn, so my statement about the PID library was more of a question, I have no idea how demanding it is on the system, obviously you do, so thank you for clearing that up.

I will look into the judicious writes to the LCD screen, I assume you would create a variable that contains what was written to the LCD last and compare it with a "!=" comparator for writing again?? I think I could muddle my way through that.

ckiick: Ditch the ISR, for the following reasons.

1 - you are doing waaay too much in the ISR. ISR time is not "free", it takes cycles away from the main loop. It will probably mess up your PID code because the timing will be off.

2 - you should not hook an un-debounced button to an interrupt. It could generate many, many spurious interrupts as it moves from one state to another, giving you unreliable results and bogging down your main loop. The forum and the playground have lots of articles about debouncing.

However, couldn't I just adjust the resistor values in the divider so that depending on what button was pressed the result was always lower than 3v ??? Then it would work fine wouldnt it???

3 - I think (not entirely sure) the way you've got the circuit wired, there will be some voltage leakage out the interrupt pin. This will make the voltage at the analog pin different from what you expect. So even with different resistor values, it's not going to act like you want.

Fortunately, you are not the first person to hook up multiple buttons to an analog pin. You can search the forum for similar topics and get lots of advice about how to do that. There is even some code in the playground http://arduino.cc/playground/Code/AnalogButtons that might be of some help. It may not be usable as-is, but you can certainly copy pieces and ideas from it.

HTH,

Hi HTH,

1- Sounds like the resounding theme here! I will certainly get rid of it. Thanks.

2- The buttons are debounced already with this :

static unsigned long last_interrupt_time = 0;
 unsigned long interrupt_time = millis();
 // If interrupts come faster than 200ms, assume it's a bounce and ignore
 if (interrupt_time - last_interrupt_time > 200);

3- The interrupt pin won't leak "volage", it will leak a microscopic amount of current. This will have a minute impact on the volage read at the divider but it would be almost immesasurable at that current level.

To combat this you just need to create the circuit and test it with analogRead, then you know the corrected values for exactly what you are using.

The volage divider and buttons already work perfectly outside of the ISR for the counters, so there is no issue here.

Thank you for your help!

I assume you would create a variable that contains what was written to the LCD last and compare it with a “!=” comparator for writing again?

Exactly. If “what to write this time” is the same as “what to write last time” (or, rather, what was written last time), don’t spend the time writing.

As for the divider working for the "falling" interrupt.....

In the circuit diagram I posted earlier if R4 = 1k, R5 = 250, R6 = 666, R7 = 1.5k,

Then:

no button = 5v button PS1 = 1v button PS2 = 2v button PS3 = 3v

I know the values of resistor are not standard, you could just pick the nearest standard value and compensate in the code.

In the diagram the resistors are in series and cumulative, meaning that when you hit PS3, current flows through R5, R6, and R7 to ground. That means that the actual resistor in the R7 position would be 584 ohms, but the cumulative resistance would be 1.5k.

EDITED to make sense.

PaulS:

I assume you would create a variable that contains what was written to the LCD last and compare it with a "!=" comparator for writing again?

Exactly. If "what to write this time" is the same as "what to write last time" (or, rather, what was written last time), don't spend the time writing.

Ok great, glad I understood you.

That might help the speed even more because I am writing a value / 10 (to get a decimal #) to the LCD and from what I understand floating point math is verrrrrry slow and to be avoided at all costs, but I need 1 decimal place in the display.

So I can store the variable before I convert it to floating point, and just convert it when it writes to the LCD.

rotorfixer:

PaulS:

I assume you would create a variable that contains what was written to the LCD last and compare it with a "!=" comparator for writing again?

Exactly. If "what to write this time" is the same as "what to write last time" (or, rather, what was written last time), don't spend the time writing.

Ok great, glad I understood you.

That might help the speed even more because I am writing a value / 10 (to get a decimal #) to the LCD and from what I understand floating point math is verrrrrry slow and to be avoided at all costs, but I need 1 decimal place in the display.

So I can store the variable before I convert it to floating point, and just convert it when it writes to the LCD.

Or...

int x = analogRead(A0);
lcd.print(x/10);
lcd.print('.');
lcd.print(x%10);

One decimal point, divided by 10, no floating point.

You shouldn't be too worried about using floating point stuff, though. As a rule of thumb, any calculations you do will be vastly faster than anything external, at least for things most people use Arduinos for.

WizenedEE: Or...

int x = analogRead(A0);
lcd.print(x/10);
lcd.print('.');
lcd.print(x%10);

One decimal point, divided by 10, no floating point.

You shouldn't be too worried about using floating point stuff, though. As a rule of thumb, any calculations you do will be vastly faster than anything external, at least for things most people use Arduinos for.

Hmmm, confused again!

when I wrote this :

speedSetting = buttonCount / 10;

That was my floating point conversion.... that made the lcd show one decimal place. Am I wrong? Is that not "floating point math?

To be honest I am not sure what your code does, since it already contains the /10 that gives me the result I need!!!

While we are on the subject of math.... I have wrote this in the program we are discussing:

 int paddlesignal = (((1 / ((pulseIn(3,LOW) * .000001) / 2) / 5.7) * 1.15));

I haven't been able to test it aside from compiling it, but is arduino able to do "complicated equations" like this??

rotorfixer:
That was my floating point conversion… that made the lcd show one decimal place. Am I wrong? Is that not "floating point math?

To be honest I am not sure what your code does, since it already contains the /10 that gives me the result I need!!!

You were concerned about speed; my version makes the code run faster because there’s no floating point math. x was an int and 10 is an int, so x/10 is fairly fast. % is the modulo operator, so x%10 will give you the remainder – ie, what’s after the decimal point. So instead of converting your reading to a float, dividing by 10, and then interpreting that as a string, you just convert the int to a string.

While we are on the subject of math… I have wrote this in the program we are discussing:

 int paddlesignal = (((1 / ((pulseIn(3,LOW) * .000001) / 2) / 5.7) * 1.15));

I haven’t been able to test it aside from compiling it, but is arduino able to do “complicated equations” like this??

Computers don’t get confused :wink:

Remember to mind your decimal points so you use floating point math when you want and integer math when you want. For integer math, mind overflow and underflow and what kind of variables you have (eg 1000*1000 will return an unexpected value since it’s a 16 bit integer)

Ok I took a look at what you said about that equasion and it appears as though it would overflow (?) in a few places.

The goal is to take "paddleSignal" and convert it to a float with one decimal named "MPH"

So I have changed it to this:

unsigned long raw = (((((10000000 / paddleSignal) / 2) * 175) * 115) / 100000)
float MPH = raw / 10

Using this with a typical paddleSignal of 22254 I get a (desired) MPH result of 4.5

I assume because I have declared it as unsigned long, when the math equals a decimal value it will either round it off or just drop the decimal completely, is that correct?

Does this seem ok to you guys??

I appreciate the help because to test this I have to take my boat out on the water and that is an all day event!!

I assume because I have declared it as unsigned long, when the math equals a decimal value it will either round it off or just drop the decimal completely, is that correct?

You phrased an either/or question, and want to know if the answer is yes. No, it isn't. The result of the math is integer at each step, since only integers are involved. At each step, any decimal value is truncated. It has nothing to do with the storage type of the variable on the left of the equation. It matters only what the type of the operands are for each operation.

rotorfixer: and from what I understand floating point math is verrrrrry slow and to be avoided at all costs

That really needs to be put into context.

Relative to integer operations, floating point operations are indeed very slow.

Relative to User Interface performance requires, floating point operations aren't slow at all. Within reason anyways, there's another thread here where the poster is perform ~65,000 loops of multiple floating point operations, and in that case it is indeed running very slow.

In your case, all your floating point calculations combined are likely taking less than a millisecond or two, so are going to have zero impact on your UI. As was already said, activities like writing to the LCD, serial comms, lots of analogReads, etc, are what's going to slow things down. Benchmarking your code is the best way to pinpoint where the real slowdowns are occurring, and that's pretty easy to do.

unsigned long timer = micros();
//code to be benchmarked here
//Can be as much code as you want
timer = micros() - timer;
//timer now equals the number of microseconds it took for your above code to execute
Serial.println(timer);

PaulS:

I assume because I have declared it as unsigned long, when the math equals a decimal value it will either round it off or just drop the decimal completely, is that correct?

You phrased an either/or question, and want to know if the answer is yes. No, it isn't. The result of the math is integer at each step, since only integers are involved. At each step, any decimal value is truncated. It has nothing to do with the storage type of the variable on the left of the equation. It matters only what the type of the operands are for each operation.

Hi Paul,

I didn't need to know specifically which one was occurring, just that one of them was. I understand now what you mean about the storage type having nothing to do with the number format (I thought unsigned long was an int no matter what), but because I didn't declare it as an int I assume it just defaults to int?

As always I appreciate your help as I try to figure this stuff out.

jraskell:

rotorfixer: and from what I understand floating point math is verrrrrry slow and to be avoided at all costs

That really needs to be put into context.

Relative to integer operations, floating point operations are indeed very slow.

Relative to User Interface performance requires, floating point operations aren't slow at all. Within reason anyways, there's another thread here where the poster is perform ~65,000 loops of multiple floating point operations, and in that case it is indeed running very slow.

In your case, all your floating point calculations combined are likely taking less than a millisecond or two, so are going to have zero impact on your UI. As was already said, activities like writing to the LCD, serial comms, lots of analogReads, etc, are what's going to slow things down. Benchmarking your code is the best way to pinpoint where the real slowdowns are occurring, and that's pretty easy to do.

unsigned long timer = micros();
//code to be benchmarked here
//Can be as much code as you want
timer = micros() - timer;
//timer now equals the number of microseconds it took for your above code to execute
Serial.println(timer);

Hi,

Thank you for the benchmarking example, I will be eager to try that out!

I didn't convert my code to integer math for speed, I did it because as a float it had too many digits a few times during the equation. I see now that if you use the newest hardware the floating point math is just as fast as integer math (within reason, I'm sure).

So the numbers that are used and the products they will develop shouldn't be an issue right? I believe that if you use unsigned long you can have a number as high as 4,xxx,xxx,xxx?

Thanks Guys!

I thought unsigned long was an int no matter what), but because I didn't declare it as an int I assume it just defaults to int?

long is an integer type. int is an integer type. long and int are distinct type, with different numbers of bytes.

Be careful using int where you mean integer. They are not the same thing in all cases.