Interrupts run multiple times, bizzare stuff

Hi, I’ve a problem with understanding what’s wrong with my code:

#include <LiquidCrystal.h>

LiquidCrystal lcd(4, 5, 6, 7, 8, 9);
volatile int x = 0;


void setup() {
  Serial.begin(9600); //debug
  pinMode(2,INPUT_PULLUP); 
  pinMode(3,INPUT_PULLUP); 
  lcd.begin(16, 2);
  lcd.setCursor(1, 0);
  lcd.print("Wybierz opcje:");
  lcd.setCursor(0, 1);
  lcd.print("<2849_50");
  lcd.setCursor(9, 1);
  lcd.print("5720SX>");
  attachInterrupt(digitalPinToInterrupt(2), interrupts1, CHANGE);
  attachInterrupt(digitalPinToInterrupt(3), interrupts1, CHANGE);
  

  
}

void loop() 
{
  switch(x)
  {
    case 1:
    {
      Serial.println(x); //debug
      ADB2849_50();
    }
    case 2:
    {
      Serial.println(x); //debug
      Serial.println("druga petla"); //debug
      ADB3740SX();
    }
    case -1:
    {
      Serial.println(x); //debug
      Serial.println("-1 petla"); //debug
      ADB5720SX();
    }
    case -2:
    {
      Serial.println(x); //debug
      Serial.println("-2 petla"); //debug
      HDS();
    }
  }
}

void ADB2849_50()
{
  noInterrupts();
  lcd.clear();
  lcd.setCursor (3, 0);
  lcd.print("ADB2849_50");
  lcd.setCursor (0, 1);
  lcd.print("<3740SX");
  lcd.setCursor (10, 1);
  lcd.print("START>");
}
void ADB3740SX()
{
  noInterrupts();
  lcd.clear();
  lcd.setCursor (4, 0);
  lcd.print("ADB3740SX");
  lcd.setCursor (8, 1);
  lcd.print("2849_50>");
}
void ADB5720SX()
{
  noInterrupts();
  lcd.clear();
  lcd.setCursor (3, 0);
  lcd.print("ADB5720SX");
  lcd.setCursor (11, 1);
  lcd.print("HDS>");
  lcd.setCursor (0, 1);
  lcd.print("<START");
}

void HDS()
{
  noInterrupts();
  lcd.clear();
  lcd.setCursor (6, 0);
  lcd.print("HDS");
  lcd.setCursor (0, 1);
  lcd.print("<5720SX");
}


void interrupts1()
{
  noInterrupts();
  x++;
}

void interrupts2()
{
  noInterrupts();
  x=x-1;
}

I want it to change the “pages” in 16x2 lcd menu based on encoder with an button(this code doesn’t include button usage). It goes to the next “pages” that require the value of “x” to be higher than 1, even thought the value is set to 1. Also, i can’t increase the value by rotating the encoder even further.
Results from debbuging:

1
druga petla
1
-1 petla
1
-2 petla
1
void interrupts2()
{
  noInterrupts();
  x=x-1;
}

I didn't read your code that well but when do you turn interrupts back on?

Hmm shouldn't they turn themselves on the moment I rotate then encoder hence adding +1 to x value?

krulzabujca112:
Hmm shouldn’t they turn themselves on the moment I rotate then encoder hence adding +1 to x value?

Hmm, let’s check the guide on noInterrupts().

First, you do not have a break; on the end of each case.

Second, you do not call interrupts2

Third, you don't need to turn off interrupts in an ISR as it is done automatically.

Fourth, you never turn interrupts on again after turning them off.

Fifth, you print with interrupts turned off but printing uses interrupts. This may work in newer versions of the IDE but is generally not recommended.

Ok, I think I fixed all of those and the code looks like this:

#include <LiquidCrystal.h>

LiquidCrystal lcd(4, 5, 6, 7, 8, 9);
volatile int x = 0;


void setup() {
  Serial.begin(9600);
  pinMode(2,INPUT_PULLUP); 
  pinMode(3,INPUT_PULLUP); 
  lcd.begin(16, 2);
  lcd.setCursor(1, 0);
  lcd.print("Wybierz opcje:");
  lcd.setCursor(0, 1);
  lcd.print("<2849_50");
  lcd.setCursor(9, 1);
  lcd.print("5720SX>");
  attachInterrupt(digitalPinToInterrupt(2), interrupts1, CHANGE);
  attachInterrupt(digitalPinToInterrupt(3), interrupts2, CHANGE);
  

  
}

void loop() 
{
  switch(x)
  {
    case 1:
    {
      Serial.println(x); //debug
      ADB2849_50();
      break;
    }
    case 2:
    {
      Serial.println(x); //debug
      Serial.println("druga petla"); //debug
      ADB3740SX();
      break;
    }
    case -1:
    {
      Serial.println(x); //debug
      Serial.println("-1 petla"); //debug
      ADB5720SX();
      break;
    }
    case -2:
    {
      Serial.println(x); //debug
      Serial.println("-2 petla"); //debug
      HDS();
      break;
    }
   break;
  }
}

void ADB2849_50()
{
  lcd.clear();
  lcd.setCursor (3, 0);
  lcd.print("ADB2849_50");
  lcd.setCursor (0, 1);
  lcd.print("<3740SX");
  lcd.setCursor (10, 1);
  lcd.print("START>");
}
void ADB3740SX()
{
  lcd.clear();
  lcd.setCursor (4, 0);
  lcd.print("ADB3740SX");
  lcd.setCursor (8, 1);
  lcd.print("2849_50>");
}
void ADB5720SX()
{
  lcd.clear();
  lcd.setCursor (3, 0);
  lcd.print("ADB5720SX");
  lcd.setCursor (11, 1);
  lcd.print("HDS>");
  lcd.setCursor (0, 1);
  lcd.print("<START");
}

void HDS()
{
  lcd.clear();
  lcd.setCursor (6, 0);
  lcd.print("HDS");
  lcd.setCursor (0, 1);
  lcd.print("<5720SX");
}


void interrupts1()
{
  x++;
}

void interrupts2()
{
  x=x-1;
}

Now the debugger spams 1, no matter which way I rotate the encoder, sometimes it adds something else, for example:

1
1
1
1
1
2
druga petla
1
1
1
1
1
1
1
1
1

every time loop is called you evaluate x and do something.

Are you not more interested in when x changes?

I don't get it, x should change the moment I rotate the encoder, and based on how many times I rotated it overall the loop should evaluate what the x is and do the "case x" based on its value. If my thought process is not valid somewhere can u please point it out?

krulzabujca112:
I don't get it, x should change the moment I rotate the encoder, and based on how many times I rotated it overall the loop should evaluate what the x is and do the "case x" based on its value. If my thought process is not valid somewhere can u please point it out?

x is evaluated, yes. Eventually loop() ends and is called again. x is again evaluated, and so on and so on many thousands of times every second.

you have to do the math…

  attachInterrupt(digitalPinToInterrupt(2), interrupts1, CHANGE);
  attachInterrupt(digitalPinToInterrupt(3), interrupts2, CHANGE);

what happens on each event of the encoder?

this:

void interrupts1()
{
  x++;
}

void interrupts2()
{
  x=x-1;
}

do you see it now?

Should I then do the switch outside of the loop somehow? But even then, if the loop is called again it doesn’t change the x value, so it still stays at 1 and should stay at “case 1” menu untill I rotate the encoder once again to change its value right? If so, how is having this block of script inside the loop a problem?

you can take a look at Kevin Darrah's youtube video, he provides some insight on how you want to determine the direction of an encoder.

You need one ISR to handle one encoder, and it should look to see what happened and update the
count. If you are responding to changes on both signals then you will need to record the previous
state of both signals in variables so you can see what happened.

OK, I modified the code so it looks like this:

#include <LiquidCrystal.h>

LiquidCrystal lcd(4, 5, 6, 7, 8, 9);
volatile int x = 0;
int pinA = 2;
int pinB = 3;
int sygnalA = 0;
int sygnalB = 1;


void setup() {
  Serial.begin(9600); //debug
  lcd.begin(16, 2);
  lcd.setCursor(1, 0);
  lcd.print("Wybierz opcje:");
  lcd.setCursor(0, 1);
  lcd.print("<2849_50");
  lcd.setCursor(9, 1);
  lcd.print("5720SX>");
  attachInterrupt(digitalPinToInterrupt(pinA), przod, RISING);
  attachInterrupt(digitalPinToInterrupt(pinB), tyl, RISING);
  

  
}

void loop() 
{
  switch(x)
  {
    case 1:
    {
      ADB2849_50();
      break;
    }
    case 2:
    {
      ADB3740SX();
      break;
    }
    case -1:
    {
      ADB5720SX();
      break;
    }
    case -2:
    {
      HDS();
      break;
    }
   break;
  }
}

void ADB2849_50()
{
  lcd.clear();
  lcd.setCursor (3, 0);
  lcd.print("ADB2849_50");
  lcd.setCursor (0, 1);
  lcd.print("<3740SX");
  lcd.setCursor (10, 1);
  lcd.print("START>");
}
void ADB3740SX()
{
  lcd.clear();
  lcd.setCursor (4, 0);
  lcd.print("ADB3740SX");
  lcd.setCursor (8, 1);
  lcd.print("2849_50>");
}
void ADB5720SX()
{
  lcd.clear();
  lcd.setCursor (3, 0);
  lcd.print("ADB5720SX");
  lcd.setCursor (12, 1);
  lcd.print("HDS>");
  lcd.setCursor (0, 1);
  lcd.print("<START");
}

void HDS()
{
  lcd.clear();
  lcd.setCursor (6, 0);
  lcd.print("HDS");
  lcd.setCursor (0, 1);
  lcd.print("<5720SX");
}


void przod()
{
  detachInterrupt(digitalPinToInterrupt(pinA));
  sygnalA = 1;
  if (sygnalB == 1)
  {
    x++;
  }
  if (sygnalB == 0)
  {
    x--;
  }
  Serial.println(x);
  attachInterrupt(digitalPinToInterrupt(pinA), przod2, FALLING);
}

void przod2()
{
  detachInterrupt(digitalPinToInterrupt(pinA));
  sygnalA = 0;
  if (sygnalB == 1)
  {
    x++;
  }
  if (sygnalB == 0)
  {
    x--;
  }
  Serial.println(x);
  attachInterrupt(digitalPinToInterrupt(pinA), przod, RISING);
}
void tyl()
{
  detachInterrupt(digitalPinToInterrupt(pinB));
  sygnalB = 1;
  if (sygnalB == 1)
  {
    x++;
  }
  if (sygnalB == 0)
  {
    x--;
  }
  Serial.println(x);
 attachInterrupt(digitalPinToInterrupt(pinB), tyl2, FALLING);
}

void tyl2()
{
  detachInterrupt(digitalPinToInterrupt(pinB));
  sygnalB = 0;
  if (sygnalB == 1)
  {
    x++;
  }
  if (sygnalB == 0)
  {
    x--;
  }
 attachInterrupt(digitalPinToInterrupt(pinB), tyl, RISING);
 Serial.println(x);
}

It no longer CONTINUOUSLY throws in a random numbers/stops at 1, but it sometimes adds/subtracts ones no matter what direction I rotate the encoder to. Practically the menu skips the “pages” at random.

BulldogLowell:

void interrupts2()

{
  noInterrupts();
  x=x-1;
}




I didn't read your code that well but when do you turn interrupts back on?

That's exactly the wrong question to be asking. You should be asking "Why do you turn interrupts off in the interrupt handler?". Interrupts are ALREADY turned off when the interrupt handler is called, and will be turned back on when the handler exits, regardless of the presence, or absence, of that noInterrupts() call.

There are, of course, NUMEROUS other places where he sprinkles in noInterrupts() where it has no business being. Those, WILL cause problems.

Regards,
Ray L.

RayLivingston:
That's exactly the wrong question

as pointed out by @UKHeliBob, above.

I think the thread has progressed since then

:slight_smile: :slight_smile:

krulzabujca112:
It no longer CONTINUOUSLY throws in a random numbers/stops at 1, but it sometimes adds/subtracts ones no matter what direction I rotate the encoder to. Practically the menu skips the "pages" at random.

Kevin points out in his video some wiring diagrams and some of the reasons for all of that.

What type of encoder are you using and how is it wired?

OK, I modified the code so it looks like this:

Your approach to reading the encoders by switching RISING and FALLING interrupts is not correct. Use CHANGE, and then read the pin to decide what transition occurred.

I think I understand now, but I won’t be able to access arduino till tommorows morning and I pretty much need to have the code working properly when I get there.
Code:

#include <LiquidCrystal.h>

LiquidCrystal lcd(4, 5, 6, 7, 8, 9);
volatile int x = 0;
int ostatniStan = 0; //last state that pinA was in
int obecnyStan = 0; //current state of pinA
int pinA = 2;
int pinB = 3;


void setup() {
  Serial.begin(9600);
  pinMode(pinA,INPUT); 
  pinMode(pinB,INPUT); 
  START();
  ostatniStan = digitalRead(pinA); //sets last state of pin A
  attachInterrupt(digitalPinToInterrupt(2), interrupts1, CHANGE);
  

  
}

void loop() 
{
  switch(x)
  {
    case 1:
    {
      ADB2849_50();
      break;
    }
    case 2:
    {
      ADB3740SX();
      break;
    }
    case -1:
    {
      ADB5720SX();
      break;
    }
    case -2:
    {
      HDS();
      break;
    }
   break;
  }
  if (x > 2 || x < -2)
  {
    x = 0;
    START();
  }
}

void START()
{
  lcd.begin(16, 2);
  lcd.setCursor(1, 0);
  lcd.print("Wybierz opcje:");
  lcd.setCursor(0, 1);
  lcd.print("<2849_50");
  lcd.setCursor(9, 1);
  lcd.print("5720SX>"); 
}

void ADB2849_50()
{
  lcd.clear();
  lcd.setCursor (3, 0);
  lcd.print("ADB2849_50");
  lcd.setCursor (0, 1);
  lcd.print("<3740SX");
  lcd.setCursor (10, 1);
  lcd.print("START>");
}
void ADB3740SX()
{
  lcd.clear();
  lcd.setCursor (4, 0);
  lcd.print("ADB3740SX");
  lcd.setCursor (8, 1);
  lcd.print("2849_50>");
  lcd.setCursor (0, 1);
  lcd.print("<START");
}
void ADB5720SX()
{
  lcd.clear();
  lcd.setCursor (3, 0);
  lcd.print("ADB5720SX");
  lcd.setCursor (11, 1);
  lcd.print("HDS>");
  lcd.setCursor (0, 1);
  lcd.print("<START");
}

void HDS()
{
  lcd.clear();
  lcd.setCursor (6, 0);
  lcd.print("HDS");
  lcd.setCursor (0, 1);
  lcd.print("<5720SX");
  lcd.setCursor (10, 1);
  lcd.print("START>");
}


void interrupts1()
{
  obecnyStan = digitalRead(pinA) //sets current state of pin A after rotating the encoder
  if (obecnyStan != ostatniStan) //checks if last and current state differ
  {
    if (digitalRead(pinB) != obecnyStan)//if state on pinB is different than the state of pinA - adds value, otherwise it subtracts it
    {
      x++;
    }
    else
    {
      x--;
    }
  }
  ostatniStan; = obecnyStan; //sets last state of pinA on the same value as the current one
}

if someone could check if I understood and coded everything correctly I would appreciate it :).

  obecnyStan = digitalRead(pinA) //sets current state of pin A after rotating the encoderNeeds a semicolon.