Problem with selecting an option

Nationalflaggen · January 31, 2018, 11:04am

Hey guys, this is my first post here and I really hope that you can help me.

I recently bought an OLED-Display for my Arduino Nano and it seems to work fine. I managed to display text and graphics, and now I'm trying to connect the display with another program that plays music via a speaker.

That means that I'm implementing a screen where the three different options for songs to choose from is shown, which works just fine. (I have written a function menu() for that)
But now, I want the user to choose a song by pressing two buttons. One is for navigating and the other is for actually selecting the song.

The problem is that I didn't even get to the selecting-part. I have written three functions [p1(), p2() and p3()] that display a dot on the left side of each song, so that the user knows which song will be played if he pushes the select button.

Everything works fine, except for the navigation-dot that should be moving down each time the users is pressing the first button. Every time I press the button, it starts to move extremely fast through all of the options, although I only want it to move one option and then stop there until I press the button again.

I have already tried several different methods, but they don't seem to work, the dot is either moving as described or not at all.

Here is the relevant part of the code: (taster = button, zustand = state)

void loop() {
display.setTextColor(WHITE);
display.setTextSize(1);

tasterZustand4 = digitalRead(taster4);
tasterZustand5 = digitalRead(taster5);

menu();

for (int i = 0; i <= 2 && tasterZustand4 == LOW; ++i) {
tasterZustand4 = digitalRead(taster4);
tasterZustand5 = digitalRead(taster5);
if (i == 0) {
display.clearDisplay();
menu();
p1();
}
else if (i == 1) {
display.clearDisplay();
menu();
p2();
}
else if (i == 2) {
display.clearDisplay();
menu();
p3();
}
tasterZustand4 = digitalRead(taster4);
// while (tasterZustand4 == LOW) {} //dot isn't moving at all if this is active
}
display.display();
}

Danois90 · January 31, 2018, 11:21am

Please use code tags and please post your entire sketch rather than parts thereof. You should debounce your buttons as the first thing: https://www.arduino.cc/en/Tutorial/Debounce

system · January 31, 2018, 11:40am

I have written three functions [p1(), p2() and p3()] that display a dot on the left side of each song

So, you have some cryptically named functions that do almost identical things. Why? p1() doesn't even hint at what it does.

You didn't post those functions, so I'm only assuming that they do almost identical things, but I can't imagine a situation where p1() draws a dot at some location and p2() draws a dot at some location and p3() draws a dot at some location where there is NOT a lot of duplicate code that would be better handled by having one function that took an argument.

It is silly to have a for loop where each iteration of the loop does something different.

system · January 31, 2018, 11:41am

By the way, it would be well worth your time to have a look at the state change detection example. It seems to me that you want the change selection switch to do something when it BECOMES pressed, not when it IS pressed.

Nationalflaggen · January 31, 2018, 12:33pm

Thanks for your answers!

Here is my updated, complete code:

#include <Adafruit_GFX.h>
#include <Adafruit_SSD1306.h>

#define OLED_RESET 4    //not used with this display
Adafruit_SSD1306 display(OLED_RESET);
#define XMAX1 122
#define YMAX1 25
#define XMAX2 116
#define YMAX2 18

int taster4 = 4;
int taster5 = 5;
int tasterZustand4 = 0;
int tasterZustand5 = 0;
int letzterZustand4 = 0;

void menu() {
  display.setTextColor(WHITE);
  display.setTextSize(1);
  display.setCursor(20, 0);
  display.println("Super Mario");
  display.setCursor(20, 10);
  display.println("Star Wars");
  display.setCursor(20, 20);
  display.println("Nyan Cat");
  display.display();
}

void p(int a) {
  if (a == 1) {
    display.fillCircle(10, 3, 2, WHITE);
    display.display();
  }
  else if (a ==2) {
    display.fillCircle(10, 13, 2, WHITE);
    display.display();
  }
  else if(a ==3) {
    display.fillCircle(10, 23, 2, WHITE);
    display.display();
  }
}

void setup() {
  display.begin(SSD1306_SWITCHCAPVCC, 0x3C);

  display.clearDisplay();

  pinMode(taster4, INPUT_PULLUP);
  pinMode(taster5, INPUT_PULLUP);

  display.setTextColor(WHITE);
  display.setTextSize(2);
  
  for (int i = 1; i <= 5; ++i) {
    display.fillCircle(45, 25, i, WHITE);
    display.fillCircle(75, 25, i, WHITE);
    delay(50);
    display.display();
  }
  
  for (int i = 25; i >= 6; --i) { 
    display.fillRect(48, i, 3, 3, WHITE);
    display.fillRect(78, i, 3, 3, WHITE);
    delay(50);
    display.display();
  }

  for (int i = 48, n = 78; i <= 63 && n >= 63; ++i, --n) {
    display.fillRect(i, 6, 3, 3, WHITE);
    display.fillRect(n, 6, 3, 3, WHITE);
    delay(50);
    display.display();
  }
  
  delay(1500);
  display.clearDisplay();
  display.display();

  display.setCursor(18,1);
  display.println("Herzlich");
  display.setCursor(6, 18);
  display.println("Willkommen");
  display.display();
  delay(3500);
  display.clearDisplay();
  display.display();

  display.setTextSize(1);

}

void loop() {
  //display.clearDisplay();
  display.setTextColor(WHITE);
  display.setTextSize(1);

  tasterZustand4 = digitalRead(taster4);
  tasterZustand5 = digitalRead(taster5);
  int i = 1;

  menu();

  while (i <= 3) {
    if (tasterZustand4 != letzterZustand4 && tasterZustand4 == LOW) {
      ++i;
    }
    delay(50);
    letzterZustand4 = tasterZustand4;
  
    if (i == 1) {
      display.clearDisplay();
      menu();
      p(1);
    }
  
    else if (i == 2) {
      display.clearDisplay();
      menu();
      p(2);
    }
  
    else if (i == 3) {
      display.clearDisplay();
      menu();
      p(3);
    }
  }
}

It still doesn't work though... The dot is flickering at the first position and it isn't moving at all.
Maybe it's just another silly mistake but I hope you can help me anyway. Thanks in advance!

Danois90 · January 31, 2018, 1:04pm

Your loop does not make much sense.. So, each time taster4 is pressed, the program should change?

int program = 1;

void loop() {
  //The next two lines should probably be moved to the end of "setup()"
  display.setTextColor(WHITE);
  display.setTextSize(1);

  //Detect a click of at least 50 milliseconds
  unsigned long storedMillis = millis();
  while ((digitalRead(taster4)==LOW) && (millis() - storedMillis < 50)) {};

  if (digitalRead(taster4) == LOW) {
    program++;
    if (program > 3) program = 1;

    //The menu is only redrawn when necessary. You must initialize the menu at end of "setup()"
    display.clearDisplay();
    menu();
    p(program);

  }
}

Nationalflaggen · January 31, 2018, 1:18pm

Thanks for your answer!

I tried what you suggested. The dot is now moving down one spot when the button is pressed, but when I let go it goes back to the top. It never reaches the third position.

Here's the code:

#include <Adafruit_GFX.h>
#include <Adafruit_SSD1306.h>

#define OLED_RESET 4    //not used with this display
Adafruit_SSD1306 display(OLED_RESET);
#define XMAX1 122
#define YMAX1 25
#define XMAX2 116
#define YMAX2 18

int taster4 = 4;
int taster5 = 5;
int tasterZustand4 = 0;
int tasterZustand5 = 0;
int letzterZustand4 = 0;

void menu() {
  display.setTextColor(WHITE);
  display.setTextSize(1);
  display.setCursor(20, 0);
  display.println("Super Mario");
  display.setCursor(20, 10);
  display.println("Star Wars");
  display.setCursor(20, 20);
  display.println("Nyan Cat");
  display.display();
}

void p(int a) {
  if (a == 1) {
    display.fillCircle(10, 3, 2, WHITE);
    display.display();
  }
  else if (a ==2) {
    display.fillCircle(10, 13, 2, WHITE);
    display.display();
  }
  else if(a ==3) {
    display.fillCircle(10, 23, 2, WHITE);
    display.display();
  }
}

void setup() {
  display.begin(SSD1306_SWITCHCAPVCC, 0x3C);

  display.clearDisplay();

  pinMode(taster4, INPUT_PULLUP);
  pinMode(taster5, INPUT_PULLUP);

  display.setTextColor(WHITE);
  display.setTextSize(2);
  
  for (int i = 1; i <= 5; ++i) {
    display.fillCircle(45, 25, i, WHITE);
    display.fillCircle(75, 25, i, WHITE);
    delay(50);
    display.display();
  }
  
  for (int i = 25; i >= 6; --i) { 
    display.fillRect(48, i, 3, 3, WHITE);
    display.fillRect(78, i, 3, 3, WHITE);
    delay(50);
    display.display();
  }

  for (int i = 48, n = 78; i <= 63 && n >= 63; ++i, --n) {
    display.fillRect(i, 6, 3, 3, WHITE);
    display.fillRect(n, 6, 3, 3, WHITE);
    delay(50);
    display.display();
  }
  
  delay(1500);
  display.clearDisplay();
  display.display();

  display.setCursor(18,1);
  display.println("Herzlich");
  display.setCursor(6, 18);
  display.println("Willkommen");
  display.display();
  delay(3500);
  display.clearDisplay();
  display.display();

  display.setTextSize(1);
  display.setTextColor(WHITE);

  menu();

}

void loop() {
  //display.clearDisplay();

  tasterZustand4 = digitalRead(taster4);
  tasterZustand5 = digitalRead(taster5);
  int i = 1;

  unsigned long storedMillis = millis();
  while ((digitalRead(taster4) == LOW) && (millis() - storedMillis < 50)) {};

  if (digitalRead(taster4) == LOW) {
    ++i;
  }
  if (i > 3) {
    i = 1;
  }

  display.clearDisplay();
  menu();
  p(i);

}

Danois90 · January 31, 2018, 1:20pm

"i" must be declared globally or else it resets to 1 for each loop.

BulldogLowell · January 31, 2018, 1:25pm

Danois90:
"i" must be declared globally or else it resets to 1 for each loop.

or... static

void loop() {
  //display.clearDisplay();

  tasterZustand4 = digitalRead(taster4);
  tasterZustand5 = digitalRead(taster5);
  static int i = 1;  // a persistent local variable with a crappy name "i"

system · January 31, 2018, 1:31pm

void p(int a) {

Will you be expanding this function to print dots on more than 255 lines? If not, int is the wrong type. No sense wasting memory when you don't need to.

 if (a == 1) {
   display.fillCircle(10, 3, 2, WHITE);
   display.display();
 }
 else if (a ==2) {
   display.fillCircle(10, 13, 2, WHITE);
   display.display();
 }
 else if(a ==3) {
   display.fillCircle(10, 23, 2, WHITE);
   display.display();
 }

Each block has some of the same code. Removing that, your function looks like:

 if (a == 1) {
    display.fillCircle(10, 3, 2, WHITE);
  }
  else if (a ==2) {
    display.fillCircle(10, 13, 2, WHITE);
  }
  else if(a ==3) {
    display.fillCircle(10, 23, 2, WHITE);
  }
  display.display();

Now, the second argument to the fillCircle() function is the only thing that changes. So,

 display.fillCircle(10, (a-1)*10 + 3, 2, WHITE);
  display.display();

Less code is better than more code.

 while (i <= 3) {
    if (tasterZustand4 != letzterZustand4 && tasterZustand4 == LOW) {
      ++i;
    }
    delay(50);
    letzterZustand4 = tasterZustand4;
 
    if (i == 1) {
      display.clearDisplay();
      menu();
      p(1);
    }
 
    else if (i == 2) {
      display.clearDisplay();
      menu();
      p(2);
    }
 
    else if (i == 3) {
      display.clearDisplay();
      menu();
      p(3);
    }
  }

Again, this can be made a lot simpler, as all that the inner if statements are doing is clearing the display, showing the menu, and calling p() with the value of i.

 while (i <= 3)
  {
    if (tasterZustand4 != letzterZustand4 && tasterZustand4 == LOW)
    {
      ++i;
    }
    delay(50);
    letzterZustand4 = tasterZustand4;
 
    display.clearDisplay();
    menu();
    p(i);
  }

Now, this code doesn't make a lot of sense. If the state of the pin changed to LOW, you increment i, and then make the current state and the previous state the same as the current state. Nowhere do you read the state of the switch again, so the while loop will just keep repeating, do the same stuff every time. Updating the display, showing the menu, and drawing the dot should happen ONLY when the change is detected. That is, the last three lines in the while body belong in the if body.

But, you also need to call digitalRead() again, to get the (new) current state of the pin.

But, you need to explain why you have a while loop. The loop() function already loops. That's why it isn't called doThisOnce(). You need to constrain the value of i, so that when it gets to 4, you reset it to 1, and get rid of the while statement (but not all the stuff in the body of the while statement.

Nationalflaggen · January 31, 2018, 1:44pm

Thank you for your help everyboby, it's finally working now!
I was already becoming desperate.
So thanks again, I'm really happy to be part of an awesome community.

Topic		Replies	Views
Menu programming problem with nano Programming	6	1236	May 5, 2021
The Machines are here!!! :D Programming	9	1372	May 5, 2021
Menu always activating at first option -- please help Programming	1	337	May 6, 2021
Selecting alternate output to display? Programming	3	698	May 5, 2021
LCD menu w/ buttons Programming	22	3046	May 5, 2021

Problem with selecting an option

Related topics