Rotary encoder, stepper motor and timers

Hi!
Im having trouble getting a delayed function using timer(). What am I doing wrong?

unsigned long  timer1;
int timer2=2000;
unsigned long timer3;
 if (RotaryPosition >= 15 && RotaryPosition <= 20) {
      
      timer1=millis();
    }
    
      timer3=millis()-timer1;
    
      if (timer3 >= timer2) {
        Gir1led=1;
      }

I have three LEDs and want them to light up one by one when I have turned the stepper to the right Rotaryposition. The first LED to light up when i pause the stepper in between the 15 and 20 value. The second LED in the 32 to 37 value and the last LED between 5 and 10. I managed to get it to work but now its enough to turn the encoder a full turn to turn on all the LEDs. So Im trying to place a timer() function so I actually have to stop at the right values for i to lit the LEDs.
I have never worked with interupts before so maybe its the interupt that is troublesome? Since the reset button dont reset the GirLEDs when I dont press the button and turn the encoder and the same time.

#include "Stepper.h"
#define STEPS  32   // Number of steps for one revolution of Internal shaft
                    // 2048 steps for one revolution of External shaft
                  
 
volatile boolean TurnDetected;  // need volatile for Interrupts
volatile boolean rotationdirection;  // CW or CCW rotation
 
const int PinCLK=2;   // Generating interrupts using CLK signal
const int PinDT=3;    // Reading DT signal
const int PinSW=4; // Reading Push Button switch
const int resetpin=12;
const int led1 =5;
const int led2 = 6;
const int led3 = 7;


unsigned long  timer1;
int timer2=2000;
unsigned long timer3;

int RotaryPosition=0;    // To store Stepper Motor Position
int Gir1led = 0;
int Gir2led = 0;
int Gir3led = 0;
int reset = 0;

int PrevPosition;     // Previous Rotary position Value to check accuracy
int StepsToTake;      // How much to move Stepper
 
// Setup of proper sequencing for Motor Driver Pins
// In1, In2, In3, In4 in the sequence 1-3-2-4
Stepper small_stepper(STEPS, 8, 10, 9, 11);
 
// Interrupt routine runs if CLK goes from HIGH to LOW
void isr ()  {
  delay(4);  // delay for Debouncing
  if (digitalRead(PinCLK))
    rotationdirection= digitalRead(PinDT);
    
  else
    rotationdirection= !digitalRead(PinDT);
  TurnDetected = true;
}
 
void setup ()  {
  Serial.begin(9600);
  //pinMode(2, INPUT);
  pinMode(led1, OUTPUT);
  pinMode(led2, OUTPUT);
  pinMode(resetpin, INPUT);
pinMode(PinCLK,INPUT);
pinMode(PinDT,INPUT);  
pinMode(PinSW,INPUT);
digitalWrite(PinSW, HIGH); // Pull-Up resistor for switch
attachInterrupt (0,isr,FALLING); // interrupt 0 always connected to pin 2 on Arduino UNO
}
 
void loop ()  {
    reset = digitalRead(resetpin);
  if (reset == LOW) {
    Gir1led = 0;
    Gir2led = 0;
    Gir3led = 0;
  }
  
  small_stepper.setSpeed(300); //Max seems to be 700
  if (!(digitalRead(PinSW))) {   // check if button is pressed
    if (RotaryPosition == 0) {  // check if button was already pressed
    } else {
        small_stepper.step(-(RotaryPosition*30));
        RotaryPosition=0; // Reset position to ZERO
      }
    }
 
  // Runs if rotation was detected
  if (TurnDetected)  {
    PrevPosition = RotaryPosition; // Save previous position in variable
    if (rotationdirection) {
      RotaryPosition=RotaryPosition-1;} // decrase Position by 1
    else {
      RotaryPosition=RotaryPosition+1;} // increase Position by 1
 
    TurnDetected = false;  // do NOT repeat IF loop until new rotation detected
 
    // Which direction to move Stepper motor
    if ((PrevPosition + 1) == RotaryPosition) { // Move motor CW
      StepsToTake=50; 
      small_stepper.step(StepsToTake);
    }
 
    if ((RotaryPosition + 1) == PrevPosition) { // Move motor CCW
      StepsToTake=-50;
      small_stepper.step(StepsToTake);
     
    }
    Serial.println(RotaryPosition);
    Serial.println(timer3);
    Serial.println(timer1);
    
    if (RotaryPosition == 41) {  
      RotaryPosition = 0;           //A total of 41steps on the stepper for a full turn
    
    }
    if (RotaryPosition == -1) {
      RotaryPosition = 40;
    
    }
    
    if (RotaryPosition >= 15 && RotaryPosition <= 20) {
      
      timer1=millis();
    }
    
      timer3=millis()-timer1;
    
      if (timer3 >= timer2) {
        Gir1led=1;
      }
     
           if (RotaryPosition >= 32 && RotaryPosition <= 37 && Gir1led == 1) {
             
              Gir2led = 1;
                   
            }

            if (RotaryPosition >= 5 && RotaryPosition <= 10 && Gir1led == 1 && Gir2led == 1) {
             
              Gir3led = 1;
                   
            }
               
      
    if (Gir1led >=1) {
      digitalWrite(led1, HIGH);
    }
    else {
      digitalWrite(led1, LOW);
    
    }


    if (Gir2led ==1) {
      digitalWrite(led2, HIGH);
    }
    else {
      digitalWrite(led2, LOW);
    }

   
    if (Gir3led >=1) {
      digitalWrite(led3, HIGH);
    }
    else {
      digitalWrite(led3, LOW);
    
    }
    
  }
}

Writing your own encoder interrupts can be tricky. Check out this great encoder library by PJRC.
If you take a look at the implementation, you'll understand that there's more to it than just digitalReading the pins.
Writing your own implementation may be good for learning purposes, if you want to use it in a project, I recommend using the library.

void isr ()  {
  delay(4);  // delay for Debouncing

Delays inside interrupts are not usually a good idea. While your little demo won't show it, 4ms wasted inside an interrupt will seriously impact the performance of the rest of the code.

Encoders often don't need debouncing as the bounces just flip back and forth between two adjacent positions.

unsigned long  timer1;
int timer2=2000;
unsigned long timer3;

Any time you have numbers in your variable names you are doing something wrong. Usually the reason is because you should have used an array. In this case, it is just a poor choice of names. It would be much better to give them names which correspond to their function. For example: currentTime, delayPeriod, and elapsedTime. Then it will be much easier to use those variables in your code and you don't need to keep scrolling up and down to find which one is which.

Any time you have numbers in your variable names you are doing something wrong.

So I changed names and removed the numbers. But no change. It seems that this line:

if (RotaryPosition <=15 && RotaryPosition >=20) {
      
      currenttime = millis();
    }

Never activates even though the RotaryPosition is below 15 and above 20 and therefore "currenttime" is always 0. Why? When I replace the currenttime = millis(); with Gir1led=1; it works and the LED is lit.
Am I really this stupid that I cant understand such simple coding?

How can it be both less than 15 and greater than 20 at the same time?

Oh, f*ck me... Thank you for helping an old blind man. Now it works... :slight_smile:

Samfag:
Oh, f*ck me... Thank you for helping an old blind man. Now it works... :slight_smile:

Welcome to the club :slight_smile:

...R