Rotary Encoder won't cooperate...

Hello i have been trying for a week to get the rotary encoder working with no results to show for my effort.

The main concern is the code from three ino files below.
The void loop code is here :

void loop()
{
  //Wifi Connection
  if (!client.connected())
    {
      reconnect();
    }
    if(client.connected())
    {
      connection_attempt=0;
    }
  //Time
  if((millis()-time_update_counter)>= 1000)
  {
    timeClient.update();
    //Serial.println(timeClient.getFormattedTime());
    time_update_counter = millis();
  }
  //Monitor Inputs
  //if((millis()-input_read_counter) >= 70)
  //{
    read_inputs();
    input_read_counter = millis();
  //}
  //Control OUTPUTS
  if((millis()- control_timer) >=100)
  {
    output_control();
  }
  //Control LCD
  if(millis() - lcd_update_timer >=500)
  {
    lcd_print(current_menu,option);
    lcd_update_timer = millis();
  }
  if(millis() - lcd_logic_loop_timer >= 100)
  {
    //lcd_loop(); //Discarded file
     menu_loop();
     lcd_logic_loop_timer = millis();
  }
}

The loop calls the function to read the inputs

void read_inputs()
{
  //Buttons
  for(int i=0; i <=3; i++) // Extra buttons i<=5 (if (i<=4 normal buttons , if i >=4 menu buttons))
  {
    if(mcp.digitalRead(button[i]) == HIGH)
    {
      button_pressed[i]=true;
      Serial.print("Button ");
      Serial.print(i);
      Serial.print(" pressed");
    }
  }
  if(mcp.digitalRead(encoder_button_pin) == LOW)
  {
    encoder_button_pressed = true;
    Serial.print("  Encoder button : ");
    Serial.println(mcp.digitalRead(encoder_button_pin));
  }
  // Encoder
  int current_encoder_state = mcp.digitalRead(encoder_pin[0]);
  if(previous_encoder_state != current_encoder_state)
  {
    
    if(mcp.digitalRead(encoder_pin[1]) !=current_encoder_state)
    {
      encoder_direction = "CCW";
      Serial.println("CCW");
    }
    else if(mcp.digitalRead(encoder_pin[1]) == current_encoder_state)
    {
      encoder_direction = "CW";
      Serial.println("CW");
    }
    encoder_rotated = true;
  }
  previous_encoder_state = current_encoder_state;
}

And finally the encoder inputs are fed into the menu_loop

void menu_loop()
{ 
  Serial.print(" Current Menu : ");
  Serial.print(current_menu);
  Serial.print(" Option No : ");
  Serial.println(option);
  if(encoder_rotated)
  {
  	encoder_rotated = false;
  	if(encoder_direction == "CCW")
  	{
      encoder_direction = "Na";
  		if(option-- >=0)
  		{
  			option --;
  		}
  		else
  		{
  			option = 0;
  		}
  	}
  	else if(encoder_direction == "CW")
  	{
      encoder_direction = "Na";
  		if(current_menu == 0 )
  		{
  			//Nothing main screen
  		}
  		if(current_menu == 1)
  		{
  			if(option++ <=2)
  			{
  				option ++;
  			}
  		}
  		if(current_menu == 2)
  		{
  			if(option++ <=2)
  			{
  				option ++;
  			}
  		}
  		if(current_menu == 3)
  		{
  			if(option++ <=2)
  			{
  				option ++;
  			}
  		}
  		if(current_menu == 4)
  		{
  			if(option++ <=2)
  			{
  				option ++;
  			}
  		}
  		if(current_menu == 5)
  		{
  			if(option++ <=2)
  			{
  				option ++;
  			}
  		}
  	}
  }
  if(encoder_button_pressed)
  {
  	encoder_button_pressed = false;
  	if(current_menu == 0)
  	{
  		current_menu = 1;
     previous_menu = 0;
  	}
  	if(current_menu == 1 && previous_menu == 1)
  	{
  		if(option == 0)
  		{
  			current_menu =2;
        previous_menu =1;
  			option = 0;
  		}
  		if(option == 1)
  		{
  			current_menu =3;
  			option = 0;
  		}
  		if(option == 2)
  		{
  			current_menu = 0;
  			option = 0;
  		}

  	}
  }
}

The problem is that the encoder output.
When turned clockwise it shows CW many times (like many movements occurred) and when turned counter clockwise it outputs a CW move followed by CCW (this may differ for example it could output CW CW CW CCW).
I believe the problem is related to the reading speed of the MCU.
I am using an ESP8266 MCU and the IO expander is the MCP23017.

Also i have not yet understood how the interrupts work with the MCP that is why i tried avoiding them for this sketch.
Edit : Also mind that the menu_loop void is not complete. I was in the middle of testing the thing again so i did not write the full code.

Please post your complete sketch inside one set of code tags. It will be much more helpful.

Inside your read_inputs() function you have this:

   if(mcp.digitalRead(encoder_pin[1]) !=current_encoder_state)
    {
      encoder_direction = "CCW";
      Serial.println("CCW");
    }
    else if(mcp.digitalRead(encoder_pin[1]) == current_encoder_state)
    {
      encoder_direction = "CW";
      Serial.println("CW");
    }

which means you are reading encoder_pin[1] twice and may not get the same value if it is moving. It is also wasteful since if a digitalRead != current_encoder_state, and a digitalRead() can only give you two values, you already know what the else clause will be.

I would also suspect that you will need to learn about the interrupt capabilities of the MCP since encoder movement can be very quick and you very well may miss some pulses.

Of course. The code is for a thermostart circuit i have designed. There is the full code.
Due to the large number of lines it would take 5 posts to post the full code if you need to see the full code i could upload it.
As a first change i will try replacing this

if(mcp.digitalRead(encoder_pin[1]) !=current_encoder_state)
    {
      encoder_direction = "CCW";
      Serial.println("CCW");
    }
    else if(mcp.digitalRead(encoder_pin[1]) == current_encoder_state)
    {
      encoder_direction = "CW";
      Serial.println("CW");
    }

with this

if(mcp.digitalRead(encoder_pin[1]) !=current_encoder_state)
    {
      encoder_direction = "CCW";
      Serial.println("CCW");
    }
    else
    {
      encoder_direction = "CW";
      Serial.println("CW");
    }

Edit : Also the problem seems to be that it reads the encoder multiple times and gets double or false results.

If it would take 5 posts to deliver your code, you have not reduced the problem down to the minimal program that exhibits the problem. I'm pretty sure it will not take 10K x 5 to exhibit the problem.

You can also attach your complete code if it exceeds the post limit.

if(mcp.digitalRead(encoder_pin[1]) !=current_encoder_state)
    {
      encoder_direction = "CCW";
      Serial.println("CCW");
    }

Serial printing is slow, and causes the program to miss encoder transitions. Don't print in the encoder read loop.

Thank you for your feedback. I commented out the serial output inside the reading loop.
As for the code the only encoder reading is happening in the code i posted everything else is just if statements that compare the encoder_direction variable.

I will post the complete code but keep in mind that there will be bugs (since i could not solve the encoder problem and needed to keep writing code i replaced the encoder with three buttons to get the thermostat prototype working.

I will post both versions of the code to have a look at.

Here is the code for the encoder sketch and the button sketch i am currently working on actively.

Thermostat Sketch.zip (14.4 KB)

There seems to be a problem in your menu_loop()

  		if(option-- >=0)
  		{
  			option --;
  		}
  		else
  		{
  			option = 0;
  		}

Code like this will cause option to be decremented twice. You have similar code for incrementing. That may be why your encoder seems to "jump".

E.g. option = 0. The if() statement will evaluate true and, after the evaluation, option will be decremented to -1. The true clause of the if() will be executed which will make option -2.

As a side note, having all that code spread out over several files/tabs makes it far more difficult to follow. Also, having code that is marked "not used" just adds noise.

Thank you for your response.
The part of the code you pointed out is used due to the rapid reading of inputs. Many times the option counter would go to -1

I think in the button version i uploaded the problem is fixed the new code being

if(option-- >0)
  		{
  			option --;
  		}

So i do not decrement when option is equal to 0 only when its more than 0.

As for the encoder jumping my experience was not with a value being adjusted. Rather it had to do with the printed message (CW , CCW) it would print CW twice or CW followed by CCW and such.

You are still decrementing twice. Once inside the if() statement, once in the true clause of the if().

if(option > 0 ) {
  option--;
}

Sorry forgot to edit that part. My code exactly how you wrote it. I managed to troubleshoot the incrementing part. But the problem with the encoder still persists. The code to read the encoder pins is this.

I mostly get good readings on the CW movement (it prints CW twice but i can live with 2 correct readings in sequence). The major problem that i don't even know whats causes it is the CCW movement. It prints CW followed by CCW other times only 3 or 4 CW messages etc.

if(mcp.digitalRead(encoder_pin[1]) !=current_encoder_state)
    {
      encoder_direction = "CCW";
    }
    else
    {
      encoder_direction = "CW";
    }
Serial.println(encoder_direction);

Edit : The problem persists even when the encoder reading is the only function called inside void loop. Tried tweaking the frequency of the readings (the timer i set reads every 5 ms , tried many increments to 300 ms with no alternate results. That tells me the problem lies in the read_input function that reads the pins.

Your code does not account for all the different states. If you look at the encoder library and adapt it to use your mcp, you get something like this

//                           _______         _______
//               Pin1 ______|       |_______|       |______ Pin1
// negative <---         _______         _______         __      --> positive
//               Pin2 __|       |_______|       |_______|   Pin2

//   new   old
//  p2 p1 p2 p1 Result
//  -- -- -- -- ------
//  0  0  0  0  no movement
//  0  0  0  1  +1
//  0  0  1  0  -1
//  0  0  1  1  +2  (assume pin1 edges only)
//  0  1  0  0  -1
//  0  1  0  1  no movement
//  0  1  1  0  -2  (assume pin1 edges only)
//  0  1  1  1  +1
//  1  0  0  0  +1
//  1  0  0  1  -2  (assume pin1 edges only)
//  1  0  1  0  no movement
//  1  0  1  1  -1
//  1  1  0  0  +2  (assume pin1 edges only)
//  1  1  0  1  -1
//  1  1  1  0  +1
//  1  1  1  1  no movement

// Simple, easy-to-read "documentation" version :-)
//
void update(void) {
  static uint8_t state = 0;
  uint8_t s = state & 3;
  if (mcp.digitalRead(encoder_pin[0])) s |= 4;
  if (mcp.digitalRead(encoder_pin[1])) s |= 8;
  switch (s) {
    case 0: case 5: case 10: case 15:
      break;
    case 1: case 7: case 8: case 14:
      position++; break;
    case 2: case 4: case 11: case 13:
      position--; break;
    case 3: case 12:
      position += 2; break;
    default:
      position -= 2; break;
  }
  state = (s >> 2);
}

where ‘position’ is a global variable you have defined

I implemented the code and commented out everything else inside the loop.

While printing only the position variable i get increments of four per encoder rotation (CW or CCW).

It works consistently which i need right now but to be honest i do not fully comprehend the code used just yet.

I will have to go over your code to see how it works but still with a fast glance at it i can tell that still its like the states

0110 and 1100 are being read and are being read twice.

Hi,
Have you got code JUST for the encoder, forget the other stuff, just run code for the encoder and make sure you are reading it correctly.
Just use the serial monitor to get your results.
If you haven't got code just for the encoder, WHY, it should have been part of writing your code in stages to minimise the bugs.

The rest of the code is not necessary at this time until you get the CW and CCW situation solved.
Can you post a circuit diagram of how you have your encoder wired to the ESP8266?
Have you googled esp8266 encoder

Tom.... :slight_smile:

The idea in reply #13 isn't just a suggestion... it's standard procedure for anyone that builds more than once (after finding out the hard way). It is the main reason why example sketches are provided with device libraries.

ChristosS:
I implemented the code and commented out everything else inside the loop.

While printing only the position variable i get increments of four per encoder rotation (CW or CCW).

It works consistently which i need right now but to be honest i do not fully comprehend the code used just yet.

I will have to go over your code to see how it works but still with a fast glance at it i can tell that still its like the states

0110 and 1100 are being read and are being read twice.

They are only being read once. The big difference is that the two pins current state are stored in bits 2 & 3 and the previous values are stored in bits 0 and 1 rather than 'currentValue' and 'previousValue'

It is not uncommon to get many more quadrature transitions than expected but consistency is more important

Hi,
Can I suggest you put 0.1uF capacitors, one on each input, between input and gnd, I found this also made a difference to readability in some cases. Depending on the quality of the encoder switches.

Tom... :slight_smile:

Hello again , yes i am running only the code for the encoder with a print.

void loop()
{   
  
 update();
 Serial.println(position);
}
void update(void) {
  static uint8_t state = 0;
  uint8_t s = state & 3;
  if (mcp.digitalRead(encoder_pin[0])) s |= 4;
  if (mcp.digitalRead(encoder_pin[1])) s |= 8;
  switch (s) {
    case 0: case 5: case 10: case 15:
      break;
    case 1: case 7: case 8: case 14:
      position++; break;
    case 2: case 4: case 11: case 13:
      position--; break;
    case 3: case 12:
      position += 2; break;
    default:
      position -= 2; break;
  }
  state = (s >> 2);
}

the position variable is declared above void setup()
as follows

//Variable Header File
#include "VARIABLES.h"
//Wifi MQTT
#include <ESP8266WiFi.h>
#include <PubSubClient.h>
//I2C
#include <Wire.h>

//IO Expander MCP232017
#include "Adafruit_MCP23017.h"
Adafruit_MCP23017 mcp;

//NTP Time
#include <NTPClient.h>
#include <WiFiUdp.h>
WiFiUDP ntpUDP;
NTPClient timeClient(ntpUDP, ntp_server , OFFSET, UPDATE_INTERVAL);

//LCD
#include <LiquidCrystal_I2C.h>
#include <LiquidMenu.h>
LiquidCrystal_I2C lcd(LCD_ADDRESS, LCD_LENGTH, LCD_HEIGHT);
#include "LCD_MENU.h"
//WIFI
WiFiClient espClient;
PubSubClient client(espClient);
int position =0;

Also yes i have googled … and i mean a lot … a great page that worked for me in the past was this https://forum.arduino.cc/index.php?topic=367483.0

To add to that i continued with the code by replacing the encoder with 3 buttons as a last resort just to get the thermostat working (i am waiting on the parts for the project). In case i need to install it and had not resolved this issue. I also made it easy to implement the encoder once i get it working in the code.

Anyway i am posting the schematic of the circuit bellow.

Hi,
OPs circuit;

Why are you putting the encoder through an expansion port?
Why can't you connect it directly to the MCU?

Tom.... :slight_smile:

I did not want to take up any more pins on the MCU since i have broken out the remaining pins to pads for future expansion. And since i had the encoder working before using the mcp i figured it would not hurt to connect directly to the expander