I must be overlooking something, please help

Hi, This is an air ride suspension controller. It has an up, and down button that are working. It also has a program ride height, that will write the pressure transducer state to the controller when both buttons are pressed, this is also working. Upon start up it will test the current air pressure against the programmed pressure, and auto air up, or auto air down depending on current state. The auto air up is working, but the auto air down is not. It is the same code, and similar loop. I can see it going through the else if statement and then serial write “Air Down” like it knows it has to air down, but it doesn’t go into the auto air down loop. What am I missing. Thanks, adown

//Air Ride Controller
#include <EEPROM.h>; //Save even when powered off
int Address = 0; //Starting location to save information

const int upswitchPin = 2;    //pin number of up button input - up button
const int downswitchPin = 3;  //pin number of down button input - down button
//const int passengerPin = 4;   //future input from passenger peg to passenger add a second programmed air pressure

const int ledPin = 5;        //set led to pin # 5 - status LED

const int goupPin = 7;        //pin number of the up output pin - to pump relay up
const int godownPin = 8;      //pin number of the down output pin - to pump relay down

const int airPin = A0;        //assign input A0 to airPin - anologue read on air pressure or linear rheostat, 0-5v

int ledState = LOW;           //innitialize led to low

int upswitchState = 0;        //innitialize the up switch state to 0
int downswitchState = 0;      //innitialize the down switch state to 0
int airState = 0;             //variable for analog read - air pressure or linear rheostat reading to set height, 0-5v
int progairState = 0;         //variable for programmed air heigth - get the preferred height and save it as progairState


long previousMillis = 0;        //store the last time the led was updated - for status LED
long interval = 250;          //set time for blink interval - for programming mode
long interval1 = 100;          //set time for blink interval - for airing up
long interval2 = 300;          //set time for blink interval - for airing down


void setup() {          
  pinMode(goupPin, OUTPUT);  // set pin assignments  
  pinMode(godownPin, OUTPUT);
  pinMode(ledState, OUTPUT);
  //pinMode(passengerPin, INPUT);
  pinMode(upswitchPin, INPUT_PULLUP);
  pinMode(downswitchPin, INPUT_PULLUP);

  Serial.begin(9600);        //set up for analog read in

  int airState = analogRead(airPin);  //read the input on analog pin 0:
  float inches = airState * (5 / 1023.0); //airstate to reflect 0-5volts, suspension travel
  Serial.print("air state: ");
  Serial.println(airState);
  progairState = EEPROM.read(Address) * 4; //Pul out of EEPROM and scale back up to normal value.
  Serial.print("Programmed Air State: ");  //Print to serial for debug
  Serial.println(progairState);  //Print to serial for debug

  if (airState < progairState) {           //if suspension height(air pressure) is less than programmed height
    autoairup();                           //run auto air up until proairstate(programmed pressure) is reached
    Serial.println("Air Up!");
  }

  else if (airState > progairState) {      //if suspension height(air pressure) is more than programmed height
    autoairdown();                        //run auto air down until proairstate(programmed pressure) is reached
    Serial.println("Air Down!");
  }
}

void loop() {
  upswitchState = digitalRead(upswitchPin);          //read the up switch input
  downswitchState = digitalRead(downswitchPin);      //read the down switch input

  if (upswitchState == LOW && downswitchState == LOW) { //if both buttons pressed
    programrideheight();            //run programming loop

  }
  if (upswitchState == LOW && downswitchState == HIGH) { //check to see if the up button is pressed and the down switch is not
    goup();
  }   
  else {
    dontMove(); 
  }  
  if (downswitchState == LOW && upswitchState == HIGH) { //check to see if the down button is preesed and the up switch is not
    godown();
  }
  else {
    dontMove();                    // if a button isn't pressed stop motor
  }
}
void goup() {
  digitalWrite(goupPin, HIGH);     // power the up output pin
  Serial.println("goup");
}   
void godown() {
  digitalWrite(godownPin, HIGH);   // power the down output pin
  Serial.println("godown");
}
void dontMove() {
  digitalWrite(goupPin, LOW);      // write power up & down output pins low
  digitalWrite(godownPin, LOW);

}
void autoairup() {
  while (airState < progairState){
    digitalWrite(goupPin, HIGH);     // power the up output pin
    airState = analogRead(airPin);
    Serial.println("auto airing up");  //Print to serial for debug  
  } 
}
void autoairdown() { 
  while (airState > progairState){
    digitalWrite(godownPin, HIGH);   // power the down output pin
    airState = analogRead(airPin);
    Serial.println("auto airing down");  //Print to serial for debug   
  }
}
void programrideheight() {
  airState = analogRead(airPin);

  progairState = airState;
  Serial.print("Saving Data for Programming Air State: "); //Print to serial for debug
  delay(5000);                                     //5 second delay just to let led blink indicate programming mode
  Serial.println(byte(progairState / 4));          //Print to serial for debug
  EEPROM.write(Address, byte(progairState / 4));   //Save Value for pump and scale down.....Must be from 0 - 255
}
void autoairdown() { 
  while (airState > progairState){
    digitalWrite(godownPin, HIGH);   // power the down output pin
    airState = analogRead(airPin);
    Serial.println("auto airing down");  //Print to serial for debug   
  }
}

Printing something useful in this function beats the hell out of printing something useless. Can you imagine what would be useful to know?

I think @PaulS is hinting that you should print the values of the variables airState and progairState.

I don’t immediately see any reason why airup should work, but not airdown.

While it is unlikely to be relevant to your problem why are you duplicating
digitalWrite(godownPin, HIGH);
in goup() and autoairup() etc. It would be neater to put a call to goup() in autorairup() because then there would only be one piece of code to control the actual pin - and once you know it works you can forget it.

I wonder should there be a call to dontmove() at the ends of autoairup() and autoairdown()

And, just a suggestion, camelCase makes names like autoAirUp() easier to read.

…R

Here:

int airState = 0;             //variable for analog read - air pressure or linear rheostat reading to set height, 0-5v

...

  int airState = analogRead(airPin);  //read the input on analog pin 0:

You have two variables: airState. One would be plenty.


Also:

#include <EEPROM.h>; //Save even when powered off

What is that semicolon doing?

Within setup your LOCAL variable airState gets set to analogRead of airpin. using this value it is decided to call autoairdown().

Within the function autoairdown() the GLOBAL variable airState gets checked. This will still hold 0 (since it wasn't affected by the analogRead within setup ) So the while statement ( airState > progairState ) is completed before it even begins.

Printing something useful in this function beats the hell out of printing something useless. Can you imagine what would be useful to know?

pauls - If you look, the airState is printed during setup. I print things throughout for debugging purposes. No one needs useless replies to get your post count up.

While it is unlikely to be relevant to your problem why are you duplicating
digitalWrite(godownPin, HIGH);

Robin2 - Thank you, I was using one until I ran into problems. I wrote dedicated loops for debugging purposes to see where the code was going. Thank you for the camelCase suggestion. Much nicer, I agree

You have two variables: airState. One would be plenty.

What is that semicolon doing?

Thank you Nick. I am a rookie at this. I thought everything had to be initialized at 0.

Either auto format, or rookie mistake. I don't know which.

KenF - Thank you, that makes sense, but why does the autoairup work when it is written the same. Should I put this piece of code before the "else if" statement also?

  int airState = analogRead(airPin);  //read the input on analog pin 0:
 float inches = airState * (5 / 1023.0); //airstate to reflect 0-5volts, suspension travel
 progairState = EEPROM.read(Address) * 4; //Pul out of EEPROM and scale back up to normal value.

Thank you for the help and suggestions, adown

KenF - Thank you, that makes sense, but why does the autoairup work when it is written the same. Should I put this piece of code before the "else if" statement also?

Because in your autoairup your while statement is looking for airState being SMALLER than progairState. Since it is holding zero, it IS smaller and therefore the while loop is executed. Once it executes once, then the global variable gets updated.

adown:
pauls - If you look, the airState is printed during setup. I print things throughout for debugging purposes. No one needs useless replies to get your post count up.

If you look, an "airState" is printed, but it's not the "airState" that's tested in "autoairup" or "autoairdown".

That may or may not be important.

pauls - If you look, the airState is printed during setup

What the value was then and what the value is when you use may not be the same thing. It would have taken less time to add the needed Serial.print() statements (4 of them) and see what the code is doing than it took to argue that you are smarter than the Arduino, and don't need to print things.

Here is what I did. I set my progairState to 0 by using gnd, then I hooked my A0 to 5v. I restart the Arduino, and it prints in airState 1023, and programmed air 0. Then I see it "print Air" Down which is at the end of the else if statement. It seems to me it is reading the AirState, at least in the if statement. Why do you say it is reading 0 in while loop, and how do I fix it. I am not arguing, rather trying to understand why I am seeing what I'm seeing, and fix it. What should I do fix it. Thanks

The problem, as several people have alluded to above, is that you have declared a second airstate variable in setup:

  int airState = analogRead(airPin);  //read the input on analog pin 0:

That 'int' is the cause of your troubles; get rid of it.

wildbill, THANK YOU!!, I thought Nick meant it was not necessary, not that it was causing my problem. Thank you for a straight forward answer, adown

adown:
wildbill, THANK YOU!!, I thought Nick meant it was not necessary, not that it was causing my problem. Thank you for a straight forward answer, adown

I described EXACTLY how it's causing your problem in answer #4

adown:
wildbill, THANK YOU!!, I thought Nick meant it was not necessary, not that it was causing my problem.

I said you had two variables called the same thing. Obviously you interpreted that as: you have one variable which you initialized earlier.

pauls - If you look, the airState is printed during setup. I print things throughout for debugging purposes. No one needs useless replies to get your post count up.

If you had added the print that PaulS suggested you would have seen that an "impossible" thing had happened, namely that "airState" had a value that you didn't expect. So his suggestion was quite good, and attacking the messenger as trying to get his post count up was not called for. An apology might be in order here.

KenF, I’m sorry, it wasn’t clear to me what you were saying. I guess I needed the dummy version.

Nick, PaulS didn’t really suggest anything in his post, and I’m not a mind reader.

I am a beginner, and I thought this was a very advanced project for me to tackle. Especially for how far I got without help.

It actually didn’t work just taking out

int airState = analogRead(airPin);  //read the input on analog pin 0:

but, I did what I said in post 5. I added

int airState = analogRead(airPin);  //read the input on analog pin 0:
 float inches = airState * (5 / 1023.0); //airstate to reflect 0-5volts, suspension travel

to the autoairup, and autoairdown loops, and it works as intended.

Thanks for the help guidance, adown

Nick, PaulS didn't really suggest anything in his post, and I'm not a mind reader

Neither is the compiler.

OK, I'll try again.

int airState = analogRead(airPin);  //read the input on analog pin 0:

That creates a new variable airState in the function where you call it. That "shadows" (hides) the global variable you also had. It was the global variable you were testing in autoairdown, not the one you just read.

All you had to do was remove the "int" from the line in setup, like this:

airState = analogRead(airPin);  //read the input on analog pin 0:

Now you are reading into the existing variable. It's important to understand this, don't just throw more lines of code at the problem, without realizing what the real error was.

adown:
Nick, PaulS didn't really suggest anything in his post, and I'm not a mind reader.

That's why I wrote Reply #2 - but it looks like you didn't take any notice of that either.

(And, for the benefit of the others, I had not spotted the duplicate definition).

...R

Nick, ohhh. Now I understand. I didn't just remove int, but now that I understand, your first reply makes more sense. Thank you for writing until I understand. I know it can be frustrating, but to many of us some of this is still like trying to read a foreign language.

Robin2, I did read it, but I didn't completely understand. I thought the consensus was I was printing too many things.

Thank you again for the help, adown

adown:
Robin2, I did read it, but I didn't completely understand.

That's fair enough. But then you should say so. Otherwise we assume you DO understand.

It's not the same as talking to someone face-to-face where I could see from your expression that you did not understand.

...R