1

My project has 4 float switches, 2 valves and 3 pumps. I created a sketch and uploaded it. But my project keep all "OUTPUT" at "HIGH" all the time. I couldn't find what the problem, can you help?

This is my sketch:

#include <DS3231.h>
#include <Wire.h>
#include <LiquidCrystal.h>
DS3231  rtc(SDA, SCL); 

LiquidCrystal lcd(8, 9, 4, 5, 6, 7);
int x = 45;
int y = 47;
int z = 41;
int w = 43;

int WaterIn =  40;
int WaterOut = 42;

int Pump1 = 25;
int Pump2 = 24;
int Pump3 = 27;

void setup() 
{
  rtc.begin();
  lcd.begin(16,2);
  // Start the I2C interface
  Wire.begin();
  // Start the serial interface
  Serial.begin(9600);
  //rtc.setDOW(TUESDAY);     // Set Day-of-Week to SUNDAY
  //rtc.setTime(1, 15, 0);     // Set the time to 12:00:00 (24hr format)
  //rtc.setDate(6, 6, 2017);   // Month, Day, Year

 //Setup FLoat Switch Pins
  pinMode (x, INPUT);
  pinMode (y, INPUT);
  pinMode (z, INPUT);
  pinMode (w, INPUT);

 //Setup Irrigation Valves    
  pinMode(WaterIn, OUTPUT);  //irrigation "in" valve
  pinMode(WaterOut, OUTPUT);  //irrigation "out" valve

//Setup Pumps    
  pinMode(Pump1, OUTPUT);  
  pinMode(Pump2, OUTPUT);  
  pinMode(Pump3, OUTPUT);    
}  // end of setup

void Calendar(){

 lcd.setCursor(0,0);
 lcd.print("Real Time Clock  ");
 lcd.setCursor(0,1);
 lcd.print("Time: ");
 lcd.print(rtc.getTimeStr());
 delay(1000);
 lcd.setCursor(0,1);
 lcd.print("Date: ");
 lcd.print(rtc.getDateStr());
 delay(1000);
 lcd.setCursor(0,1);
 lcd.print("Day: ");
 lcd.print(rtc.getDOWStr());
 lcd.print("           ");
 delay(1000);
 lcd.setCursor(0,1);
 lcd.print("Temp: ");

 lcd.print(rtc.getTemp());
 lcd.print(" C");
 lcd.print("          ");
 delay(1000);
}  // end of setup  

void Valves(){
    int pinToTurnHigh = 24;
    if ((x == HIGH) && (y == HIGH) && (z == HIGH) && (w == LOW))  //Condition 1
    {
        pinToTurnHigh = WaterIn;
        pinToTurnHigh = Pump1;
        pinToTurnHigh = Pump2;
        pinToTurnHigh = Pump3;
    }       
    if ((x == LOW) && (y == LOW) && (z == HIGH) && (w == HIGH))  //Condition 2
    {
       pinToTurnHigh = WaterOut;
    } 
    resetAllPumpValve();
    digitalWrite(pinToTurnHigh, HIGH);
}  // end of setup

void resetAllPumpValve(){
     digitalWrite(WaterIn, LOW); 
     digitalWrite(Pump1, LOW); 
     digitalWrite(Pump2, LOW);
     digitalWrite(Pump3, LOW);
     digitalWrite(WaterOut, LOW);
}  // end of setup  
void loop()
{
    Calendar();
    Valves();
    resetAllPumpValve();  
}  // end of loop
  • 1
    use serial.print() to display debugging information on the serial console .... verify that the program flow is as you expect it to be – jsotola May 23 at 2:37
  • I'm new with arduino, can you make it clear? You mean replace lcd.print () to serial.print()? – user56886 May 23 at 3:38
  • No, rather, in addition to the lcd.print() commands. This way you can see on the serial monitor where your code is going during execution. delay() may also come in handy. – sa_leinad May 29 at 11:28
1

chrisl made an excellent analysis of your program and the problems therein. I would just want to add a point about how to handle the Calendar() function. You have been given the suggestion to look at the Blink Without Delay Arduino tutorial, and I second that suggestion. This is indeed the first thing you should do.

However, once you have studied that tutorial and you try to apply it to your specific problem, you will notice that yours is somewhat harder than blinking an LED. The reason it is harder is that you have to manage a more complex system state. The LED has only two states, and it periodically transitions between them as:

  • LOWHIGHLOW

Your display routine will instead have four states and make the following transitions:

  • SHOW_TIMESHOW_DATESHOW_DAYSHOW_TEMPSHOW_TIME

The relevant programming concept for dealing with this kind of system is called a “finite state machine” and I suggest you also study this finite state machine tutorial. This tutorial proposes a very generic switch/case control structure for programming an FSM. However, if there is some behavior common to all states (in your case: the fact that each one lasts for exactly one second), you can factor that behavior out of the switch/case construct.

Here is what I would propose as a non-blocking Calendar(). First, put this near the top of the program, along with the other constants:

const LCD_DISPLAY_TIME = 1000;  // display each item for 1,000 ms

Then, is setup(), right after initializing the LCD:

lcd.setCursor(0, 0);
lcd.print("Real Time Clock ");

As a general rule, setup() is the right place for anything that only needs to be done once, at the start of the program.

And finally, the Calendar() function to be called from loop():

void Calendar()
{
    // Do nothing unless it's time to refresh the display.
    static uint32_t last_change = -LCD_DISPLAY_TIME;
    if (millis() - last_change < LCD_DISPLAY_TIME) return;
    last_change += LCD_DISPLAY_TIME;

    // Switch to the next item: Time -> Date -> Day -> Temp -> Time
    lcd.setCursor(0, 1);
    static enum { SHOW_TEMP, SHOW_TIME, SHOW_DATE, SHOW_DAY } state;
    switch (state) {
    case SHOW_TEMP:
        lcd.print("Time: ");
        lcd.print(rtc.getTimeStr());
        state = SHOW_TIME;
        break;
    case SHOW_TIME:
        lcd.print("Date: ");
        lcd.print(rtc.getDateStr());
        state = SHOW_DATE;
        break;
    case SHOW_DATE:
        lcd.print("Day: ");
        lcd.print(rtc.getDOWStr());
        state = SHOW_DAY;
        break;
    case SHOW_DAY:
        lcd.print("Temp: ");
        lcd.print(rtc.getTemp());
        lcd.print(" C");
        state = SHOW_TEMP;
        break;
    }
    lcd.print("           ");  // pad
}
5

It is not clear, what the code actually should do (what behaviour you are trying to achieve), so I will only look into the obvious programming problems.

  1. The following declarations at the global scope seem to be pin numbers:

    int x = 45;
    int y = 47;
    int z = 41;
    int w = 43;
    
    int WaterIn =  40;
    int WaterOut = 42;
    
    int Pump1 = 25;
    int Pump2 = 24;
    int Pump3 = 27;
    

    But in the Valves() function yout treat them as they were representing the value of that pins. That is not the case. If you want to read the state of a pin, you cannot do

    if ((x == HIGH) && (y == HIGH) && (z == HIGH) && (w == LOW))
    

    You are just comparing the pin number with HIGH, which is defined as 1. That cannot work. You have to actually read the pin using it's pin number, which looks like this:

    if( digitalRead(x) == HIGH && digitalRead(y) == HIGH && digitalRead(z) == HIGH && digitalRead(w) == HIGH)
    

    And that can be shortened to:

    if( digitalRead(x) && digitalRead(y) && digitalRead(z) && digitalRead(w) )
    
  2. Inside of the mentioned if statements you set a variable multiple times without doing anything with it. Any consecutive write to this variable will overwrite the previous value. After the code block

    pinToTurnHigh = WaterIn;
    pinToTurnHigh = Pump1;
    pinToTurnHigh = Pump2;
    pinToTurnHigh = Pump3;
    

    The variable pinToTurnHigh is 27 (Pump3). The values, that you assigned right before that line, are lost. You actually have to do something with the variable, or the first 3 lines of that block makes no sense.

  3. In most cases it is not a good idea, to use variables with changing values to handle pin numbers, since that will make the code difficult to understand. The circuitry is mostly hardwired, it will not change. So there is no need to assign pin numbers to a changing variable. Instead you can directly do, what you need to do with a constant pin number. (Actually the code optimizer, that comes with the compiler, can optimize the not-changing pin-number variables out, so that you wrote them in your code, but the actually code might not have them.) Here it seems, that you want to write a HIGH value to all the mentioned pins. You can do that directly:

    digitalWrite(WaterIn, HIGH);
    digitalWrite(Pump1, HIGH);
    digitalWrite(Pump2, HIGH);
    digitalWrite(Pump3, HIGH);
    

    Here it is important to understand, that digitalWrite() is only working on 1 single pin at a time. If you want to set multiple pins with it, you have to call it multiple times with the according pin numbers.

  4. Now the timing of your code: The Calendar() function will block the rest of the code for about 4s, because you used that much of delay(). During that time the Arduino cannot react to anything. The switches will not do anything either (since the Arduino is busy waiting in the delay() instead of checking the switches). The code, that is called after the Calendar() function, contains no delay()s, so it will run very fast. In the Valves() function you first reset all the pumps and valves to LOW. Then you write HIGH to one pin (as mentioned above I assume, that you wanted to write them all on condition). After that the program will go back to the loop() function and execute the resetAllPumpValve() function again, which will set all the outputs to LOW.

    All in all that means, that the Arduino is fiddling thumbs inside the Calendar() function, without doing anything else. Then he will reset, set and reset the pins again. Since there is not timing code in there, this will happen very fast. So the pins have to be LOW most of the time and being turned on about every 4s for a tiny amount of time (a low number of milliseconds, since that is about the time to execute digitalWrite() or it's siblings). The motors or values will hardly have time to move with that.

    For solving this you might want to look into the BlinkWithoutDelay example, that comes with the Arduino IDE. It shows a non-blocking coding style, that will only do something, if it is the time to do it. Much like when you bake a pizza. You will not sit in front of the oven and wait patiently, until the pizza is fully baked. You will put it into the oven and regularly (maybe every 5 min) look, if the pizza is ready to eat. In the meantime you would do other things. Thats what is done there by using the millis() function, that will return the number of milliseconds passed since the programs startup. You can look into the example and google about it to learn more. It has been covered numerous times throughout the internet.

That's it for now. To really help you with the logic of the program, you will first have to get these parts right.

Have fun while learning :-)

  • 1
    Outstanding analysis of the problems with that code. (voted) I got part-way through it and threw up my hands. I couldn't make much sense of what it was supposed to do, and saw so many problems that gave up trying to call them all out. You were very thorough and patient, and provided clear explanations. Huzzah! – Duncan C May 23 at 22:44
1

I would add to Chrisl's answer by saying that, if you wanted to make the code more compact, you could represent the 4 boolean variables by a single byte,

e.g. w HIGH + x HIGH + y LOW + z LOW = 1100.
e.g. w HIGH + x HIGH + y LOW + z HIGH = 1101.

Then it would be easier to implement a switch statement instead of if( digitalRead(x) == HIGH && digitalRead(y) == HIGH && digitalRead(z) == HIGH && digitalRead(w) == HIGH) etc.

For example:

byte state;

void setup(){}

void loop(){
 // insert code to read state
 switch (state)
 {
     case 1: 
         // insert code to be run if state = 0b0001;
     break;

     case 13: 
         // insert code to be run if state = 0b1101;
     break;

     default: // code to be run if state doesn't match any cases
 }
 // insert other code
}

If you were willing to use the ATmega's digital 30-37 pins then you could use the AVR's port command PINC to read all of the HIGH/LOW states at once, using a single instruction. I leave this as an exercise for you to try!

  • This is likely to make the program harder to read and understand. There are situations where your suggestion would make the program more readable, e.g. if all 16 cases have to be dealt with and there is no easy way to factor cases. But if there are only two or three cases to handle, if/else is likely to be both more compact and more readable. – Edgar Bonet May 23 at 8:14
  • 1
    Very true, but I wrote "if you wanted to make the code more compact", not necessarily readable. – MichaelT May 23 at 8:16
  • I fixed the Calendar(), it works perfect. – user56886 May 24 at 0:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.