Problem using Keypad to control GLCD menu

Hi everyone,
I'm creating a menu for my project to help my it look more intuitive. I'm using a GLCD with a u8g2 library, the menu is as seen in the picture attached.

Keypad keypad = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );
char key;
void setup(){
  Serial.begin(9600);
  u8g2.begin();
  startup();
}
  
void loop(){
  
  key = keypad.getKey();// Read the key
  if(key)
  Serial.print(key);
  // Print if key pressed
  switch(key)
 {
  case 'A' :
  InHouse();
  break;

  case 'B':
  refGUI();
  break;
  
  
 }

}



void startup(){
  u8g2.clearBuffer();          // clear the internal memory
  u8g2.setFont(u8g2_font_6x10_tf);  // choose a suitable font
  u8g2.drawStr(30,7,"STARTING...");  // write something to the internal memory
  u8g2.sendBuffer();          // transfer internal memory to the display
  /*
   *  Put the SYSRESET* code here so the addresses will load in the background
   */

  delay(1500);
  u8g2.clearBuffer(); 
  u8g2.setFont(u8g2_font_6x10_tf);  // choose a suitable font
  u8g2.drawStr(0,7,"Select Mode:");  // write something to the internal memory
  u8g2.drawStr(0,24,"(A) GUI");  // write something to the internal memory
  u8g2.drawStr(0,32,"(B) Project Mode");  // write something to the internal memory
  u8g2.sendBuffer();          // transfer internal memory to the display
  return;
  
}


void InHouse(){
  u8g2.clearBuffer();          // clear the internal memory
  u8g2.setFont(u8g2_font_6x10_tf);  // choose a suitable font
  u8g2.drawStr(0,7,"Project Selcted.");  // write something to the internal memory
  u8g2.sendBuffer();
  delay(1500);
  u8g2.clearBuffer();
  u8g2.setFont(u8g2_font_6x10_tf);  // choose a suitable font
  u8g2.drawStr(0,7,"Select Mode:");  // write something to the internal memory
  u8g2.drawStr(0,24,"(A) Automatic");  // write something to the internal memory
  u8g2.drawStr(0,32,"(B) Manual");  // write something to the internal memory
  u8g2.drawStr(0,40,"(C) Back");  // write something to the internal memory
  u8g2.sendBuffer();          // transfer internal memory to the display
  delay(250);
  while(key = keypad.getKey())
  switch(key){
    case 'A':
    AutoRefer();
    break;
    case 'B':
    ManualRefer();
    break;
  }
}


void refGUI(){
  u8g2.clearBuffer();          // clear the internal memory
  u8g2.setFont(u8g2_font_6x10_tf);  // choose a suitable font
  u8g2.drawStr(0,7,"Continue On PC ");  // write something to the internal memory
  u8g2.sendBuffer();
  //Put the required code to transfer data to the GUI application
}

The problem is that once the Arduino starts and the program is loaded, the only part that works in the menu is the first part in the void loop, that selects between the Arduino mode or GUI. I tried to make another loop inside of the "InHouse" function, that will wait until a key is presses in the keypad and then will go to the switch case statement.
I tried using "if... else.." method but it faced the same problem; Stucked in the first selection choice.
Can anyone help me get in to the second selection choice? (also i'm quit new so go easy on me :wink: )

Is that the complete code and if not, can you post it all (including stuff above the Keypad definition...)

The loop function is called repeatedly so if it doesn't see an A or a B from the keyboard you will get another chance in a few microseconds.

Your InHouse function doesn't loop, so if it doesn't see a keypress immediately, it returns. You need to loop waiting for a key and you need a third key there to say "go back".

i doubt if this does what you want it to do..

while(key = keypad.getKey())
  switch(key){
    case 'A':
    AutoRefer();
    break;
    case 'B':
    ManualRefer();
    break;
  }

although it might do so partly.
i have to admit that i find these kind of statements confusing.

while(key = keypad.getKey())

the condition always true (it's an assignment) and the statement continues on the next line with switch(key) { etc.
The same here

if(key)
  Serial.print(key);
  // Print if key pressed

putting the statement on a new line, but without the braces.

that will wait until a key is presses in the keypad and then will go to the switch case statement.

Well you see it should do as you want, but not in the way you intend it.

   case 'A':
    AutoRefer();
    break;
    case 'B':
    ManualRefer();
    break;

Where are those functions ?

Don't forget that getKey returns zero for no_key. That breaks the while if you're not holding a key on the first iteration.

For Blackfin,
this is the complete code

#include <Arduino.h>
#include <U8g2lib.h>
U8G2_ST7920_128X64_F_SW_SPI u8g2(U8G2_R0, /* clock=*/ 13, /* data=*/ 11, /* CS=*/ 10, /* reset=*/ 8);
#include <Keypad.h>

const byte ROWS = 4; //four rows
const byte COLS = 4; //four columns

char keys[ROWS][COLS] = {
  {'1','2','3','A'},
  {'4','5','6','B'},
  {'7','8','9','C'},
  {'.','0','\r','D'}
};

byte rowPins[ROWS]={39,41,43,45};
byte colPins[COLS]={31,33,35,37};

//Create an object of keypad
Keypad keypad = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );
char key;
void setup(){
  Serial.begin(9600);
  u8g2.begin();
  startup();
}
  
void loop(){
  key = keypad.getKey();// Read the key
  if(key)
  Serial.print(key);
  // Print if key pressed
  switch(key)
 {
  case 'A' :
  InHouse();
  break;

  case 'B':
  refGUI();
  break;
 }

}
void startup(){
  u8g2.clearBuffer();          // clear the internal memory
  u8g2.setFont(u8g2_font_6x10_tf);  // choose a suitable font
  u8g2.drawStr(30,7,"STARTING...");  // write something to the internal memory
  u8g2.sendBuffer();          // transfer internal memory to the display
  delay(1500);
  u8g2.clearBuffer(); 
  u8g2.setFont(u8g2_font_6x10_tf);  // choose a suitable font
  u8g2.drawStr(0,7,"Select Mode:");  // write something to the internal memory
  u8g2.drawStr(0,24,"(A) GUI");  // write something to the internal memory
  u8g2.drawStr(0,32,"(B) Project Mode");  // write something to the internal memory
  u8g2.sendBuffer();          // transfer internal memory to the display
  return;
  
}

void refGUI(){
  u8g2.clearBuffer();          // clear the internal memory
  u8g2.setFont(u8g2_font_6x10_tf);  // choose a suitable font
  u8g2.drawStr(0,7,"Continue On PC ");  // write something to the internal memory
  u8g2.sendBuffer();
  //Put the required code to transfer data to the GUI application
}


void InHouse(){
  u8g2.clearBuffer();          // clear the internal memory
  u8g2.setFont(u8g2_font_6x10_tf);  // choose a suitable font
  u8g2.drawStr(0,7,"Project Selcted.");  // write something to the internal memory
  u8g2.sendBuffer();
  delay(1500);
  u8g2.clearBuffer();
  u8g2.setFont(u8g2_font_6x10_tf);  // choose a suitable font
  u8g2.drawStr(0,7,"Select Mode:");  // write something to the internal memory
  u8g2.drawStr(0,24,"(A) Automatic");  // write something to the internal memory
  u8g2.drawStr(0,32,"(B) Manual");  // write something to the internal memory
  u8g2.drawStr(0,40,"(C) Back");  // write something to the internal memory
  u8g2.sendBuffer();          // transfer internal memory to the display
  delay(250);
  while(key = keypad.getKey())
  switch(key){
    case 'A':
    AutoRefer();
    break;
    case 'B':
    ManualRefer();
    break;
    case 'C':
    startup();
    loop();
    break;
  }
}

the AutoRefer and ManualRefer functions are just displaying a message (just so I can see if the functions are actually called in first place, right now i’m using u8g2.drawStr to display messege on the GLCD display) so I didn’t included them.
For wildbill,
The while(key = keypad.getKey()) that in InHouse function doesn’t supposed to “wait” until there is a key press and then go to the Switch Case statement ?
And if I add a "Go back " option in the InHouse “Switch Case”, it would look like what I wrote in the code i’ve posted in this reply?
Thanks ahead.

No. InHouse needs to loop forever looking for a key that isn’t no_key. One of the keys you act on should exit the function to go back to the loop function.

But. Never call the loop function directly. If you do, you may well consume all your RAM and crash.

Using a

InHouse(){
while(keypad.getKey() != NO_KEY)
 switch(key){
    case 'A':
    AutoRefer();
    break;
    case 'B':
    ManualRefer();
    break;
    case 'C':
    startup();
    loop();
    break;
}
}

didn't solved the issue for me.
And I can move the first Switch Case to the startup function once I will understand how to make the loop inside the functions work in a proper way.

You missed the second point. For key 'C', just return.

Nesting menus can get really messy. You can end up with really knotted, tangled code if you have more than a couple of depths of nesting and don’t “design” the code for it but rather just sort of keep adding stuff.

I’ve attached an example of how I’ve done nesting in the past. Some of the more seasoned programmer will likely laugh and scoff but it might work for you. A lot depends on what you want the various things to actually do deeper in the nest.

Uses non-beginner concepts. Commented pretty heavily. Hopefully helpful.

nested_menus.ino (15.6 KB)

Thanks Blackfin, really appreciate that you send me your menu example. The menu itself won't be that nested since each AutoRefer and ManualRefer functions, that are having some text right know, will have an actual thing to do (Mainly sending data to the serial port).

the menu you sent is not working, but I will read the comments and see what can I learn from it.
Any other advice would be appreciated.

emribg:
the menu you sent is not working, but I will read the comments and see what can I learn from it.
Any other advice would be appreciated.

In what way is it not working? I was only able to test it here on a 2560 using pins 2, 3 and 4 acting as test inputs. If you check the ReadKeys() function you’ll see that, as is, it reads the pins, not the keypad. Make the following change to enable the keypad:

char ReadKey( void )
{
    uint8_t
        keyNow;
    static uint32_t
        timeKey;
    uint32_t
        timeNow = millis();

    //keys are read every 50mS
    if( (timeNow - timeKey) < 50ul )
        return NO_KEY;

    timeKey = timeNow;
#if 0                                        //<<<< change to #if 1 to enable keypad
    return( keypad.getKey() );
#else
    //as I don't have a keypad, i just mimic A, B and C with digital IOs
    //2, 3 and 4
    //  2 == A
    //  3 == B
    //  4 == C
    //DEBUG
    for( uint8_t i=0; i<3; i++ )
    {
        //look for a change of state -- transition high to low
        //return the first one found
        //if none found, return NO_KEY
        keyNow = digitalRead( fakeKeys[i] );
        if( keyNow != lastFakeKeys[i] )
        {
            lastFakeKeys[i] = keyNow;
            if( keyNow == LOW )
            {
                return( fakeKeyReply[i] );
                    
            }//if
            
        }//if
        
    }//for

    return NO_KEY;
    
    //DEBUG
#endif    
    
}//ReadKey

Don't forget that getKey returns zero for no_key. That breaks the while if you're not holding a key on the first iteration.

it would if you would test for that, but since the assignment of key = keypad.getKey()while(key = keypad.getKey())always returns true, it really doesn't matter what getkey() returns. It is assigned to ke and that assignment returns true.

Deva_Rishi:
it would if you would test for that, but since the assignment of key = keypad.getKey()while(key = keypad.getKey())always returns true, it really doesn't matter what getkey() returns. It is assigned to ke and that assignment returns true.

No, it doesn't. It's using the value of key as a Boolean. No_key is zero AKA false.

First of all thanks foe everyone who helped here.
To Blackfin, I worked a little bit on your code but I still didn't understand the part of the "statesFunc". Most of what I did is working on the current code you sent and modify it (not by much) for my needs. The main problem is with creating a new function/sub-menu in the future since I didn't quit understood the principle of how does the code works. Again, I'm new here so I don't have a lot of experience.
My question is how do I call for a sub menu when a button is pressed, for example, if I would like to make a "Choose card list" before the AutoRefer() and ManualRefer(), and after the InHouse() how do I call it?

Hi emribg.

In loop(), you see this:

   if( pfnActivePtr[funcIndex]() == true )
    {
        //make sure we don't somehow go back further than the main/start menu
        if( funcIndex )
            funcIndex--;
            
    }//if

The part “pfnActivePtrfuncIndex” is calling a function from the array of function pointers called pfnActivePtr using the value of funcIndex as the index into the array.

In setup(), you see this:

   funcIndex = 0;
    pfnActivePtr[funcIndex] = &handlerStartUp;

This sets that index to point to the first entry in that table of function pointers and sets the first pointer to the function “bool handlerStartUp( void )” using its name (the little ‘&’ means we want the address of that function.)

The first time through loop funcIndex is 0 and the function handlerStartup() is run.

Each of these handlers is a simple state machine with two states: It is either in an initializing state or it is in what I called the “run” state.

In the INIT state the static menu items are drawn to the screen and whatever setups need to be done are completed and then the state it moved to RUN. For example, in handlerStartup:

case    statesFunc::ST_INIT:
            //if in init state, set up the screen, then move to run state
            //DEBUG
            Serial.println( "Main Menu" );
            Serial.println( "(A) Project Mode" );
            Serial.println( "(B) GUI" );
            //DEBUG
            u8g2.clearBuffer();
            u8g2.setFont(u8g2_font_6x10_tf);  // choose a suitable font
            u8g2.drawStr(0,7,"Select Mode:");  // write something to the internal memory
            u8g2.drawStr(0,24,"(A) Project Mode");  // write something to the internal memory
            u8g2.drawStr(0,32,"(B) GUI");  // write something to the internal memory
            u8g2.sendBuffer();          // transfer internal memory to the display
            //
            stateStartup = statesFunc::ST_RUN;
            
        break;

You can see a debug serial print, the LCD is cleared and titles drawn on and then the static variable that holds the current state, “stateStartup” is set to run.

As this is the home menu, we always return ‘false’ (more on this later.)

In loop(), the next thing that’s done is I run BlinkLED() to check on a blinking LED timing and state (on or off.) When loop() runs again, pfnActivePtrfuncIndex is executed again which sends us back to handlerStartup.

But now the state is RUN. In this state we’re looking at keypresses. (You could also do other things, depending on what the menu options are, etc). Suppose you select ‘A’. The menu says that “(A)” selects the Project Mode. So in the state RUN, we check to see if the ‘A’ key was pressed. If it is, then this executes:

               case    'A':
                    //user pressed A (pass control to "InHouse" handler)
                    //we bump the active function pointer index
                    funcIndex++;
                    //and add the address of the handler to the FPS
                    pfnActivePtr[funcIndex] = &handlerInHouse;
                    //before we leave, make sure to re-init this function's state to INIT
                    //to be sure we draw the screen properly when the user hits "BACK"
                    stateStartup = statesFunc::ST_INIT;
                    
                break;

We want to move to another menu screen when we press ‘A’. In order to do this, I bump the funcIndex by one and in the array of function pointers – at the index now pointed to by funcIndex – I place the address of the function handlerInHouse. I also set stateStartup back to INIT for reasons I’ll get to a bit later.

Now when loop() gets back to run pfnActivePtrfuncIndex, funcIndex is now ‘1’ and the entry there is handlerInHouse, so that runs. Note that handlerStartUp is still in the '0’th entry of the array but because funcIndex is 1, we run that instead.

handlerInHouse is constructed similarly to handlerStartUp: It has INIT and RUN states. In the INIT state we draw the menu screen associated with selecting the mode and then set the state to RUN.

Next time in we enter the RUN state. Here we look a the keypad again. If there was no key pressed, we do nothing and return false:

               default:
                    //no action
                    return false;
                break;

If the person hits ‘A’ or ‘B’ the exact same process occurs as happened when we went from handlerStartUp to handlerInHouse: the funcIndex is bumped up and a new handler address is placed in the function pointer array at that index.

So you can add menu items in this way by just continuing this logic.

If the user hit ‘C’, which is “Back”, all we do is set the state variable for handlerInHouse back to INIT and return ‘true’.

Recall the part of loop calling these handlers:

   if( pfnActivePtr[funcIndex]() == true )
    {
        //make sure we don't somehow go back further than the main/start menu
        if( funcIndex )
            funcIndex--;
            
    }//if

If the handler function returns ‘true’ it means we want to go back. So all we have to do is decrement funcIndex by one; that will point us to the previous handler – the one that sent us to the handler that just ended (i.e. user hit Back and function returned ‘true’). And because we ended that previous handler by setting its state variable to INIT, the menu is redrawn and initializations done etc.

I’m not a programmer so this is probably all wrong but I also understand it’s not “simple.” Nesting menus with multiple selections etc can get pretty complex, all the moreso when you want to do multiple things at once and can’t afford to have delay()s clogging up the execution path.

This is a framework: In order to understand how you would add menus etc, it might be a good idea to sit down with pen and paper and layout the entire desired menu structure in a tree diagram, starting with the “home/main” menu and identifying every possible path down through the menus. I just grabbed this off the web but it sort of illustrates what I’m saying:

Thank you for writing this detailed guide Blackfin !
Helped me understand this program really easily!

I’ve been working on my own version of this menu including a couple of features that I’m about to add and a problem that related to the menu.

The first thing that I want to ask is about the manual option, (case " A " in handlerManualRefer) for entering numbers from the keypad. It seems that since I’ve pressed " A " in the Case Switch statement, it wont recognize any other key press and it will display the letter " A " in a loop. I tried to make an " Terminal " that will show any key pressed before sending the data to the serial port when pressed enter ( # on the keypad) and it suppose to show up on the GLCD, and when pressed enter it would have cleaned the screen and be ready to enter the next number. But since it wasn’t able to show the numbers I got stuck on that part.

The second thing that relates to the program is the choose car handler (handlerChooseCard), that when selected (for example if I press number 1) it will refer me to serial 1, and will store an array of numbers that will sent to the serial port selected if automatic mode selected. I tried to think of way to do it but I can’t figure it out, how do I define a serial communication ( Since it will use it in the next handlers manual / automatic) when selected in handlerChooseCard and use it in the next handlers?
BTW I posted it here because it is related to the menu, and there is a working automatic mode when pressed I just split the program to a couple of files.

nested_menus_test.ino (17.5 KB)

#if 1
  return ( keypad.getKey() );
#endif

What is the purpose of this at the end of the ReadKey() function ?

In that part of the code, you have this:

            switch( keyval )
            {
                case 'A':

               char c;
               while (keyval) 
               {
               c = keyval;     // read from Serial Monitor
               u8g2log.print(keyval);               // print to display
               Serial.print(c);                // and print back to monitor
               }

You're inside case 'A' becuase keyval is equal to 'A'.

You then have while(keyval). Now, keyval is never updated inside this while loop and, since it's equal to 'A' (or more specifically, is not zero (0) meaning the while() loop will continue forever), you print keyval to the LCD and spool it out the serial port in the form of variable 'c' in an infinite loop.

What are you trying to do here?