0
int value1;
int value2;
int runs1 = 0;
int runs2 = 0;

#include <Wire.h>
#include <LiquidCrystal_I2C.h>
LiquidCrystal_I2C lcd(0x3F, 16, 2);
int inPin = 6;
int val = 0; 
int push_count = 0;
int buttonState = 0; 
unsigned long previousMillis = 0; 
const long interval = 1800000; 
int currentTime;

void setup() {
  Serial.begin(9600);
  lcd.begin();
  pinMode(inPin, INPUT);
}

void loop() {

  unsigned long currentMillis = millis();
  currentTime = 30-(currentMillis - previousMillis)/60000;
  if (currentMillis - previousMillis >= interval || previousMillis == 0) { 
    previousMillis = currentMillis;

  measure();
  }

  buttons();
}

void measure() {
 value1=300;
 value2=300;

  if (value1 <= 650) 
{
   lcd.setCursor(8,0);
   lcd.print("Plant");
   lcd.setCursor(10,1);
   lcd.print("1");
   delay(2000);

}
else
{
  lcd.clear();
  lcd.print("Pump 1 turned ON"); 
  lcd.setCursor(0,1);
  lcd.print("Water for 40 sec"); 
  runs1++; // increment the number of times pump 1 has run

}

  if (value2 <= 550)
{
   lcd.setCursor(8,0);
   lcd.print("Plant");
   lcd.setCursor(10,1);
   lcd.print("2");
   delay(2000);
   Serial. print("Pump 2 OK");
}
else
{
  lcd.clear();
  lcd.print("Pump 2 turned ON"); 
  lcd.setCursor(0,1);
  lcd.print("Water for 40 sec"); 
  runs2++; // increment the number of times pump 2 has run
  Serial. print("Pump 2");
}  
  }

void buttons() {
  val = digitalRead(inPin); 

  if (val != buttonState && val == HIGH){ 
    push_count++; 
  }
      else {
      // do nothing
    }

  buttonState = val; 
  switch (push_count) {
    case 0:
      lcd.clear();
      lcd.setCursor(0,0);    
      lcd.print("Sensor 1 reads");
      lcd.setCursor(0,1);
      lcd.print(value1); 
      delay(20);
      Serial. print("Case 0");
      push_count++;
      break;
    case 1:
      Serial. print("Case 1");
      break;            
    case 2: 
      lcd.clear();
      lcd.setCursor(0,0);  
      lcd.print("Sensor 2 reads");
      lcd.setCursor(0,1);
      lcd.print(value2); 
      delay(10);
      push_count++;
      break;
    case 3:   
      break; 
    case 4:   
      lcd.clear();
      lcd.setCursor(0,0);
      lcd.print("Pump 1 ran");
      lcd.setCursor(0,1);
      lcd.print(runs1);
      lcd.setCursor(5,1);
      lcd.print(" times");
      delay(10);
      push_count++;
      break;
    case 5:   
      break; 
    case 6:   
      lcd.clear();
      lcd.setCursor(0,0);
      lcd.print("Pump 2 ran");
      lcd.setCursor(0,1);
      lcd.print(runs2);
      lcd.setCursor(5,1);;
      lcd.print(" times");
      delay(10);
      push_count++;
      break;
    case 7:   
      break; 
    case 8:   
      lcd.clear();
      lcd.setCursor(0,0);
      lcd.print("Next cycle in");
      lcd.setCursor(0,1);
      lcd.print(currentTime);
      lcd.setCursor(5,1);
      lcd.print(" Minutes");
      delay(10);
      push_count++;
      break;
    case 9:   
      break;      
    default:
      push_count=0;  
  }
}

When I boot the system (standalone atmega328p circuit) the connected LCD either shows

Plant

2

originating from

   lcd.setCursor(8,0);
   lcd.print("Plant");
   lcd.setCursor(10,1);
   lcd.print("2");
   delay(2000);
   Serial. print("Pump 2 OK");

or in my real code (where value1 and value2 are actually measured)

Pump 2 turned ON

Water for 40 sec

originating from

  lcd.clear();
  lcd.print("Pump 2 turned ON"); 
  lcd.setCursor(0,1);
  lcd.print("Water for 40 sec"); 
  runs2++; // increment the number of times pump 2 has run
  Serial. print("Pump 2");

after I use the button to cycle through all the cases everything works fine. On the next button press I get to case 2 (-> 3). After a full cycle case 0 (-> 1) gets displayed correctly.

The above MCVE runs perfectly on my breadboard setup, but not in the real setup.

Here's the schematic of the real setup.

So somewhere in the real setup has to be something that my code does not handle properly. The breadboard setup is minimal, just all GND and VCC connections, a 16 Mhz crystal with 22 pF caps.

enter image description here

  • Consider writing an MCVE – Dmitry Grigoryev Mar 13 '18 at 8:06
  • @DmitryGrigoryev I wasn't sure whether I should do that, I didn't want to delete the faulty part of the code by accident. See my edit, I guess I have to do some further testing tonight. I use pull down resistors in my atmega setup for the button. – idkfa Mar 13 '18 at 8:27
  • The "V" part of MCVE means Verifiable – Test the code you're about to provide to make sure it reproduces the problem – jose can u c Mar 13 '18 at 15:33
  • @josecanuc For some reason I can't reproduce the error on the arduino or the breadboard. Only in my real system. I know that this makes finding the root cause for people here almost impossible. – idkfa Mar 13 '18 at 17:33
  • Intermittent errors are likely to be undefined values. That could be software uninitialized variables, or hardware: floating pins. You may also be dealing with bouncing button contacts, as noted in one answer. – jose can u c Mar 13 '18 at 17:42
1
int buttonState = 0;

So buttonState is initially zero (LOW).

  val = digitalRead(inPin); 

  if (val != buttonState && val == HIGH){ 
    push_count++; 
  }

Since you have an external pull-up on inPin it will read HIGH initially. Thus push_count will be incremented immediately.

  switch (push_count) {
    case 0:
      lcd.clear();
      lcd.setCursor(0,0);    
      lcd.print("Sensor 1 reads");
      lcd.setCursor(0,1);
      lcd.print(value1); 
      delay(20);
      Serial. print("Case 0");
      push_count++;
      break;
    case 1:
      Serial. print("Case 1");
      break;            

Therefore you execute case 1 initially.


Also, I don't see how this works:

 value1=300;
 value2=300;

  if (value1 <= 650) 
{
   lcd.setCursor(8,0);
   lcd.print("Plant");
   lcd.setCursor(10,1);
   lcd.print("1");immediately
   delay(2000);

}
else
{
  lcd.clear();
  lcd.print("Pump 1 turned ON"); 
  lcd.setCursor(0,1);
  lcd.print("Water for 40 sec"); 
  runs1++; // increment the number of times pump 1 has run

}

  if (value2 <= 550)
{
   lcd.setCursor(8,0);
   lcd.print("Plant");
   lcd.setCursor(10,1);
   lcd.print("2");
   delay(2000);
   Serial. print("Pump 2 OK");
}
else
{
  lcd.clear();
  lcd.print("Pump 2 turned ON"); 
  lcd.setCursor(0,1);
  lcd.print("Water for 40 sec"); 
  runs2++; // increment the number of times pump 2 has run
  Serial. print("Pump 2");
}  

Since you are doing both if tests it seems likely (indeed certain) that you will display "Plant 1" and then immediately* overwrite it with "Plant 2" so quickly that you wouldn't notice it.

* apart from the 2-second delay of course. :)

  • Luckily this project is one of my first so everything is still in prototype stage, i.e. a lot of jumper cables so this was an easy fix. So actually my code progressed the cases on release, not on press. To answer the second part of your answer ( ;) ) this is intended. It's just a visualization if you happen to watch the display during measurement. It will display "reading sensors" before. I also got a boot animation which I probably won't ever see again once the system doesn't need changes anymore. – idkfa Mar 13 '18 at 22:39
2

Personally I don't like the way you handle the "state machine", and I'd implement it in what I perceive is a more robust way (I may be wrong, though).

Moreover if you have a button please add a debounce; I usually do this with the Bounce2 library (or alternatives I code which are modified versions of that library). So... Let's change the things in the way I prefer.

// Include the library
#include <Bounce2.h>

// declare a bounce object
Bounce buttonDebounce = Bounce();

//in the setup:
pinMode(IN_PIN, INPUT); // or INPUT_PULLUP?
buttonDebounce.attach(IN_PIN);
buttonDebounce.interval(5);

//in the loop:
buttonDebounce.update();

Then in the loop you can use the buttonDebounce.rose() function or buttonDebounce.fell() to detect the button presses.

Now, for the state machine. You have four meaningful states (let's call them DISPLAY_1 to 4). It's better to give them more meaningful names. And let's add an invalid state. I usually prefer to use const bytes to force the size rather than enums or defines, but.. whatever ;)

So outside loop or setup declare the values and the state variable:

const byte SM_ErrorState = 0;
const byte SM_Display_1 = 1;
const byte SM_Display_2 = 2;
const byte SM_Display_3 = 3;
const byte SM_Display_4 = 4;
const byte SM_Display_5 = 5;
byte SM_currentState;

Now, the function buttons can be implemented in the classical way I learned to implement state machines, which is first make a switch case to test whether you have to advance, then advance to the next state if necessary and perform the entry action.

void buttons()
{
    buttonDebounce.update();

    nextState = SM_currentState;

    // Check if a new state is requested
    switch (SM_currentState)
    {
    case SM_Display_1:
        if (buttonDebounce.rose())
            nextState = SM_Display_2;
        break;
    case SM_Display_2:
        if (buttonDebounce.rose())
            nextState = SM_Display_3;
        break;
    case SM_Display_3:
        if (buttonDebounce.rose())
            nextState = SM_Display_4;
        break;
    case SM_Display_4:
        if (buttonDebounce.rose())
            nextState = SM_Display_5;
        break;
    case SM_Display_5:
        if (buttonDebounce.rose())
            nextState = SM_Display_1;
        break;
    default:
        nextState = SM_Display_1;
        break;
    }

    // If an advancement is requested, advance and then perform the entry action
    if (nextState != SM_currentState)
    {
        SM_currentState = nextState;
        switch (SM_currentState)
        {
        case SM_Display_1:
            lcd.clear();
            lcd.setCursor(0,0);    
            lcd.print("Sensor 1 reads");
            lcd.setCursor(0,1);
            lcd.print(value1); 
            delay(20);
            Serial. print("Case 0");
            break;
        case SM_Display_2:
            lcd.clear();
            lcd.setCursor(0,0);  
            lcd.print("Sensor 2 reads");
            lcd.setCursor(0,1);
            lcd.print(value2); 
            delay(10);
            break;
        case SM_Display_3:
            lcd.clear();
            lcd.setCursor(0,0);
            lcd.print("Pump 1 ran");
            lcd.setCursor(0,1);
            lcd.setCursor(5,1);
            lcd.print(runs1);
            lcd.print(" times");
            delay(10);
            break;
        case SM_Display_4:
            lcd.clear();
            lcd.setCursor(0,0);
            lcd.print("Pump 2 ran");
            lcd.setCursor(0,1);
            lcd.print(runs2);
            lcd.setCursor(5,1);;
            lcd.print(" times");
            delay(10);
            break;
        case SM_Display_5:
            lcd.clear();
            lcd.setCursor(0,0);
            lcd.print("Next cycle in");
            lcd.setCursor(0,1);
            lcd.print(currentTime);
            lcd.setCursor(5,1);
            lcd.print(" Minutes");
            delay(10);
            break;
        default:
            break;
        }
    }
}

Now, the initialization. Remember that I told you to add an error state? This way you can force a state change on initialization by setting the variable to the SM_ErrorState value.

Then, the millis management in the loop. I'd avoid the particular case of the variable equal to 0 to trigger an immediate execution, since this will lead to errors later. Then I also prefer to sum the interval, to avoid having a drift. So I changed your way to another one. I also changed some code for better readability (for instance, letting the "timer" reset every minute rather than every 30 minutes, and changing the defines accordingly).

Here is my complete code; I left out the measure function since it does not impact the other code.

Let me know if this solves your problem. Oh, BTW, UNTESTED CODE, so some bugs may appear...

#include <Wire.h>
#include <LiquidCrystal_I2C.h>
#include <Bounce2.h>

LiquidCrystal_I2C lcd(0x3F, 16, 2);

int value1; // I think it is better if these
int value2; // two variables are initialized
int runs1 = 0; // Unsure if this is needed
int runs2 = 0; // Unsure if this is needed
int val = 0; // Unsure if this is needed

int inPin = 6;
unsigned long previousMillis = 0;
const unsigned int millisInOneMinute = 60000U;

byte elapsedMinutes;
const byte intervalMinutes = 30;

Bounce buttonDebounce = Bounce();

void setup() {
    Serial.begin(9600);
    lcd.begin();
    pinMode(inPin, INPUT);
    buttonDebounce.attach(inPin);
    buttonDebounce.interval(5);
    previousMillis = millis();
    elapsedMinutes = intervalMinutes; // Force a new measure
}

void loop() {
    if (((unsigned int)(millis() - previousMillis)) >= millisInOneMinute)
    {
        previousMillis += millisInOneMinute; // Avoid drifting
        elapsedMinutes++;
    }

    if (elapsedMinutes >= intervalMinutes)
    {
        elapsedMinutes = 0;
        measure();
    }

    buttons();
}

void measure() {
    // do your stuff
}

void buttons()
{
    buttonDebounce.update();

    nextState = SM_currentState;

    switch (SM_currentState)
    {
    case SM_Display_1:
        if (buttonDebounce.rose())
            nextState = SM_Display_2;
        break;
    case SM_Display_2:
        if (buttonDebounce.rose())
            nextState = SM_Display_3;
        break;
    case SM_Display_3:
        if (buttonDebounce.rose())
            nextState = SM_Display_4;
        break;
    case SM_Display_4:
        if (buttonDebounce.rose())
            nextState = SM_Display_5;
        break;
    case SM_Display_5:
        if (buttonDebounce.rose())
            nextState = SM_Display_1;
        break;
    default:
        nextState = SM_Display_1;
        break;
    }

    if (nextState != SM_currentState)
    {
        SM_currentState = nextState;
        switch (SM_currentState)
        {
        case SM_Display_1:
            lcd.clear();
            lcd.setCursor(0,0);    
            lcd.print("Sensor 1 reads");
            lcd.setCursor(0,1);
            lcd.print(value1); 
            delay(20);
            Serial. print("Case 0");
            break;
        case SM_Display_2:
            lcd.clear();
            lcd.setCursor(0,0);  
            lcd.print("Sensor 2 reads");
            lcd.setCursor(0,1);
            lcd.print(value2); 
            delay(10);
            break;
        case SM_Display_3:
            lcd.clear();
            lcd.setCursor(0,0);
            lcd.print("Pump 1 ran");
            lcd.setCursor(0,1);
            lcd.setCursor(5,1);
            lcd.print(runs1);
            lcd.print(" times");
            delay(10);
            break;
        case SM_Display_4:
            lcd.clear();
            lcd.setCursor(0,0);
            lcd.print("Pump 2 ran");
            lcd.setCursor(0,1);
            lcd.print(runs2);
            lcd.setCursor(5,1);;
            lcd.print(" times");
            delay(10);
            break;
        case SM_Display_5:
            lcd.clear();
            lcd.setCursor(0,0);
            lcd.print("Next cycle in");
            lcd.setCursor(0,1);
            lcd.print(elapsedMinutes);
            lcd.setCursor(5,1);
            lcd.print(" Minutes");
            delay(10);
            break;
        default:
            break;
        }
    }
}

EDIT: Generic implementation of a state machine

Ok, here I post my generic scheme for a state machine. Usually I use the "state-based" state machine, which is actions are linked to states; sometimes I also saw "transition-based" state machines, where actions were linked to transitions, but... Ok, I'm not a real expert, so I don't want to write a lot of (wrong) things.

In this kind of machines, each state can have up to three kind of actions and a check:

  • actions you perform when you ENTER the state
  • actions you perform continuously when you are INSIDE the state
  • actions you perform when you EXIT the state
  • and of course, you have to CHECK whether you need to make a transition to another state

For instance, when you have in a microwave a state corresponding to the ON state, you have to:

  • when you ENTER the state, turn on the heater
  • when you are INSIDE the state, update the timer value
  • when you are done and you are EXITing the state, turn off the heater
  • when during the CHECK you see that the timer has expired or the stop button was pressed, you switch to another state

So there are four kind of actions: ENTER, INSIDE, EXIT and CHECK. For linearity I learned that it is much better to write each of them in a separate switch case. This is my usual implementation scheme:

void stateMachine()
{
    byte nextState = currState; // currState of course is defined outside

    switch (currState)
    {
    // Here you have to CHECK, for each state, the conditions
    // needed to switch to another state; if you switch, put
    // the new state in nextState
    }

    if (nextState != currState)
    {
        switch (currState)
        {
        // Perform the EXIT functions for each state
        }

        // Advance to the next state
        currState = nextState;

        switch (currState)
        {
        // Perform the ENTER functions for each state
        }
    }
    else // See note below
    {
        switch (currState)
        {
        // Perform the actions to be performed continuously
        // INSIDE each state
        }
    }
}

NOTE: the else can be omitted or not. If you leave it, in the cycle that you detect the change you do not perform the INSIDE action; if you omit it, the INSIDE action will be performed. This is useful in different cases.

Now, usually (and yours is one of these cases) you only need the CHECK and ENTER sections (if you look at the code, you will see that only these two sections were implemented). Once in a while you will need also the INSIDE section, and once in your life you will also need the EXIT ;) (at least in my experience)

  • I will report back once I had the time to implement and test the code. Your answer is greatly appreciated. However I have a small note on why I handled the statemachine the way I did: If I see it correctly the selected case runs every loop in your code too. This creates a flickering of the text on the LCD as it gets deleted and rewritten every cycle. That's why I wrote it once, and advanced to the next case where nothing happens. – idkfa Mar 13 '18 at 17:36
  • @idkfa (BTW nice nickname, but I preferred iddqd :P) no, the code is executed just once. I'm adding a quick section at the end of the answer to explain the way I usually implement state machines (it is the way they thought me), but the code here is executed only once.. – frarugi87 Mar 14 '18 at 14:10
  • I finally got around to read through your examples. I learned a lot from it and I must say I definitely understand now why you didn't like my implementation. I also see how this avoids the flickering issue. Thank you very much! – idkfa Mar 25 '18 at 22:02
  • 1
    @idkfa I'm glad I could help you :) – frarugi87 Mar 26 '18 at 7:19
1

You haven't ever set value1 or value2, but your if-else statements depend on them.

Before you can test a variable to have a specific value, you must set the variable at some point prior to using it. In your code, value1 and value2 are declared global, but never initialized and never set, so they are undefined and could have any value in them at the start.

  • I added the values (and confirmed that it still doesn't work). They got lost during creating the MCVE from my code. – idkfa Mar 13 '18 at 17:33
  • Global variables are set to zero by the compiler. – Nick Gammon Mar 13 '18 at 21:20

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.