Rotery encoder in menu?

I've made a controlling system for my air pumps in my garden pond.

As a part of that system I have 3 parameters I want to be able to change by a simple menu system.

I've got a rotary encoder and use the library "SimpleRotary" from the Arduino lib pool and tested it and it works nicely - I ONLY uses PUSHED, CW and CCW turning - I commented out a line not used in the ccp-file according to the compiler.

A click on the encoder should bring me from NORMAL-mode til MENU-mode and show me the first menu item of 4 = "TURN for choosing parameters to change or CLICK for return to normal mode" (translated from my danish text in the end part of the code.
A CLICK should bring me back to NORMAL-mode and TURNING should show me the 3 other menu items BUT ...

it seems like NOTHING is coming back from the encoder - I've marked the problematic line in the code here:

#include <LCD_I2C.h>
#include <SimpleRotary.h>

String actualMachineState = "";

LCD_I2C lcd(0x27);                            // Default address of most LCDs

// Pin A, Pin B, Button Pin
SimpleRotary rotary(8,9,7);

void lcdShow(String line0, String line1) {
  lcd.clear(); lcd.print(line0);
  lcd.setCursor(0, 1); lcd.print(line1);
  delay(400);       // just to prevent flickering sometimes
}

void setup() {
  Serial.begin(9600);
  lcd.begin(); lcd.backlight();
  actualMachineState = "NORMAL";
}

byte checkEncoder() { 
  byte i = 0;
  // 0 = not pushed, 1 = pushed
  if (rotary.push() != 0) {
    // 1 is changed to 3 instead !!!!!!!
    i = 3;
    } 
  else {
    // 0 = not turning, 1 = CW, 2 = CCW
    i = rotary.rotate();
  }
  if (true) {
    switch(i) {
      case 3:
        Serial.println("Pushed");
        break;
      case 2:
        Serial.println("CCW");
        break;
      case 1:
        Serial.println("CW");
        break;
    }
  }
  return i;
}



void loop() {
  if (checkEncoder() == 3 || actualMachineState == "MENU") { manageMenues(); }
  if (actualMachineState == "NORMAL" || actualMachineState == "PAUSE" || actualMachineState == "ONEHOT" || actualMachineState == "TWOHOT")  {
    lcdShow("Normal", "Pumping !");
  }
}

void manageMenues() {
  byte encoder = 0;
  static byte menuState;
  if (actualMachineState != "MENU") {
    actualMachineState = "MENU";
    menuState = 1;
  } 
  else {
    // we ARE in MENU-mode
    encoder = checkEncoder();      // HERE NOTHING COMES BACK FROM THE ENCODER 
Serial.print("encoder = "); Serial.println(encoder);
    if (encoder == 3) {
      // go back to NORMAL-mode
      menuState = 0;
      actualMachineState = "NORMAL";
      } 
    else if (encoder == 2 ) {     // CCW turning
      menuState = menuState - 1;
      if (menuState <= 1) { menuState = 4; }     
      }
    else if (encoder == 1 ) {     // CW turning
      menuState = menuState + 1;
      if (menuState >= 5) {menuState = 1; }
    }
  }
  showMenuStateText(menuState);
}

void showMenuStateText(byte mState) {
  switch (mState) {
  case 1:
    lcdShow("Drej for juster","   ellers KLIK !");
    break;
  case 2:
    lcdShow("Justering af","Max. temp:");
    break;
  case 3:
    lcdShow("Justering af","Max. pumpetid:");
    break;
  case 4:
    lcdShow("Justering af","Pausetid:");
    break;
  }
}

I will point out that I've NOT finished the menu structure - I just want to test the GO IN and OUT of menu mode and the showing of menu items when in menu mode.

What is wrong in the checkEncoder code ?

The encoder library looks pretty basic and I suspect that it doesn't work well with delay. Your LCD display routine is likely causing you to miss almost all encoder activity.

Hello
Clean-up your sketch wrt the IPO model.
Design input-, process- and output- funktions.
Input: read endcoder
Process: handle input data to result in counters and/or stati
Out: view predefined LCD menus with om information from counters and/or stati
Each function can be tested separately before integration.
Have a nice day and enjoy coding.

THX, It really helps me :crazy_face:

Hello
Each LCD menue will have two parts:
Part one -> Setup text contants
Part two -> Loop to fill value from variables in case of value changes
This can easly desigen by the using of the struct instruction to build LCD menue objects. The object contains the cursor data and text strings to the values to be displays and the addresses of the variables.
Have a nice day and enjoy coding.

I DON'T get you at ALL :worried:

Would it be better to use interrupts for the rotery encoder ?

I've searched out there and found some 'solutions' but what should I choose - there are some with interrupt on the BUTTON output and no interrupts on the A and B outputs of the encoder.
In the UNO-clone I only have interrupts on pin 2 and 3 - how should I deal with that - interrupt on the BUTTON (pin2) and A on pin3 or ... ?

Maybe use 3 buttons instead of the rotary encoder - a SELECT and 2 UP/DOWN buttons ?

I never tried thia before so what do the experts in here advise me to do ?

Hello
I´m using this Rotary Encoder libary for my projects.

Refering to what you wrote in your earlier postings - why do you think it's the rotary encoder library causing the problem ?

If I test SimpleRotary as the example in your library my works nicely too.

If I comment out the delay in my lcdShow my code works better, but still it's missing some turns and clicks on the button - and missing so much that I will say it's not useable.

Just try shift to another lib is a never ending story I think - there are lot of rotary encoder lib out there.

I think more drastic changes have to be made - but I'm a novice so some CONSTRUCTIVE advices will be appreciated - thx

Maybe using Interrups or buttons instead of retary encoder ... I don't know.

In theory, there is no difference between theory and practice. In practice, there is!
Have a nice day and enjoy coding.

?????
What / who are you ... a troll or what ?

You're doing too much unconditional display stuff.

If you're in menu mode, you check the encoder and serial print the result. Repeatedly. You're running at a catastrophically slow baud rate, so I wouldn't be surprised if that's blocking as it fills up the serial output buffer.

Jack up the baud rate and move the serial print into each of the sections where some encoder event actually happened. If the encoder returned zero, there's little point remarking on it.

Similarly, you call lcdshow repeatedly even if nothing has changed. Some more complex logic might help, but as a first attempt, use of millis to restrict the call in loop to once a second may improve matters.

The key is to check the encoder as often as possible. You may need to structure your code to ensure that anything time consuming is done in smaller pieces to let you get back to the encoder quickly.

Interrupts are what you use when you absolutely have to and know what you're doing. There are plenty of speed improvements you can make to what you currently have before that becomes necessary.

THX for the explanation !

Yeah I deleted the delay() I had in the lcdShow() and only shows the texts when they'ra changed and that improved it ... but still I need to do more of that stuff ... so I'll carry on !

Thx !

#wildbill
These small tricks got me way way further up the hill - thx a lot - and convinced me that I can do the job.
Your wrote: "You're running at a catastrophically slow baud rate" - do you mean the 9600 for the serial print OR the communication with the LCD - I think changing the rate to the LCD is over my skills. If you mean the serial terminal then it will be deleted in the final version.

I was referring to the 9600. Even if it will eventually go away, it may be causing problems now. Sadly, many examples use that speed so it gets copied again and again.

If you're printing a little bit, occasionally, it doesn't matter, but if you print on every iteration of loop, it can have blocking consequences that aren't easy to figure out, as you have observed.

THX again !