Rotation encoder: interrupts are neglected when in While Loop

[fanofdan]

Hi all,
I am not a very experienced Arduino programmer and I could use help on the following issue:
I want to make a windingmachine with a 12 volt motor. On this machine I want to use a rotary-encoder (KY-040) to step through some menu options to set number of turns, motorspeed, pauze and restart the winding etc. I use the KY040rotary library and I (try to) use interrupts to trigger the rotary actions.
However when I have started the windup() function, where a While loop runs until the number of turns is reached, the interrupts of the rotary-encoder are not triggered.
Please, can anyone help me on my way?
Here is my code:

#include "KY040rotary.h"
#include <Wire.h> 
#include <LiquidCrystal_PCF8574.h>
#include <L298N.h>

// Rotary Encoder Module connections
const int pinCLK=2;    // CLOCK signal
const int pinSW=3;   // Rotary Encoder Switch
const int pinDT=4;    // DATA signal

// init the button
KY040 Rotary(pinCLK, pinDT, pinSW);

// Motor Pin definition
const unsigned int IN1 = 7;
const unsigned int IN2 = 8;
const unsigned int EN = 9;

const int IN_D6 = 6; // digital input

// Create one motor instance
L298N motor(EN, IN1, IN2);

int turns = 0;
int speed = 50;
boolean direction = true;
boolean state = true;
int pauzeMenuItem = 0;

//init LiquidCrystalDisplay
LiquidCrystal_PCF8574 lcd(0x27);  // set the LCD address to 0x27 for a 16 chars and 2 line display

void OnButtonClicked(void) {
  Serial.println("Button 1: clicked");
  pushButton();
}
void OnButtonLeft(void) {
  Serial.println("Button Left");
  rotateMenu(2);
}
void OnButtonRight(void) {
  Serial.println("Button right");
  rotateMenu(1);
}

/* Handlers for interrupts mode*/
  void RSW1_SwitchInterruptHandler(void) {
    Rotary.HandleSwitchInterrupt();
  }
  void RSW1_RotateInterruptHandler(void) {
    Rotary.HandleRotateInterrupt();
 }

int sequenceStep = 0;
char *sequenceStepText[] = {"Start",
                            "Windings (1000)",
                            "Windings (100)",
                            "Start winding",
                            "Winding",
                            "Pauze",
                            "Continue",
                            "Rewind",
                            "Cancel",
                            "Finished"};

void setup() {
  pinMode (IN_D6, INPUT);   //Counter pulse
  int error;

  // Used to display information
  Serial.begin(115200);
  // Wait for Serial Monitor to be opened
  while (!Serial)
  {
    //do nothing
  }

  //Set up lcd
  Wire.begin();
  Wire.beginTransmission(0x27);
  error = Wire.endTransmission();
  if (error == 0) {
    Serial.println(": LCD found.");
    lcd.begin(16, 2);  // initialize the lcd
  } else {
    Serial.println(": LCD not found.");
  }  // if

  lcd.setBacklight(255);
  lcd.home();
  lcd.clear();
  lcd.setCursor(0,0); //Defining positon to write from first row, first column .
  lcd.print("Winder");
  lcd.setCursor(0,1); //Second row, first column
  lcd.print("Preparation"); 

  /* Init in interrupts mode*/
    if ( !Rotary.Begin(RSW1_SwitchInterruptHandler, RSW1_RotateInterruptHandler) ) {
    Serial.println("unable to init rotate button");
    while (1);
  }

  // init callbacks
  Rotary.OnButtonClicked(OnButtonClicked);
  Rotary.OnButtonLeft(OnButtonLeft);
  Rotary.OnButtonRight(OnButtonRight);

  Serial.println("KY-040 rotary encoder OK");
  lcd.clear(); //clear the whole LCD
  lcd.setCursor(0,0); 
  lcd.print(sequenceStepText[sequenceStep]);
}


void loop() {
  Rotary.Process( millis() );
}

void pushButton() {
  Serial.println("pushButton pressed");
  Serial.println(sequenceStep);
  delay(10); 
  switch(sequenceStep) {
    case 0:             // Start --> Windings (1000)
      sequenceStep++;
      lcd.clear();
      lcd.setCursor(0,0); 
      lcd.print(sequenceStepText[sequenceStep]);
      lcd.setCursor(0,1); 
      lcd.print(turns);
      break;
    case 1:             // Windings (1000) --> Windings (100)
      sequenceStep++;
      lcd.clear();
      lcd.setCursor(0,0); 
      lcd.print(sequenceStepText[sequenceStep]);
      lcd.setCursor(0,1); 
      lcd.print(turns);
      break;
    case 2:             // Windings (100) --> Start Winding
      sequenceStep++;
      lcd.clear();
      lcd.setCursor(0,0); 
      lcd.print(sequenceStepText[sequenceStep]);
      lcd.setCursor(0,1); 
      lcd.print(turns);
      break;
    case 3:             // Start Winding --> Winding
      sequenceStep++;
      lcd.clear();
      lcd.setCursor(0,0); 
      lcd.print(sequenceStepText[sequenceStep]);
      lcd.setCursor(0,1); 
      lcd.print(turns);
      lcd.setCursor(6, 1);
      lcd.print("Speed ");
      lcd.print(speed);
      windup();
      sequenceStep = 9;
      lcd.clear();
      lcd.setCursor(0,0); 
      lcd.print(sequenceStepText[sequenceStep]);
      break;
    case 4:             //Winding --> Pauze
      motor.stop();
      sequenceStep++;
      lcd.clear();
      lcd.setCursor(0,0); 
      lcd.print(sequenceStepText[sequenceStep]);
      break;
    case 8:             //Cancel --> Start
      sequenceStep = 0;
      lcd.clear();
      lcd.setCursor(0,0); 
      lcd.print(sequenceStepText[sequenceStep]);
      break;
    case 9:             //Finished --> Start
      sequenceStep = 0;
      lcd.clear();
      lcd.setCursor(0,0); 
      lcd.print(sequenceStepText[sequenceStep]);
      break;
  } 
  
}

void rotateMenu(int rotationDir) { 
  boolean rotationDirection;
  switch(rotationDir) {
    case 0:
      Serial.println("rotary bounced!");
      exit;
    case 1:
      rotationDirection = true;
      break;
    case 2:
      rotationDirection = false;
      break;
  }
  switch(sequenceStep) {
    case 1:             // Windings (1000)
      Serial.print("sequenceStep = ");
      Serial.println(sequenceStep);
      if (rotationDirection) {
        turns = turns + 1000;
      } else {
        turns = turns - 1000;
        if (turns < 0) {turns = 0;}
      }
      lcd.setCursor(0,1); 
      lcd.print("     ");
      lcd.setCursor(0,1); 
      lcd.print(turns);
      break;
    case 2:             // Windings (100)
      Serial.print("sequenceStep = ");
      Serial.println(sequenceStep);
      if (rotationDirection) {
        turns = turns + 100;
      } else {
        turns = turns - 100;
        if (turns < 0) {turns = 0;}
      }
      lcd.setCursor(0,1); 
      lcd.print("     ");
      lcd.setCursor(0,1); 
      lcd.print(turns);
      break;
    case 3:             // Start Winding
      Serial.print("sequenceStep = ");
      Serial.println(sequenceStep);
      if (rotationDirection) {
        speed = speed + 10;
        if (speed > 250) {speed = 250;}
      } else {
        speed = speed - 10;
        if (speed < 10) {speed = 10;}
      }
      lcd.setCursor(6, 1);
      lcd.print("         ");
      lcd.setCursor(6, 1);
      lcd.print("Speed ");
      lcd.print(speed);
      break;
    case 4:             // Winding
      Serial.print("sequenceStep = ");
      Serial.println(sequenceStep);
      if (rotationDirection) {
        speed = speed + 10;
        if (speed > 250) {speed = 250;}
      } else {
        speed = speed - 10;
        if (speed < 10) {speed = 10;}
      }
      lcd.setCursor(12, 1);
      lcd.print("    ");
      lcd.setCursor(12, 1);
      lcd.print(speed);
      break;
    case 5:             // Pauze
      if (rotationDirection) {
        if (pauzeMenuItem < 1) { 
          pauzeMenuItem++;
        }
        sequenceStep = 3;
        lcd.setCursor(0, 1);
        lcd.print("                ");
        lcd.setCursor(0, 1);
        lcd.print(sequenceStepText[6]);
      } else {
        if (pauzeMenuItem > -1) { 
          pauzeMenuItem--;
        }
      }
      switch (pauzeMenuItem) { 
        case 1:
          direction = true;
          sequenceStep = 3;
          lcd.setCursor(0, 1);
          lcd.print("                ");
          lcd.setCursor(0, 1);
          lcd.print(sequenceStepText[6]);
          break;
        case 0:
          direction = false;
          sequenceStep = 3;
          lcd.setCursor(0, 1);
          lcd.print("                ");
          lcd.setCursor(0, 1);
          lcd.print(sequenceStepText[7]);
          break;
        case -1:
          sequenceStep = 8;
          lcd.setCursor(0, 1);
          lcd.print("                ");
          lcd.setCursor(0, 1);
          lcd.print(sequenceStepText[8]);
          break;
      }
  }
}

void windup(){
  while(turns > 0){
    countTurns();
  }
  motor.stop();
  sequenceStep = 9;
  lcd.clear();
  lcd.setCursor(0,0); 
  lcd.print(sequenceStepText[sequenceStep]);
}

void countTurns() {
	motor.setSpeed(speed);
	if (direction) {
		motor.forward();
	} else {
		motor.backward();
	}
	boolean readVal;
	while(true) {
    readVal = digitalRead(IN_D6);
    if (!readVal && state){
      if (direction) {
        turns--;
      } else {
        turns++;
      }
      state = false;
      lcd.setCursor(0,1); 
      lcd.print("      ");
      lcd.setCursor(0,1); 
      lcd.print(turns);
    }
    if (readVal != 0) {
      state = true;
      break;
      
    }
  }
}

[rsmls]

@fanofdan

a While loop runs until the number of turns is reached, the interrupts of the rotary-encoder are not triggered.

They're probably triggered alright, but since the microcontroller is busy on that while loop, what happens with this information? After all, you only process rotary data in the loop and you only get back there after the while loop breaks.

[StefanL38]

if void loop has only this single function-call
How should this work at all?

@fanofdan

void loop() {
  Rotary.Process( millis() );
}

I have never used this rotary-library
If you are trying to execute all your functions through an interrupt this messes up everything

[DaveX]

I've only glanced at your code, but it looks like you are doing an awful lot within the interrupt contexts, including windup(). Inside of an interrupt, none of the prints(), timing, or other button-press functions can advance.

Instead of thinking of using an interrupt to trigger a whole routine, think of using the interrupt to set a flag, and then, back in user/loop() space use the flag to trigger the actions. It likely will require a complete re-structuring of your code.

It isn't so much that interrupts are ignores within a While loop, as interrupts are ignored during processing of interrupts.

[StefanL38]

After looking a bit closer to your code.
This rotary-library uses call-back-functions.

Here I have a question of my own:
If the library uses call-back-functions, if the callback-function is beeing called is this still "inside" the interrupt?
Regarding the fact that as long as an interrupt-service-routine is executed no other interrupts can occur

[gfvalvo]

@StefanL38

If the library uses call-back-functions, if the callback-function is beeing called is this still "inside" the interrupt?

The source code is here: https://github.com/dmachard/KY040-rotary/tree/master
You'll see that the callback functions are dispatched from the Process() method.

[DaveX]

I'm wrong in #3. The library sets flags within the interrupts, and then acts on those flags in ...Process(). Sorry.

It isn't your issue, but the library's internal debouncing scheme looks...confusing? millis()%1000 phase dependent?

dmachard/KY040-rotary/blob/8a9a04665de64e6eb96ba2e8ea45415c0d82ce46/KY040rotary.cpp#L83-L94

[dth1971]

Interrupts are get in and get out as soon as possible. Your main loop comes to a stop while the interrupt is running. Also with some interrupts, some functions will not update like millis()

[J-M-L]

@fanofdan

I use the KY040rotary library

I don't know that one - may be it's not a great library...

I use the encoder library

Also on this:

@fanofdan

const int pinCLK=2;    // CLOCK signal
const int pinSW=3;   // Rotary Encoder Switch
const int pinDT=4;    // DATA signal

you should have CLK and DT on pin 2 and 3 if you have a UNO so that you can benefit from the interrupts on those pins. Connect the switch on another pin like 4 and use one of the numerous button library such as Button in easyRun or OneButton or Toggle or EasyButton or Bounce2, ... to handle that switch

[rsmls]

@StefanL38

if the callback-function is beeing called is this still "inside" the interrupt?

No.

As I said in #2, the problem is that the callbacks are never run as long as the controller is in the while() of the windup() function. Interrupts are likely triggering fine, the ISR likely runs fine just as well - it's just that nothing is ever done with that information.

@J-M-L

may be it's not a great library...

What's "great" - it looks fine to me. We all have our preferences, and every approach has its pros & cons. Having looked at this particular library, it looks entirely acceptable and decent to me. Maybe the structure surrounding the ISR's is a bit awkward, but I can see why they did it like this. Debouncing seems to be handled well, it's not massively resource-hungry...my main qualm is that the code lacks any form of comments, but it's still straightforward to follow.

In any case, the library as such is not the problem. It's really how it's being used here. Once you enter a while() loop and there's only one way to get out of it (i.e. let it run its course), evidently nothing is ever being done with any user input. Regardless of what library one would use, this problem will continue to exist.

The solution is to modify the while() loop that counts the turns, and process rotary input there as well, and act on that input once it arrives. There are other ways as well, of course, but this would be the last invasive to the present structure of the code.

[StefanL38]

As long as you are using this while-loop the only way to become reactive again would be to insert the code that shall be executed additionally inside this while-loop.

The name is well chosen "while...... loop. (and stay inside this loop

You have structured your code which is good. Though the way you have structured it here would require to add the react_on_rotating_code in two while-loops

void windup() {
  while (turns > 0) {
    // code_to_react_on_rotating
    countTurns();
  }

// and inside
void countTurns() {
  motor.setSpeed(speed);
  if (direction) {
    motor.forward();
  } else {
    motor.backward();
  }
  boolean readVal;
  while (true) {
    // code_to_react_on_rotating
    readVal = digitalRead(IN_D6);
    if (!readVal && state) {
//...

If you don't want to add this additional code inside the while-loop your code must be changed to remove this inner while-loops and using a more outside "loop" for the looping. Which is void loop() itself.

The basic principle is to have an if-condition in void loop() that makes the execution of the while-loop-code conditional

void loop() {
  Rotary.Process( millis() );
  // no longer while (turns > 0) { but instead
  // because void loop() is looping anyway
  if (turns > 0) {
    countTurns();
  }
}

Inside countturns() you have another while-loop that is only exited when a break is executed. Well this is one way to make a program functional but a rather akward one.

For the always fast responsiveness your code must be changed to quickly jump in / jump out of all functions to return back to the level of void loop() to make sure that the function-calls to

Rotary.Process( millis() );

are repeated fast and often.
Here a thread with graphics that explain this

Explaining how to write NON-blocking all time responsive code

Hi Everybody, from time to time I enjoy writing tutorials in my kind and style. My intention is to make it easy to understand. Though I'm aware: of the fact that one way of explaining things is not the best way for everybody. that the best experts about easyness-to-understand are: beginners . I want to encourage especially beginners to ask whatever question comes to their mind. I will use these questions as suggestion to improve the tutorial to become even more easy to understand. The u…

The basic principle to use interrupts is:
react immidiately in the same microseconds on signals that are

• only a few microseconds short
• the point in time must be catched very precisely with microseconds resolution
• each and every signal must be catched regardless of what your code is doing (like reading in rotary-encoder-pulses

storing the signal-occurence in some kind of a flag so the rest of the code can be informed "oh! not to forget there was a ....-signal!"
or
storing the exact point in time when the signal occured for precise calculations.
or
counting up/down a variable because the rotary-encoder was rotated

[ricky3321]

After all, you only process rotary data in the loop and you only get back there after the while loop breaks.

[J-M-L]

@rsmls

What's "great" - it looks fine to me. We all have our preferences, and every approach has its pros & cons. Having looked at this particular library, it looks entirely acceptable and decent to me. Maybe the structure surrounding the ISR's is a bit awkward, but I can see why they did it like this. Debouncing seems to be handled well, it's not massively resource-hungry...my main qualm is that the code lacks any form of comments, but it's still straightforward to follow.

I had a look too...

The way the interrupts are managed (going to user code to go back to the library) is indeed awkward

It's not beginner friendly as it's up to the user to ensure the pins you pass are external interrupts ready... (no pinChange)

for example this might be an issue for that if OP is using a UNO or MEGA as pinDT does not support external interrupts.

@fanofdan

// Rotary Encoder Module connections
const int pinCLK=2;    // CLOCK signal
const int pinSW=3;   // Rotary Encoder Switch
const int pinDT=4;    // DATA signal

// init the button
KY040 Rotary(pinCLK, pinDT, pinSW);

My main "complaint" though would be that it does not maintain some sort of counter for the encoder and so the ISR might be triggered multiple times before you come back to the main loop() where you call the Rotary.Process() function ➜ this leads to missing ticks.

(and there are a few more things that bother me like taking a modulo 1000 which is costly, it would have been wiser to use 1024 and just mask to keep the first 10 bits or just go for a uint16_t for the timing...)

[DaveX]

@J-M-L

things that bother me like taking a modulo 1000 which is costly, it would have been wiser to use 1024 and just mask to keep the first 10 bits or just go for a uint16_t for the timing...

I wasn't sure about storing 'uint32_t's in this->swLastTime as an 'int32_t' and the interactions with %1000, and then I gave up.

dmachard/KY040-rotary/blob/8a9a04665de64e6eb96ba2e8ea45415c0d82ce46/KY040rotary.h#L43-L43

[rsmls]

@J-M-L

My main "complaint" though would be that it does not maintain some sort of counter for the encoder and so the ISR might be triggered multiple times before you come back to the main loop() where you call the Rotary.Process() function ➜ this leads to missing ticks.

Yeah, that's annoying; I also noticed that. It's clearly made with the intention that this is avoided. I never do my rotary routines this way and simply have CW and CCW update the same counter so I know what the net number of motions is that was counted.