Third Relay always on... can't find the problem

Hi There,

In a nutshell - building a simple irrigation control. You hit a button, an LED turns on, Relay switches on, gives power to a solenoid, opens the valve for that area. An indicator light turns on/blinks when the time is almost up on a relay. I have three buttons, leds and relays, solenoids ...

The problem I'm having is that the third LED and Relay are always on... immediately from startup/reset. I know that its not my wiring, because I can move the inputs around on the board and the same behaviour happens to a different set of LED/relays.

I don't know if I'm having a legit problem or if I've been looking at the code for too long and can't see an obvious error that's messing me up...I'm fairly certain its the code, but I can't figure it out. A second pair of eyes would be greatly appreciated.

Thank you.

//CONSTANTS

const int BUTTON_PIN2 = 2; // Button2
const int BUTTON_PIN3 = 3; // Button3
const int BUTTON_PIN4 = 4;
const int greenLED2 = 5;
const int greenLED3 = 6;
const int greenLED4 = 7;
const int redLED2 = 8;
const int relay1 = 9;
const int relay2 = 10;
const int relay3 = 11;


//VARIABLES

int buttonState1 = 0;          // current state of the button
int buttonState2 = 0;          // current state of the button
int buttonState3 = 0;
int lastButtonState = 0;       // previous state of the button
bool relayOn1 = false;
bool relayOn2 = false;
bool relayOn3 = false; 

//MILLIS
unsigned long previousMillis = 0;
unsigned long previousMillis2 = 0;
unsigned long previousMillis3 = 0;
const unsigned long interval = 3000;
const unsigned long redLedInterval = 1000;





void setup()
{
  pinMode(BUTTON_PIN2, INPUT);
  digitalWrite(BUTTON_PIN2, HIGH);  
  pinMode(BUTTON_PIN3, INPUT);
  digitalWrite(BUTTON_PIN3, HIGH);  
  pinMode(BUTTON_PIN4, INPUT);
  digitalWrite(BUTTON_PIN4, HIGH);
  
  Serial.begin(9600);

  pinMode(greenLED2, OUTPUT);
  pinMode(greenLED3, OUTPUT);
  pinMode(greenLED4, OUTPUT);
  pinMode(redLED2, OUTPUT);

  //SWITCH HANDLERS
  pinMode(relay1, OUTPUT);
  digitalWrite(relay1, HIGH);
  pinMode(relay2, OUTPUT);
  digitalWrite(relay2, HIGH);
  pinMode(relay3, OUTPUT);
  digitalWrite(relay3, HIGH);

}


void loop(){
  
    // read the pushbutton input pin:
    buttonState1 = digitalRead(BUTTON_PIN2);
    buttonState2 = digitalRead(BUTTON_PIN3);
    buttonState3 = digitalRead(BUTTON_PIN4);

    unsigned long currentMillis = millis(); // unsigned long currentMillis = millis();

    // if button is pressed, turn relay on (if it wasn't already on), and reset the timer
    if( buttonState1==HIGH ) // no need to check for previous state, in this specific case
    {
        previousMillis = currentMillis;
        digitalWrite(relay1, LOW);
        digitalWrite(greenLED2, HIGH);
        digitalWrite(redLED2, LOW);
        relayOn1 = true;
    }
    
    // if relay is currently on...
    if( relayOn1 )
    {
        // turn red led on, if close to turning off the relay
        if (currentMillis - previousMillis >= interval-redLedInterval )
            digitalWrite(redLED2, (millis()/100)%2);//blink red led; 300ms on; 300ms off     
        // if enough time has elapsed, turn of the relay
        if (currentMillis - previousMillis >= interval) 
        {
            // .. turn off relay
            digitalWrite(relay1, HIGH);
            digitalWrite(greenLED2, LOW);
            digitalWrite(redLED2, LOW);
            relayOn1 = false;
        } 
    
    } 
     // if button is pressed, turn relay on (if it wasn't already on), and reset the timer
    if( buttonState2==HIGH ) // no need to check for previous state, in this specific case
    {
        previousMillis2 = currentMillis;
        digitalWrite(relay2, LOW);
        digitalWrite(greenLED3, HIGH);
        digitalWrite(redLED2, LOW);
        relayOn2 = true;
    }
    
    // if relay is currently on...
    if( relayOn2 )
    {
        // turn red led on, if close to turning off the relay
        if (currentMillis - previousMillis2 >= interval-redLedInterval )
            digitalWrite(redLED2, (millis()/100)%2);//blink red led; 300ms on; 300ms off     
        // if enough time has elapsed, turn of the relay
        if (currentMillis - previousMillis2 >= interval) 
        {
            // .. turn off relay
            digitalWrite(relay2, HIGH);
            digitalWrite(greenLED3, LOW);
            digitalWrite(redLED2, LOW);
            relayOn2 = false;
        } 
    
    }
        // if button is pressed, turn relay on (if it wasn't already on), and reset the timer
    if( buttonState3==HIGH ) // no need to check for previous state, in this specific case
    {
        previousMillis3 = currentMillis;
        digitalWrite(relay3, LOW);
        digitalWrite(greenLED4, HIGH);
        digitalWrite(redLED2, LOW);
        relayOn3 = true;
    }
    
    // if relay is currently on...
    if( relayOn3 )
    {
        // turn red led on, if close to turning off the relay
        if (currentMillis - previousMillis3 >= interval-redLedInterval )
            digitalWrite(redLED2, (millis()/100)%2);//blink red led; 300ms on; 300ms off     
        // if enough time has elapsed, turn of the relay
        if (currentMillis - previousMillis3 >= interval) 
        {
            // .. turn off relay
            digitalWrite(relay3, HIGH);
            digitalWrite(greenLED4, LOW);
            digitalWrite(redLED2, LOW);
            relayOn3 = false;
        } 
    
    } 
    
}

Your code is confusing to me. Sometimes you count 1, 2, 3 and sometimes it's 2, 3, 4. If it is confusing to read then it is a problem to write. I would start by getting your numbering consistent and see if that doesn't cause something to pop out.

Pro-tip: When you find yourself numbering variables, what you really want is an array. When you find yourself with several sets of different variables numbered together like this, then what you really need is a class. Don't have to do it that way, but man it sure would make it easier.

pinMode(BUTTON_PIN2, INPUT);
digitalWrite(BUTTON_PIN2, HIGH);

Those two lines should be:

pinMode(BUTTON_PIN2, INPUT_PULLUP); // button between pin and ground


pinMode(relay1, OUTPUT);
digitalWrite(relay1, HIGH);

Should be swapped:
// pull up BEFORE setting pin to output eliminates relay chatter during bootup
digitalWrite(relay1, HIGH);
pinMode(relay1, OUTPUT);


// if button is pressed,

if( buttonState2==HIGH)

should be:

if( buttonState2 == LOW) // pin is normally high, and goes low when button is pressed.


Fix all of that, and re-post.
Leo..

NEVERMIND!
IT WAS THE WIRING - THANKS FOR YOUR TIME!

Also, thank you for your replies. I just noticed them when I posted the previous comment and it refreshed the page.

I will take your feedback into consideration and try to learn from them. I'm still pretty new at this and my code is admittedly very shotgunned together.

Thank you though - I love the support this forum provides.

This is how the code would look using arrays. If you use the six analog input pins as digital pins you could add two more buttons, relays, and green LEDs very easily.

//CONSTANTS
const byte BUTTON_COUNT = 3;


const byte ButtonPins[BUTTON_COUNT] = {2, 3, 4};
const byte GreenLEDPins[BUTTON_COUNT] = {5, 6, 7};
const byte RelayPins[BUTTON_COUNT] = {9, 10, 11};


const int RedLEDPin = 8;




//VARIABLES
boolean ButtonStates[BUTTON_COUNT] = {HIGH, HIGH, HIGH};
boolean RelayOnStates[BUTTON_COUNT] = {false, false, false};


int lastButtonState = 0;       // previous state of the button


// MILLIS
unsigned long RelayStartTimes[BUTTON_COUNT] = {0, 0, 0};


const unsigned long interval = 3000;
const unsigned long redLedInterval = 1000;

void setup()
{
  Serial.begin(115200);  // No good reason to go any slower  :)


  for (byte i = 0; i < BUTTON_COUNT; i++)
  {
    pinMode(ButtonPins[i], INPUT_PULLUP);


    pinMode(GreenLEDPins[i], OUTPUT);


    //SWITCH HANDLERS
    digitalWrite(RelayPins[i], HIGH);  // Active Low
    pinMode(RelayPins[i], OUTPUT);
  }


  digitalWrite(RedLEDPin, LOW);
  pinMode(RedLEDPin, OUTPUT);
}


void loop()
{
  for (byte i = 0; i < BUTTON_COUNT; i++)
  {
    unsigned long currentMillis = millis();


    // read the pushbutton input pin:
    ButtonStates[i] = digitalRead(ButtonPins[i]);




    // if button is pressed, turn relay on (if it wasn't already on), and reset the timer
    if (ButtonStates[i] == LOW) // Active Low
    {
      RelayStartTimes[i] = currentMillis;
      digitalWrite(RelayPins[i], LOW);  // Active Low
      digitalWrite(GreenLEDPins[i], HIGH);
      digitalWrite(RedLEDPin, LOW);
      RelayOnStates[i] = true;
    }


    // if relay is currently on...
    if (RelayOnStates[i])
    {
      // turn red led on, if close to turning off the relay
      if (currentMillis - RelayStartTimes[i] >= interval - redLedInterval)
      {
        digitalWrite(RedLEDPin, (millis() / 100) % 2); //blink red led; 100ms on; 100ms off
      }


      // if enough time has elapsed, turn of the relay
      if (currentMillis - RelayStartTimes[i] >= interval)
      {
        // .. turn off relay
        digitalWrite(RelayPins[i], HIGH);  // Active Low
        digitalWrite(GreenLEDPins[i], LOW);
        digitalWrite(RedLEDPin, LOW);
        RelayOnStates[i]  = false;
      }
    } // End of if (RelayOnStates[i])
  } // End of BUTTON_COUNT loop
}

Note: You can get rid of the RelayOnStates array and just use the RelayStartTimes to mean the same thing. Set the start time to 0 when the timer expires. Then the RelayStartTimes will be ==0 (false) when the relay is off and !=0 (true) when the relay is on.