Can I use an array or other method to simplyfy my code?

[arneko]

Hello all,

I am unexperienced writing code, still learning from examples, but trying to broaden my horizon.
Chief amongst previous advice was "keep it simple". So i've build up a function to set and save 1 value to EEPROM. Now I want to set a time and duration and save that to EEPROM, so I've copied the code 3 times and added conditions to all of them so they don't mix eachother up. Over a 100 lines for something simple, and even worse I need to copy and paste the entire function 3 more times to get a total of 4 timers with duration.
I know about the TimeAlarm library but I want to save all values to EEPROM as a failsafe.

I feel like a monkey banging a wrench on the keyboard to make this work.

I'm am sure there must be beter or more elegant ways to write this.

Would anyone be willing to take a look and offer advice? Any help is greatly appreciated.

Here is what I have:

void setRain1Timer() {

  int hour1Saved = 0, minute1Saved = 0, duration1Saved = 0, rain1Hr = 0, rain1Min = 0, rainTime1 = 0;
  myScreen.fillScreen(BLACK);
  myScreen.setCursor(0, 0);
  myScreen.print("Rain Time 1:");
  myScreen.setCursor(0, 10);
  myScreen.print("ENTER ON TIME");
  while (1) {
    if (hour1Saved == 0 && minute1Saved == 0 && duration1Saved == 0 && joyButtonRead(pbUp) == HIGH) {  // joystick up
      rain1Hr++;
      myScreen.setCursor(10, 20);
      myScreen.print("hr ");
      printDigits(rain1Hr);
      delay(200);
      if (rain1Hr == 24)
        rain1Hr = 0;
    }
    if (hour1Saved == 0 && minute1Saved == 0 && duration1Saved == 0 && joyButtonRead(pbDown) == HIGH) {  // joystick down
      rain1Hr--;
      myScreen.setCursor(10, 20);
      myScreen.print("hr ");
      printDigits(rain1Hr);
      delay(200);
      if (rain1Hr == 0)
        rain1Hr = 23;
    }
    if (hour1Saved == 0 && minute1Saved == 0 && duration1Saved == 0 && joyButtonRead(pbRight) == HIGH) {  // joystick right to save value
      EEPROM.write(4, rain1Hr);
      delay(1000);
      hour1Saved = 1;
    }


    if (hour1Saved == 1 && minute1Saved == 0 && duration1Saved == 0 && joyButtonRead(pbUp) == HIGH) {  // joystick up
      rain1Min++;
      myScreen.setCursor(40, 20);
      myScreen.print(" min ");
      printDigits(rain1Min);
      delay(200);
      if (rain1Min == 60)
        rain1Min = 0;
    }
    if (hour1Saved == 1 && minute1Saved == 0 && duration1Saved == 0 && joyButtonRead(pbDown) == HIGH) {  // joystick down
      rain1Min--;
      myScreen.setCursor(40, 20);
      myScreen.print(" min ");
      printDigits(rain1Min);
      delay(200);
      if (rain1Min == 0)
        rain1Min = 59;
    }
    if (hour1Saved == 1 && minute1Saved == 0 && duration1Saved == 0 && joyButtonRead(pbRight) == HIGH) {  // joystick right to save value
      EEPROM.write(5, rain1Min);
      delay(1000);
      minute1Saved = 1;
    }


    if (hour1Saved == 1 && minute1Saved == 1 &&  duration1Saved == 0 && joyButtonRead(pbUp) == HIGH) {  // joystick up
      rainTime1++;
      myScreen.setCursor(90, 20);
      myScreen.print(" sec ");
      printDigits(rainTime1);
      delay(200);
      if (rainTime1 == 60)
        rainTime1 = 0;
    }
    if (hour1Saved == 1 && minute1Saved == 1 && duration1Saved == 0 &&  joyButtonRead(pbDown) == HIGH) {  // joystick down
      rainTime1--;
      myScreen.setCursor(90, 20);
      myScreen.print(" sec ");
      printDigits(rainTime1);
      delay(200);
      if (rainTime1 == 0)
        rainTime1 = 59;
    }
    if (hour1Saved == 1 && minute1Saved == 1 && duration1Saved == 0 &&  joyButtonRead(pbRight) == HIGH) {  // joystick right to save value
      EEPROM.write(6, rainTime1);
      delay(1000);
      duration1Saved = 1;
    }

    if (hour1Saved == 1 && minute1Saved == 1 && duration1Saved == 1) {  // 3 values saved, print results
      myScreen.setCursor(10, 30);
      myScreen.print(" START TIME SET ");
      myScreen.setCursor(10, 40);
      myScreen.print("AT: ");
      printDigits(EEPROM.read(5));
      myScreen.print(":");
      printDigits(EEPROM.read(6));
      myScreen.print(":00");
      myScreen.print("     ");
      myScreen.setCursor(10, 50);
      myScreen.print("DURATION: ");
      printDigits(EEPROM.read(6));
      myScreen.print(" SECONDS");
      myScreen.setCursor(10, 60);
      myScreen.print("SAVED");
      delay(5000);
      rain1Hr = 0;
      rain1Min = 0;
      hour1Saved = 0;
      minute1Saved = 0;
      duration1Saved = 0;
      rainTime1 = 0;
      break;
    }
  }
}

[ec2021]

Hi @arneko,

one of the things to do when you want to keep the code "small" is to look for double/multiple lines with the same content and to look for the differences...

More precise:

If you look at the part for increasing and decreasing the hours you see that these lines are identical

      myScreen.setCursor(10, 20);
      myScreen.print("hr ");
      printDigits(rain1Hr);
      delay(200);
   

and the difference is in

      rain1Hr++;
      // print it
      if (rain1Hr == 24)
        rain1Hr = 0;

and

      rain1Hr--;
     // print it
      if (rain1Hr == 0)
        rain1Hr = 23;

controlled by joyButtonRead(pbUp) or (pbDown).

This could be a first step for optimization.

A further point is that you use the same procedure for hours, minutes and seconds.

So it might be of advantage to write a function that gets the variable to set and the further data (like text and upper liimit as well as the EEPROM address as parameters.

[PerryBebbington]

Hello @arneko

Your topic was moved to its current location as it is more suitable.

Could you also take a few moments to Learn How To Use The Forum.

It will help you get the best out of the forum in the future.

Thank you

[lastchancename]

Look into nesting your IFs, rather than repeating them.

[ec2021]

I used your sketch to create one that might give you some hints; it may not work with yur setup as you quite likely use a different display, but the principle should be the same.

You will have to adapt it to your hardware:

#include "SPI.h"
#include "Adafruit_GFX.h"
#include "Adafruit_ILI9341.h"

#define TFT_DC 9
#define TFT_CS 10

Adafruit_ILI9341 myScreen = Adafruit_ILI9341(TFT_CS, TFT_DC);
constexpr int pbRight = 5;
constexpr int pbUp    = 6;
constexpr int pbDown  = 7;


void setup() {
  Serial.begin(9600);
  Serial.println("Test");
  pinMode(pbRight, INPUT_PULLUP);
  pinMode(pbUp, INPUT_PULLUP);
  pinMode(pbDown, INPUT_PULLUP);
  myScreen.begin();
  myScreen.setTextColor(ILI9341_RED);
  myScreen.setTextSize(2);
  myScreen.setRotation(3);
  setRain1Timer();
}

void loop(void) {
}

void setRain1Timer() {
  int rainHr = 0;
  int rainMin = 0;
  int rainTime = 0;
  myScreen.fillScreen(ILI9341_BLACK);
  myScreen.setCursor(0, 0);
  myScreen.print("Rain Time 1:");
  myScreen.setCursor(0, 20);
  myScreen.print("ENTER ON TIME");
  int inputGiven = 0;
  while (inputGiven < 4) {
    switch (inputGiven) {
      case 0 : if (getInput("Hr  ", 200, 20, rainHr, 24) == true) {
          inputGiven = 1;
        };
        break;
      case 1 : if (getInput("Min ", 200, 40, rainMin, 60) == true) {
          inputGiven = 2;
        };
        break;
      case 2 : if (getInput("Sec ", 200, 60, rainTime, 60) == true) {
          inputGiven = 3;
        };
        break;
      case 3 : // Write all data to EEPROM here
          Serial.println("Write data to EEPROM");
          inputGiven = 4;
          break;
        }
  }
  // Read and print data from EEPROM here
  Serial.println("Read and print data from EEPROM");
  printData(rainHr,rainMin,rainTime);

}


boolean getInput(String txt, int x, int y, int &data, int maxValue) {
  static unsigned long lastPrint = 0;
  static unsigned long lastChange = 0;
  static int oldData = 0;
  static String oldText = "";
  // Assuming that only one button is pressed at a time!!!
  boolean Up    = joyButtonRead(pbUp);
  boolean Down  = joyButtonRead(pbDown);
  boolean Right = joyButtonRead(pbRight);
  if (millis() - lastPrint > 200) {
    lastPrint = millis();
    if (oldData != data || oldText != txt ) {
      myScreen.setTextColor(ILI9341_BLACK);
      myScreen.setCursor(x, y);
      myScreen.print(oldText);
      myScreen.print(oldData);
      oldData = data;
      oldText = txt;
    } 
    myScreen.setTextColor(ILI9341_RED);
    myScreen.setCursor(x, y);
    myScreen.print(txt);
    myScreen.print(data);
  }
  if (millis() - lastChange > 200) {
    if (Up)  {
      lastChange = millis();
      data++;
      if (data > maxValue) {
        data = 0;
      }
    }  
    if (Down)  {
      lastChange = millis();
      data--;
      if (data < 0) {
        data = maxValue-1;
      }
    }
    if (Right) {
      lastChange = millis();
      return true;
    }  
  }
  return false;
}

boolean joyButtonRead(int pin) {
  // To make it easy for me I removed the "bounce" option of the buttons in Wokwi ;-)
  return !digitalRead(pin);
}

void printData(int hr, int mi, int se){
      myScreen.fillScreen(ILI9341_BLACK);
      myScreen.setCursor(10, 30);
      myScreen.print(" START TIME SET ");
      myScreen.setCursor(10, 50);
      myScreen.print("AT: ");
      myScreen.print(hr);
      myScreen.print(":");
      myScreen.print(mi);
      myScreen.print(":00");
      myScreen.print("     ");
      myScreen.setCursor(10, 70);
      myScreen.print("DURATION: ");
      myScreen.print(se);
      myScreen.print(" SECONDS");
      myScreen.setCursor(10, 90);
      myScreen.print("SAVED");
  }

Feel free to check it out on Wokwi:

RainTime I - Wokwi Arduino and ESP32 Simulator

and of course insert your routines to store to and read from EEPROM ...

Buttons in accordance with their names "Up", "Down" and "Right".
To ease the sketch I removed the "bounce" function from the buttons ... Debouncing is probably inside your "joyButtonRead" function (I hope)

[build_1971]

To answer your initial question...
...no, arrays are not the obvious way to improve your code.
The other posters have already given you some ideas that are very worthwhile un your particular case.
You could put hr, min and sec in an array. But then a struct may be a better alternative.

[arneko]

Thank you @ec2021 and @lastchancename , I have taken notes and made some changes. both to improve the code and also improve "user experience". It is now more clear what is being changed and processed.

This is the current state:

void setRain1Timer() {

  int hour1Saved = 0, minute1Saved = 0, duration1Saved = 0, rain1Hr = 0, rain1Min = 0, rainTime1 = 0;
  myScreen.fillScreen(BLACK);
  myScreen.setCursor(0, 0);
  myScreen.print("Rain Time 1:");
  myScreen.setCursor(0, 10);
  myScreen.print("ENTER ON TIME");
  myScreen.setCursor(10, 20);
  myScreen.print("00:00");
  myScreen.setCursor(10, 30);
  myScreen.print(" ^");
  while (1) {
    if (hour1Saved == 0 && minute1Saved == 0 && duration1Saved == 0) {
      if (joyButtonRead(pbUp) == HIGH) {  // joystick up
        rain1Hr++;
        myScreen.setCursor(10, 20);
        printDigits(rain1Hr);
        delay(200);
        if (rain1Hr == 24)
          rain1Hr = 0;
      }
      if (joyButtonRead(pbDown) == HIGH) {  // joystick down
        rain1Hr--;
        myScreen.setCursor(10, 20);
        printDigits(rain1Hr);
        delay(200);
        if (rain1Hr == 0)
          rain1Hr = 23;
      }
      if (joyButtonRead(pbRight) == HIGH) {  // joystick right to save value
        EEPROM.write(4, rain1Hr);
        delay(500);
        myScreen.setCursor(10, 30);
        myScreen.print("   ");
        myScreen.setCursor(28, 30);
        myScreen.print(" ^");
        hour1Saved = 1;
      }
    }

    if (hour1Saved == 1 && minute1Saved == 0 && duration1Saved == 0) {
      if (joyButtonRead(pbUp) == HIGH) {  // joystick up
        rain1Min++;
        myScreen.setCursor(28, 20);
        printDigits(rain1Min);
        delay(200);
        if (rain1Min == 60)
          rain1Min = 0;
      }
      if (joyButtonRead(pbDown) == HIGH) {  // joystick down
        rain1Min--;
        myScreen.setCursor(28, 20);
        printDigits(rain1Min);
        delay(200);
        if (rain1Min == 0)
          rain1Min = 59;
      }
      if (joyButtonRead(pbRight) == HIGH) {  // joystick right to save value
        EEPROM.write(5, rain1Min);
        myScreen.setCursor(30, 30);
        myScreen.print("   ");
        delay(500);
        myScreen.setCursor(50, 20);
        myScreen.print("duration:");
        minute1Saved = 1;
      }
    }

    if (hour1Saved == 1 && minute1Saved == 1 && duration1Saved == 0) {
      if (joyButtonRead(pbUp) == HIGH) {  // joystick up
        rainTime1++;
        myScreen.setCursor(110, 20);
        printDigits(rainTime1);
        delay(200);
        if (rainTime1 == 60)
          rainTime1 = 0;
      }
      if (joyButtonRead(pbDown) == HIGH) {  // joystick down
        rainTime1--;
        myScreen.setCursor(110, 20);
        printDigits(rainTime1);
        delay(200);
        if (rainTime1 == 0)
          rainTime1 = 59;
      }
      if (joyButtonRead(pbRight) == HIGH) {  // joystick right to save value
        EEPROM.write(6, rainTime1);
        delay(1000);
        duration1Saved = 1;
      }
    }

    if (hour1Saved == 1 && minute1Saved == 1 && duration1Saved == 1) {  // 3 values saved, print results
      myScreen.setCursor(10, 30);
      myScreen.print(" START TIME SET ");
      myScreen.setCursor(10, 40);
      myScreen.print("AT: ");
      printDigits(EEPROM.read(4));
      myScreen.print(":");
      printDigits(EEPROM.read(5));
      myScreen.print(":00");
      myScreen.print("     ");
      myScreen.setCursor(10, 50);
      myScreen.print("DURATION: ");
      printDigits(EEPROM.read(6));
      myScreen.print(" SECONDS");
      myScreen.setCursor(10, 60);
      myScreen.print("SAVED");
      delay(5000);
      rain1Hr = 0;
      rain1Min = 0;
      hour1Saved = 0;
      minute1Saved = 0;
      duration1Saved = 0;
      rainTime1 = 0;
      break;
    }
  }
}

I really like the example you gave me, based on the Wokwi, @ec2021 . I will need to study it some more before I'm comfortable with things like

boolean getInput(String txt, int x, int y, int &data, int maxValue)

It just looks intimidating with the ammount of variables I have to wrap my head around.

and

static unsigned long lastPrint = 0;

why is it static? what does unsigned even mean? I think I am about to find out with the help of my friend Google

@build_1971
I had seen structs but I think implementing them at this point I would just be copy-pasting something without understanding what I was doing. Maybe in time

[Perehama]

@arneko

I had seen structs but I think implementing them at this point I would just be copy-pasting something without understanding what I was doing.

@arneko I am in the process of writing a tutorial on this very subject. I hope to make it very approachable. It will be a couple weeks before I am finished as it's secondary to other responsibilities. However, when I have finished it, I'll send you a private message.

[arneko]

cheers!

[alto777]

Nice.

@ec2021

  // To make it easy for me I removed the "bounce" option of the buttons in Wokwi ;-)

A handy feature. I turned the bouncing back on thinking it should work fine without, it seems to if you keep the presst time quite short.

An easy way to eliminate that and the need to say so in the user manual (!) is to use a small capacitor to do the denouncing in hardware, traditionally offered by @groundFungus:

RGB LED Push Button

And you could loose the external pullup by using the internal pullup by setting the pinMode to INPUT_PULLUP. pinMode(buttonPin, INPUT_PULLUP); I would also like it to debounce to ensure accuracy. I think I saw something about using a library, but I would like to try and avoid it if possible. Add hardware debounce with a 0.1uF cap across the switch (input to ground). [dig in no pullup]

a7