Any format/code suggetions for my first program?

Using Arduino Programming Questions
#1

This is my first program and I would love criticism. The Uno also appears to freeze after 10-15 minutes. The UNO will only work if I reload the program. Can I be running out of SRAM somehow??? Any suggestions for debugging??? Thank you for any help. :slight_smile:

/* This program uses an Arduino Uno at compares operator input from a potentiometer joystick to control  the position of air cylinders via PWM proPortPotional valves with
    a positive feedback loop from potentiometers on the air cylinders.
*/

const byte AFTPOTPIN=A0;
const byte PORTPOTPIN=A1;
const byte JOYXPIN=A2;
const byte JOYYPIN=A3;

const byte ESWITCHPIN=4;
const byte VALVEPIN1=5;
const byte VALVEPIN2=6;
const byte RELAYPIN1=10;
const byte RELAYPIN2=11;
const byte RELAYPIN3=12;
const byte RELAYPIN4=13;


int AftPot = 0;
int PortPot = 0;
int PotDiff = 0;
int PotDiff1 = 0;
int JoyX = 0;
int JoyY = 0;
unsigned long previousMillis = 0;



void setup() {

  pinMode(ESWITCHPIN, INPUT); //estop switch
  pinMode(VALVEPIN1, OUTPUT);//PWM Valve#1
  pinMode(VALVEPIN2, OUTPUT);//PWM Valve#2

  pinMode(RELAYPIN1, OUTPUT);//relay#1
  pinMode(RELAYPIN2, OUTPUT);//relay#2
  pinMode(RELAYPIN3, OUTPUT);//relay#3
  pinMode(RELAYPIN4, OUTPUT);//relay#4


  //these are not needed only for reference
  // pinMode(AFTPOTPIN,INPUT);//AftPot cylinder potentiometer Input
  //pinMode(PORTPOTPIN,INPUT); //PortPot cylinder potentiometer Input
  //pinMode(JOYXPIN,INPUT); /JoyX joystick control input
  //pinMode(JOYYPIN,INPUT);//JoyY joystick control input




  digitalWrite(RELAYPIN1, HIGH); //High Turns off 4 channel relays - LOW to turn on
  digitalWrite(RELAYPIN2, HIGH);
  digitalWrite(RELAYPIN3, HIGH);
  digitalWrite(RELAYPIN4, HIGH);

}




void loop() {

  while (digitalRead(ESWITCHPIN) == LOW) {
    digitalWrite(RELAYPIN1, HIGH); //High Turns off 4 chananel relays - LOW to turn on
    digitalWrite(RELAYPIN2, HIGH);
    digitalWrite(RELAYPIN3, HIGH);
    digitalWrite(RELAYPIN4, HIGH);
  }//if switch is low do nothing but set relays to OFF

  analogRead(AFTPOTPIN);          //read first analog input potentiometer twice to settle DAC
  AftPot = analogRead(AFTPOTPIN); //read first analog input potentiometer twice to settle DAC
  delay(2);
  PortPot = analogRead(PORTPOTPIN); //Port cylinder potentiometer
  delay(2);
  JoyX = analogRead(JOYXPIN); //Joystick brake potentiometer
  delay(2);
  JoyY = analogRead(JOYYPIN); //Joystick throttle potentiometer
  

  /*JoyX pot goes 0-850
     JoyY joystick 728-849
     gas joystick 700-850
     AftPot pot pneumatic cylinder 300-900
     PortPot pot penumatic cylinder 389-975
  */
  PortPot = constrain(PortPot, 266, 982); //needed to constrain actual max & min numbers come from the pots
  PortPot = map(PortPot, 266, 982, 0, 500);

  JoyX = constrain(JoyX, 530, 680);
  JoyX = map(JoyX, 530, 680, 0, 500);

  AftPot = constrain(AftPot, 400, 1000);
  AftPot = map(AftPot, 400, 1000, 0, 500);

  JoyY = constrain(JoyY, 428, 637);
  JoyY = map(JoyY, 428, 637, 0, 500);



  unsigned long currentMillis = millis();//this is a timing loop because the relays should fire no more than 20x/s.
  if (currentMillis - previousMillis >= 50) {
    previousMillis = currentMillis;

    /*below we take the difference from where we are (cylinder pots) to where we want to be (joystick pots) and
      send it to the PWM valves. The valves are proPortPotional so the bigger the
      difference the wider the PWM valves open.  It is a non-linear equation so writing it
      with multiple if statements is easier to adjust in the field.

      /////////////////////////Left and Right//////////////////////////////
    */
    PotDiff = (PortPot - JoyX);
    if (PotDiff > 200) {
      digitalWrite(RELAYPIN3, HIGH);
      digitalWrite(RELAYPIN4, LOW);
      analogWrite(VALVEPIN2, 155);
    }
    if (PotDiff > 100 && PotDiff <= 200) {
      digitalWrite(RELAYPIN3, HIGH);
      digitalWrite(RELAYPIN4, LOW);
      analogWrite(VALVEPIN2, 120);
    }
    if (PotDiff > 20 && PotDiff <= 100) {
      digitalWrite(RELAYPIN3, HIGH);
      digitalWrite(RELAYPIN4, LOW);
      analogWrite(VALVEPIN2, 100);
    }
    if (PotDiff <= 20 && PotDiff >= -20) {
      digitalWrite(RELAYPIN3, HIGH);
      digitalWrite(RELAYPIN4, HIGH);
      analogWrite(VALVEPIN2, 0);
    }
    if (PotDiff < -20 && PotDiff >= -100) {
      digitalWrite(RELAYPIN3, LOW);
      digitalWrite(RELAYPIN4, HIGH);
      analogWrite(VALVEPIN2, 100);
    }
    if (PotDiff < -100 && PotDiff >= -200) {
      digitalWrite(RELAYPIN3, LOW);
      digitalWrite(RELAYPIN4, HIGH);
      analogWrite(VALVEPIN2, 130);
    }
    if (PotDiff < -200) {
      digitalWrite(RELAYPIN3, LOW);
      digitalWrite(RELAYPIN4, HIGH);
      analogWrite(VALVEPIN2, 145);
    }


    /////////////////AftPot- JoyY//////////////////////////////////////////////////
    PotDiff1 = (AftPot - JoyY );//)
    if (PotDiff1 > 200) {
      digitalWrite(RELAYPIN1, HIGH);
      digitalWrite(RELAYPIN2, LOW);
      analogWrite(VALVEPIN1, 125);
    }
    if (PotDiff1 > 100 && PotDiff1 <= 200) {
      digitalWrite(RELAYPIN1, HIGH);
      digitalWrite(RELAYPIN2, LOW);
      analogWrite(VALVEPIN1, 90);
    }
    if (PotDiff1 > 20 && PotDiff1 <= 100) {
      digitalWrite(RELAYPIN1, HIGH);  //extend or JoyY (recover from go fast)
      digitalWrite(RELAYPIN2, LOW);
      analogWrite(VALVEPIN1, 80);
    }
    if (PotDiff1 <= 20 && PotDiff1 >= -20) {
      digitalWrite(RELAYPIN1, HIGH);
      digitalWrite(RELAYPIN2, HIGH);
      analogWrite(VALVEPIN1, 0);
    }
    if (PotDiff1 < -20 && PotDiff1 >= -100) {
      digitalWrite(RELAYPIN1, LOW);
      digitalWrite(RELAYPIN2, HIGH);
      analogWrite(VALVEPIN1, 80);
    }
    if (PotDiff1 < -100 && PotDiff1 >= -200) {
      digitalWrite(RELAYPIN1, LOW);  //retract or go fast
      digitalWrite(RELAYPIN2, HIGH);
      analogWrite(VALVEPIN1, 100);
    }
    if (PotDiff1 < -200) {
      digitalWrite(RELAYPIN1, LOW);  //retract or go fast
      digitalWrite(RELAYPIN2, HIGH);
      analogWrite(VALVEPIN1, 140);
    }

  }

}
#2

I could have missed something but I do not think that you are running out of RAM. It is more likely that the problem is something with your wiring or the way that you wired or powered the relays. However, there was no schematic and no information about the relays or any drivers.

Many people make the mistake of attempting to power their relays from their Arduino. Doing so may make the Arduino appear to freeze or reset.

If you disconnect the power to the relays, does the Arduino continue to run?

#3

Ditch the delay(2); You don’t need it.

This is bad style:

  PortPot = map(PortPot, 266, 982, 0, 500);

The reason why this is bad is because at some points in your program, this variable contains a 266-982 value and at other times it contains a 0-500 value. How do you know which one you have, inside some random function?

Use different variable names for different things. One is a raw analog value, one is a mapped value that has some mathematical meaning. Use two variables.

VALVEPIN2

Any time you have numbers in your variable names, you either need an array or you have poor variable names. Why don’t you call this ControlValveXAxis or something related to its actual function?

This seems like a really crude attempt at proportional control. Instead of jumping the analogWrite values by 10 or 20, make them a function of the input.

const float GainX = 0.5; //adjust this gain to give more or less output effort for the X axis
const int ThresholdX = 100; //the gain signal is added to this minimum threshold (valves don't open properly if you give them less than this amount)
const int DeadBandX = 20; //the minimum error signal that will result in a non-zero output
const int MaxOutputX = 155; //don't give more than this value to the X axis valve

if(PotDiff > DeadBandX) {
  digitalWrite(RELAYPIN3, HIGH);
  digitalWrite(RELAYPIN4, LOW);
  Output = constrain(ThresholdX + GainX * (PotDiff-DeadBandX), 0, MaxOutputX);
} else if(PotDiff < -DeadBandX) {
  digitalWrite(RELAYPIN3, LOW);
  digitalWrite(RELAYPIN4, HIGH);
  Output = constrain(ThresholdX + GainX * (-PotDiff-DeadBandX), 0, MaxOutputX);
} else  {
  //inside dead band - output zero
  digitalWrite(RELAYPIN3, HIGH);
  digitalWrite(RELAYPIN4, HIGH);
  Output = 0;
}
analogWrite(VALVEPIN2,  Output);
#4