I'm starting to feel like I have made a Rube Goldberg device

Projects Programming
1

I am trying to automate all of the water handling functions of my aquarium (yes another person trying to make an aquarium controller). It should be able to keep my filtered freshwater reservoir full, keep my sump topped off from the freshwater reservoir, and perform a complete water change which consists of filling a saltwater mixing reservoir, notifying the user to add salt, mixing for a specified period of time, topping off the sump before draining it, draining the sump, and then pumping in the new saltwater. It all makes perfect sense in my head but I am starting to feel like it is becoming a mess. I would love to hear some critique of my code. Here it is in all of its glory:

#define On HIGH
#define Off LOW

boolean freshwater_fill_flag = false; //freshwater is topping off flag    `
boolean saltwater_fill_flag = false;
boolean sump_fill_flag = false; //sump needs topping off flag
boolean water_change_flag = false; //water change in process
boolean add_salt_flag = false; //add salt request sent to user..wait for flag to be cleared
boolean mix_saltwater_flag = false; //saltwater mixing in process
boolean drain_sump_flag = false; //sump draining in process
boolean fill_sump_from_reservoir_flag = false; //sump filling in process
int wc_stage = 0;
const int freshwater_high_float_switch = 2;
const int freshwater_fill_float_switch = 3;
const int freshwater_low_float_switch = 4;
const int saltwater_high_float_switch = 5;
const int saltwater_low_float_switch = 6;
const int sump_high_float_switch = 7;
const int sump_fill_float_switch = 8;
const int sump_low_float_switch = 9;
const int salt_added_button = 10;
const int fresh_to_sump_pump = 11;
const int fresh_to_salt_pump = 12;
const int sump_to_drain_pump = 13;
const int salt_to_sump_pump = 14;
const int freshwater_relay_valve = 15;
const int saltwater_mixer = 16;
unsigned long mixTime;



void setup()
{
  
  
  Serial.begin(115200);
  //set D2 through D10 as input with weak pullup enabled
  for(int i = 2; i<11; i++)
  {
    pinMode(i,INPUT);
    digitalWrite(i,On);
  }
  //set D11 through D16 as OUTPUTS and initialize to logic high
  //which turns the relay off...logic low turns the relay on
  for(int i = 11; i<17; i++)
  {
    pinMode(i,OUTPUT);
    digitalWrite(i, Off);
  }
  
}

void loop()
{
  freshwater_level();
  sump_level();
  water_change();
}



void freshwater_level()
{
  //if freshwater fill float switch is logic low and if freshwater high 
  //float switch is logic low and if freshwater filling flag is not set 
  //then set it and turn the water filter on

  if(digitalRead(freshwater_fill_float_switch) == false && 
     freshwater_fill_flag == false && 
     digitalRead(freshwater_high_float_switch) == false)
     {
       freshwater_fill_flag = true;
       digitalWrite(freshwater_relay_valve, On); //turns the water on
       Serial.println("fill flag = true");
       Serial.println("freshwater relay = on");
     }
  
  //if freshwater high float switch is logic high and if freshwater
  //filling flag is set, then freshwater filling flag is false and turn the 
  //water filter off
  
  if(digitalRead(freshwater_high_float_switch) == true && 
     freshwater_fill_flag == true)
  {
    freshwater_fill_flag = false;
    digitalWrite(freshwater_relay_valve, Off); //turns the water off
    Serial.println("fill flag = false");
    Serial.println("relay is off");
  }
  
}

void sump_level()
{
  //if sump fill float switch is logic low and if sump high 
  //float switch is logic low and if sump filling flag is not set 
  //then set it and turn the freshwater topoff pump on.  
  //However, if the freshwater low float switch is logic low, 
  //then shut the pump off until the freshwater is replenished 
  //to avoid burning it up
  
  if(digitalRead(freshwater_low_float_switch) == false && 
           sump_fill_flag == true)
          {
            digitalWrite(fresh_to_sump_pump, Off);
            sump_fill_flag == false;
          }
          

  if(digitalRead(sump_fill_float_switch) == false && 
     sump_fill_flag == false && 
     digitalRead(sump_high_float_switch) == false &&
     digitalRead(freshwater_low_float_switch) == true && 
     drain_sump_flag == false) //drain sump flag suppresses this function if
                               //the sump is low as a result of a water change
                               //that is in progress
     {
       sump_fill_flag = true;
       digitalWrite(fresh_to_sump_pump, On); //turns the water on
     }
  
  //if sump high float switch is logic high and if sump
  //filling flag is set, then filling flag is false and turn the 
  //sump freshwater topoff pump off
  
  if(digitalRead(sump_high_float_switch) == true && 
     sump_fill_flag == true)
  {
    sump_fill_flag = false;
    digitalWrite(fresh_to_sump_pump, Off); //turns the water off
  }
  
}


void water_change()
{
  
  if(water_change_flag == true)
  {
    switch(wc_stage)
    {
      case 0: //top off freshwater reservoir
       if(freshwater_fill_flag == false && 
       digitalRead(freshwater_high_float_switch) == false)
        {
          freshwater_fill_flag = true;
          digitalWrite(freshwater_relay_valve, On); //turns the water on
        }
        
        if(digitalRead(freshwater_high_float_switch) == true)
        {
          wc_stage = 1;
        }
        break;
        
      case 1: //fill saltwater reservoir
        if(digitalRead(freshwater_low_float_switch) == false && 
         saltwater_fill_flag == true)
        {
          digitalWrite(fresh_to_salt_pump, Off);
          saltwater_fill_flag == false;
          break;
        }
        if(digitalRead(saltwater_high_float_switch) == false &&
        saltwater_fill_flag == false && 
        digitalRead(freshwater_low_float_switch) == true)
        {
          saltwater_fill_flag == true;
          digitalWrite(fresh_to_salt_pump, On);
        }
        if(digitalRead(saltwater_high_float_switch) == true)
        {
          saltwater_fill_flag == false;
          digitalWrite(fresh_to_salt_pump, Off);
          wc_stage = 2;
        }
        break;
        
      case 2: //add salt
        if(add_salt_flag == false)
        { 
          Serial.println("Add Salt = True");
          add_salt_flag = true;
        }
        if(digitalRead(salt_added_button) == false) //push when salt is added
        {
          add_salt_flag = false;
          wc_stage = 3;
        }
        break;
        
      case 3: //mix saltwater
        if(mix_saltwater_flag == false)
        {
          mix_saltwater_flag = true;
          mixTime = millis() + 600000;
          digitalWrite(saltwater_mixer, On);
        }
        
        if(timer(mixTime))
        {
          mix_saltwater_flag = false;
          digitalWrite(saltwater_mixer, Off);
          wc_stage = 4;
        }
        break;
        
      case 4: //top off sump
        if(digitalRead(sump_high_float_switch) == false && 
         sump_fill_flag == false && 
         digitalRead(freshwater_low_float_switch) == true)
        {
          sump_fill_flag = true;
          digitalWrite(fresh_to_sump_pump, On);
        }
        
        if(digitalRead(sump_high_float_switch) == true)
        {
          wc_stage = 5;
        }
        break;
        
      case 5: //drain sump
        if(drain_sump_flag == false) 
        {
          drain_sump_flag == true;
          digitalWrite(sump_to_drain_pump, On);
        }
        if(digitalRead(sump_low_float_switch) == false)
        {
          digitalWrite(sump_to_drain_pump, Off);
          wc_stage = 6;
        }
        break;
        
      case 6: //fill sump but don't let the saltwater reservoir run dry
        if(fill_sump_from_reservoir_flag == true &&
        digitalRead(saltwater_low_float_switch == false))
        {
          fill_sump_from_reservoir_flag = false;
          digitalWrite(salt_to_sump_pump, Off);
        }
        
        if(fill_sump_from_reservoir_flag == false && 
        digitalRead(saltwater_low_float_switch == true))
        {
          fill_sump_from_reservoir_flag = true;
          digitalWrite(salt_to_sump_pump, On);
        }
        
        if(digitalRead(sump_high_float_switch) == true)
        {
          digitalWrite(salt_to_sump_pump, Off);
          drain_sump_flag = false;
          wc_stage = 7;
        }
        break;
        
      case 7: //clean up
        //send message to user notifying them of a completed water change
        wc_stage = 0;
        water_change_flag = false;
        
    }
  }
  
  
}

//keep track of time and handle millis() rollover
boolean timer(unsigned long timeout)
  {
    return (long)(millis() - timeout) >= 0;
  }
2 
    digitalWrite(i,On);
  }
  //set D11 through D16 as OUTPUTS and initialize to logic high
  //which turns the relay off...logic low turns the relay on
  for(int i = 11; i<17; i++)
  {
    pinMode(i,OUTPUT);
    digitalWrite(i, Off);

Most of us are used to seeing HIGH and LOW, and knowing instinctively what they mean. You'd do well to do the same.

  if(digitalRead(freshwater_fill_float_switch) == false &&

The digitalRead() function does not return true or false. It returns HIGH or LOW. Yes, I know that numerically the values are the same, but there are conventions for a reason. You'd do well to adopt those conventions.

If the switch actually reads LOW when the tank is low, then using HIGH or LOW actually makes a lot more sense. If not, you might consider rewiring the switches to incorporate external pulldown resistors, so that LOW means the tank level is low and HIGH means that the tank level is high.

You might also consider storing the value read from the switch into a local variable, so that digitalRead(freshwater_high_float_switch) doesn't have to be called (or typed) so many times. After all, during any one iteration of the function, how likely is the state to change? And, even if it did, how long will it be until you check again?

       Serial.println("fill flag = true");

Rather than testing fill flag, and printing out its state, why not just print its state?

The code is well commented, and not all that difficult to follow, once one gets used to your not following conventions. The Tools + Auto Format menu item would fix the uneven indenting and make the code easier to read. Deleting (some of) the white space IN functions would, too. I hate having to scroll so much to read, when all I'm scrolling past is white space.

The == true parts are not needed. The value IS true and matches true or it isn't and doesn't. The == false stuff isn't either. Just add a ! in front of the comparison.

        if(digitalRead(sump_high_float_switch) == false && 
         sump_fill_flag == false && 
         digitalRead(freshwater_low_float_switch) == true)

could be

        if(!digitalRead(sump_high_float_switch) && 
         !sump_fill_flag && 
         digitalRead(freshwater_low_float_switch))
3

Thanks PaulS! Exactly the kind of advice that I love to get. I will clean up my code with all of your suggestions. The only one that I am hesitant on is your first suggestion. I defined "On" and "Off" at the beginning of the sketch to be "HIGH" and "LOW" respectively because I unfortunately purchased two different types of relay boards. One type turns the relay on with an active high signal and the other type turns the relay on with an active low signal. I wanted to make it simple to switch between the two if need be by simply changing the #define at the beginning of the sketch rather than having to go through the entire sketch and change all the HIGH's to LOW's and LOW's to HIGH's. I guess I could use a find and replace if I needed to. Thoughts?

4

The only one that I am hesitant on is your first suggestion. I defined "On" and "Off" at the beginning of the sketch to be "HIGH" and "LOW" respectively because I unfortunately purchased two different types of relay boards. One type turns the relay on with an active high signal and the other type turns the relay on with an active low signal.

So, one relay turns on with On, and the other turns on with Off. Well, that will be easy to follow in the code.

I suggest that you define RelayTypeOneOn, RelayTypeTwoOn, RelayTypeOneOff, and RelayTypeTwoOff, and use them in place of Off and On. The switches, then, have nothing to to with On or Off.

5

Not sure I understand. Do you mean something like this?

#define RelayTypeOneOn HIGH
#define RelayTypeTwoOn LOW
#define RelayTypeOneOff LOW
#define RelayTypeTwoOff HIGH

const int relayPin = 3

void setup()
{
     digitalWrite(relayPin, RelayTypeOneOn);

}
6

I agree with the comments made by PaulS. In addition:

#define On HIGH
#define Off LOW

You are using On and Off not only for your relay control, but also to control enabling the pullup resistors on the inputs, salt water control etc. Use:

#define RelayOn LOW
#define RelayOff HIGH

or the other way round, depending on the which relays you use. Then use RelayOn and RelayOff only in conjunction with the relay output pins, and use HIGH and LOW everywhere else (or define other on/off constants for particular input and output devices, if you want).

    pinMode(i,INPUT);
    digitalWrite(i,On);

Simpler to use:

   pinMode(i, INPUT_PULLUP);

instead.

  //set D11 through D16 as OUTPUTS and initialize to logic high
  //which turns the relay off...logic low turns the relay on
  for(int i = 11; i<17; i++)
  {
    pinMode(i,OUTPUT);
    digitalWrite(i, Off);
  }

Better to write the data before setting the pin to be an output, to avoid getting a negative going glitch when using active-low relays:

  //set D11 through D16 as OUTPUTS and initialize to logic high
  //which turns the relay off...logic low turns the relay on
  for(int i = 11; i<17; i++)
  {
    digitalWrite(i, RelayOff);
    pinMode(i,OUTPUT);
  }
  1. Change:
Serial.println("Add Salt = True")

to:

Serial.println(F("Add Salt = True"))

and similarly everywhere else you print a string literal. It saves RAM, which is precious on an Arduino.

7

Do you mean something like this?

The first part is correct. The second part is questionable. Not wrong, just questionable.

Questionable in the sense that relayPin doesn't way anything about the type of relay it controls, so one doesn't know whether to use type one or type two constants.

8

dc42:
2.

    pinMode(i,INPUT);

digitalWrite(i,On);




Simpler to use:

pinMode(i, INPUT_PULLUP);




instead.

Cool! I didn't know that you could do that. Good to know.

  //set D11 through D16 as OUTPUTS and initialize to logic high

//which turns the relay off...logic low turns the relay on
  for(int i = 11; i<17; i++)
  {
    pinMode(i,OUTPUT);
    digitalWrite(i, Off);
  }




Better to write the data before setting the pin to be an output, to avoid getting a negative going glitch when using active-low relays:

//set D11 through D16 as OUTPUTS and initialize to logic high
  //which turns the relay off...logic low turns the relay on
  for(int i = 11; i<17; i++)
  {
    digitalWrite(i, RelayOff);
    pinMode(i,OUTPUT);
  }

I remember from coding on AVR Studio that this was appropriate because you want to initialize the pins to a known state before exposing them by setting the data direction register. However, it wasn't working on the arduino so I posted a question on this forum and received a reply that suggested that pinMode initializes the pins to LOW regardless of the state that they were in prior to calling pinMode. It is under reply #1 on this thread:

Am I still missing something?

  1. Change:
Serial.println("Add Salt = True")

to:

Serial.println(F("Add Salt = True"))

and similarly everywhere else you print a string literal. It saves RAM, which is precious on an Arduino.

I have just started noticing the println(F(...)). Is this just an alternative to using PROGMEM....not that I have ever even used it? I could be way off here.

9

jerseyguy1996:
I remember from coding on AVR Studio that this was appropriate because you want to initialize the pins to a known state before exposing them by setting the data direction register. However, it wasn't working on the arduino so I posted a question on this forum and received a reply that suggested that pinMode initializes the pins to LOW regardless of the state that they were in prior to calling pinMode. It is under reply #1 on this thread:

Confusion using digitalRead and internal pull up resistors - #5 by jerseyguy1996 - Programming Questions - Arduino Forum

Am I still missing something?

I've checked the Arduiono 1.01 pinMode() code. It does not write to the data register when you set the mode to OUTPUT. So it is safe to set the state before setting the pin mode to OUTPUT. I have been doing this in my own programs for some time.

jerseyguy1996:
I have just started noticing the println(F(...)). Is this just an alternative to using PROGMEM....not that I have ever even used it? I could be way off here.

Yes, it's an easy way of using PROGMEM to hold string literals that are used only as arguments to print() and println() functions.