Why does printing onto an LCD break the Code?

[charlix14]

Hello,
i am an arduino newbie and I am trying to programm a menu shown on a 16x2 LCD and a rotary encoder to scroll through.
I want to be able to scroll through the menu points and when pushing the encoder button, go into the selected option and scroll through more options there.

My code is working perfectly fine until I try to print things onto my LCD. Somehow my little Nano seems to have problems reading the encoder correctly when i try to print stuff on the lcd.

What could be the cause of this?
I have been trying to fix this problem for hours now and dont know how to tackle it. I have no clue what to google or what video to watch.

It would be awesome if someone could help me out.

Here is my code:

 #include <LiquidCrystal_I2C.h>


// Encoder Pins
int encoderPinA = 11;
int encoderPinB = 12;
int encoderButton = 10;
int encoderPinALast = LOW;
int encoderPinANow = LOW;
int lastButtonState = 1;
int buttonState = 1;
int holdTime = 0;
int startPressed = 0;
int endPressed = 0;


// Menue - Values
int menuOption = 0;
int Layer = 0;

//LCD
LiquidCrystal_I2C lcd(0x27, 16, 2);


void setup() {
  pinMode (encoderPinA, INPUT_PULLUP);
  pinMode (encoderPinB, INPUT_PULLUP);
  pinMode (encoderButton, INPUT_PULLUP);
  Serial.begin (9600);

  
  lcd.init();
  lcd.backlight();
 

}


void loop () {

 refreshEncoder();
 checkButton();

 refreshLCD();

}

//void loop() {
void refreshEncoder() {
  
  encoderPinANow = digitalRead(encoderPinA);
  if ((encoderPinALast == HIGH) && (encoderPinANow == LOW)) {
    if (digitalRead(encoderPinB) == HIGH) {
      MenuOptionUp();
    } else {
      MenuOptionDown();
    }
  }
  
  encoderPinALast = encoderPinANow;

  delay(100);
}

void MenuOptionUp() {
 if (menuOption <2){
 menuOption++;
    }
  else {
    menuOption = 0;
  }
 Serial.print ("menuOption(Up):");
 Serial.println (menuOption);
}

void MenuOptionDown() {

 if (menuOption > 0){
   menuOption--;
    }
  else {
    menuOption = 2;
  }
 Serial.print ("menuOption(DO):");
 Serial.println (menuOption);
}


void checkButton(){

  buttonState = digitalRead(encoderButton);
  if (buttonState != lastButtonState){

    updateButton();

  }
  lastButtonState = buttonState;
}

void updateButton(){

 if(buttonState == LOW) {
   startPressed = millis();
 }
 else {
   endPressed = millis();

   holdTime = endPressed - startPressed;

   if (holdTime <= 500) {
     if (Layer < 2){
          Layer++;
          }
    } 
    else{
      if (Layer > 0) {
        Layer--;
      }
      
    }
   Serial.print ("Layer:");
   Serial.println (Layer);  
 }     

 delay(50);
}


void refreshLCD() {

lcd.setCursor(0,0);
lcd.print ("MenuOption:");
lcd.setCursor(12,0);
lcd.print (menuOption);

lcd.setCursor(0,1);
lcd.print ("Layer:");
lcd.setCursor(7,1);
lcd.print (Layer);

}

Thank you in advance!

[anon57585045]

I think, you can not read an encoder properly using polling, if there are significant minor delays in loop(). Usually, a quadrature encoder is handled with an ISR.

[jremington]

Remove all the delays and see if that helps.

Please post a link to the encoder. If it is a small knob intended for manual operation, polling should be OK.

Hint: there is no need to "refresh" the LCD if nothing to be displayed has changed. Skipping the unnecessary refresh allows the Arduino to do more important things, like monitor the encoder.

[gilshultz]

Writing to the LCD takes time and some of the libraries even use the dreaded delay() function. This is saying you have blocking code while writing the display. You need to do the LCD printing when you are not scanning the rotary. You can also print only one line or a few characters in between scans of the rotary. Another option is to have the LCD call an external function that scans the rotary. You would have to modify the library to do this. None of these is easy and will take some coding.

[mrburnette]

@charlix14

My code is working perfectly fine until I try to print things onto my LCD.

Then, your code is not working.

Always start by finding an example to use: like a skeleton, you will add code to flesh it out.

Research by constructing a composite query:
"Arduino" + "LCD-1602" +"menu" - Search (bing.com)

Find something close, then modify and enhance.

[sterretje]

Don't print like an idiot (no offence intended) to the LCD. Only update the LCD if there is a change in the data that you want to display has changed or only update it e.g. every second. For the former

void refreshLCD()
{
  // remember the previous menuOption and layer; we assume that you don't have a menuOption -1 and a layer -1
  static int oldMenuOption = -1;
  static int oldLayer = -1;

  // if menuOption or layer changed
  if (menuOption != oldMenuOption || layer != oldLayer)
  {
    oldMenuOption = menuOption;
    oldLayer = layer;

    lcd.setCursor(0, 0);
    lcd.print("MenuOption:");
    lcd.setCursor(12, 0);
    lcd.print(menuOption);

    lcd.setCursor(0, 1);
    lcd.print("Layer:");
    lcd.setCursor(7, 1);
    lcd.print(Layer);
  }
}

Code not compiled nor tested.

[RayLivingston]

The second setCursor call in each pair is not necessary. Add a space to the end of the label string in each pair, and the cursor will already be where you need it to print the value.

[charlix14]

Thats a lot of new terms in a short Awnser. so baisically what you are saying is that I should be using some sort of intterupt to read the encoder, right?

[jremington]

@charlix14

I should be using some sort of intterupt to read the encoder

No, fix the obvious problems in the code first.

[charlix14]

@mrburnette

Always start by finding an example to use: like a skeleton, you will add code to flesh it out.

That is how i got so far in the first place...

[charlix14]

I only put in the delays because I thought it might help...
Same problem without them.

Thanks for the hint with the refresh though.

I am using a KY-040 Joy-it Rotary Encoder...

[jremington]

Unnecessary printing is a delay.

You do NOT need interrupts to read that encoder.

[mrburnette]

I agree with @jremington approach, too, for most users as interrupts can play havoc with the novice programmer (And experienced too!)

Arduino has a library for encoder use that should play nicely; but I have not utilized myself:

• Encoder provides 4X counting mode and highly optimized code (explained below) when running on Teensy or Arduino boards. Chipkit boards have limited support.
  https://www.arduino.cc/reference/en/libraries/encoder/

[bperrybap]

Even better is to only print the labels in setup() since they are not changing.
i.e print "MenuOption:" and "Label:" only once since they don't ever change.

Another thing to speed things up would be to switch over the hd44780 library hd44780_I2Cexp i/o class. It is about double the speed without making any other changes.

As an example it is roughly 1 ms per character to write to the LCD with the LiquidCrystal_I2C library and about half that with with hd44780_I2Cexp i/o class.

--- bill

[bperrybap]

@RayLivingston

The second setCursor call in each pair is not necessary. Add a space to the end of the label string in each pair, and the cursor will already be where you need it to print the value.

I takes the same amount of time to print a space as to set the cursor position.
They both will send 1 instruction to the LCD.

I would guess that printing a space would actually take slightly more time given the overhead of going through the Print class code and then the vtable to get the virtual function to get to the write() function in the LCD library.

--- bill

[bperrybap]

@charlix14

o baisically what you are saying is that I should be using some sort of intterupt to read the encoder, right?

It depends on a few things.
The main one being what is the maximum latency the code can handle?
i.e. what is the maximum amount of time that can elapse between calls to RefreshEncoder() ?

As ball park you can use 1ms per character written or setCursor() for LiquidCrytal_I2C library to estimate the LCD updating time overhead.

As mentioned switching to the hd44780 library can cut the overhead roughly in half.

My recommendation would be to do the math first to see where you are then you can start to approach what and where to optimize to see if you can even get to where you need to be.

If you can't modify the code to limit the amount of overhead from updating the LCD to be under your maximum latency, through some combination of switching libraries and/or reducing what is written to the display,
then using interrupts should be considered.

--- bill

[charlix14]

Thank you all so much for your efforts!
Actually only printing to the LCD when something needs to be changed seems to have solved it.
But only having the problem solved doesnt really satisfy me. I am always a fan of understanding in order to learn properly. So would someone elaborate why my arduino had problems reliably reading the encoder while constantly printing to the lcd?

[bperrybap]

@charlix14

So would someone elaborate why my arduino had problems reliably reading the encoder while constantly printing to the lcd

See post #4 and post #17
Both explain it.

Summary: You are polling the encoder.
printing to the LCD takes time and slows down how fast you can poll.
poll too slowly/not often enough, and things won't work as intended/expected since the encoder can change faster than you are polling it.

Think about watching an intersection and counting cars.
If you blink you likely won't miss any but if you close your eyes for too long you may miss some.

post #17 explains how to calculate the polling latency between polls depending on how much you print to the LCD and which library you use.

--- bill

[charlix14]

Okay thank you very much.
So when continuing this project one of my highest priorities should be to keep the code "unoccupied". So polling the encoder is reliable and subfunctions only start working if an input has been given.
Does that sound right?