1

The past few days I've had trouble with a FOR loop iteration within an IF statement.

I have a 12V DC geared motor with magnetic encoder being driven by an L298N motor driver which is controlled by an Arduino. Most of the code works, being that if button 2 is pressed, the motor rotates clockwise; if button 32 is pressed, the motor rotates counter-clockwise.

However, if button 1 is pressed, I want to motor to rotate clockwise to a specified limit (for which position is reported to the Arudino from encoder), then counter clockwise to another specified limit, and continually repeat this back-and-forth sequence. This FOR loop iteration will correctly work if it (the back-and-forth sequence) alone is placed inside the void loop, but if it is nested within the IF statement, it correctly rotates clockwise but then incorrectly rotates counter-clockwise continuously (never returning to the clockwise direction).

The code is below, any help is appreciated and my apologies for the wordy explanation.

#define enA 9
#define in1 6
#define in2 7

const int button1Pin = 8;
const int button2Pin = 12;
const int button3Pin = 13;

int button1State = 0;
int button2State = 0;
int button3State = 0;

int rotDirection = 0;

volatile long temp, counter = 0;

void setup() {
  Serial.begin (9600);
  pinMode(2, INPUT_PULLUP);
  pinMode(3, INPUT_PULLUP);

  attachInterrupt(0, ai0, RISING);
  attachInterrupt(1, ai1, RISING);

  pinMode(enA, OUTPUT);
  pinMode(in1, OUTPUT);
  pinMode(in2, OUTPUT);

  digitalWrite(in1, LOW);
  digitalWrite(in2, LOW);

  pinMode(button1Pin, INPUT);
  pinMode(button2Pin, INPUT);
  pinMode(button3Pin, INPUT);
}

void loop() {
  analogWrite(enA, 255);

  if( counter != temp ){
    Serial.println (counter);
    temp = counter;
  }

  button1State = digitalRead(button1Pin);
  button2State = digitalRead(button2Pin);
  button3State = digitalRead(button3Pin);

  if (button1State == HIGH) {
    for (counter = 0; counter < 2000; counter++) {
      clockwise();
    }
    for (counter = 2000; counter > 0; counter--) {
      counterclockwise();
    }
  }
  if (button2State == HIGH) {
    clockwise();
  }
  if (button3State == HIGH) {
    counterclockwise();
  }
}

void ai0() {
  if (digitalRead(3) == LOW) {
    counter++;
  } else {
    counter--;
  }
}

void ai1() {
  if (digitalRead(2) == LOW) {
    counter--;
  } else {
    counter++;
  }
}

void clockwise () {
  digitalWrite(in1, HIGH);
  digitalWrite(in2, LOW);
  rotDirection = 1;
  delay(20);
}

void counterclockwise () {
  digitalWrite(in1, LOW);
  digitalWrite(in2, HIGH);
  rotDirection = 0;
  delay(20);
}

The simpler code (without any other button inputs) that allowed for back-and-forth motion to continually toggle in both directions can be seen here:

#define enA 9
#define in1 6
#define in2 7

const int button1Pin = 8;     
const int button2Pin = 12;
const int button3Pin = 13;

int button1State = 0;     
int button2State = 0;  
int button3State = 0;  

int rotDirection = 0;


volatile long temp, counter = 0; 

void setup() {
  
  Serial.begin (9600);
  pinMode(2, INPUT_PULLUP); 
  pinMode(3, INPUT_PULLUP); 
  attachInterrupt(0, ai0, RISING);
  attachInterrupt(1, ai1, RISING);
  
  pinMode(enA, OUTPUT);
  pinMode(in1, OUTPUT);
  pinMode(in2, OUTPUT);
  
  digitalWrite(in1, LOW);
  digitalWrite(in2, LOW);
  
  pinMode(button1Pin, INPUT);
  pinMode(button2Pin, INPUT);
  pinMode(button3Pin, INPUT);

}

void loop() {

  analogWrite(enA, 255);

  if( counter != temp ){
  Serial.println (counter);
  temp = counter;
  }

  button1State = digitalRead(button1Pin);
  button2State = digitalRead(button2Pin);
  button3State = digitalRead(button3Pin);

   for (counter =0; counter<2000; counter++) {
    digitalWrite(in1, HIGH);
    digitalWrite(in2, LOW);
    rotDirection = 1;   
    delay(20);
  }


  
  // If button is pressed - change rotation direction
  for (counter= 2000; counter>0; counter--) {
    digitalWrite(in1, LOW);
    digitalWrite(in2, HIGH);
    rotDirection = 0;
    delay(20);
  }
 
}

 void ai0() {
  if(digitalRead(3)==LOW) {
  counter++;}
  
  else{
  counter--;}
  }
   
  void ai1() {
  if(digitalRead(2)==LOW) {
  counter--;}
  
  else{
  counter++;}
  }

New Void Loop:

 if( counter != temp ){

Serial.println (counter); temp = counter; } long local_counter = counter;

  attachInterrupt(digitalPinToInterrupt(0), ai0, RISING); 
  attachInterrupt(digitalPinToInterrupt(1), ai1, RISING); 

  while(local_counter <2000){
  analogWrite(enA, 255);
  clockwise();
  
  detachInterrupt(digitalPinToInterrupt(0));
  detachInterrupt(digitalPinToInterrupt(1));
  
  local_counter = counter;
  
  attachInterrupt(digitalPinToInterrupt(0), ai0, RISING); 
  attachInterrupt(digitalPinToInterrupt(1), ai1, RISING);

}

6
  • Why are you in-/decrementing the counter variable in the for loop, when you are already doing this inside the ISRs? That does not make sense. Also by setting the variable there, you will loose some steps, that the motor might have done over the 2000 position
    – chrisl
    Aug 29 '20 at 21:58
  • And you don't need 2 interrupts to read the encoder. One is enough. One input gets the interrupt, the other defines the direction of the tick
    – chrisl
    Aug 29 '20 at 22:21
  • When I place the the FOR iteration alone in the void loop (without any other commands or requested button inputs) , it worked properly (or at least to my liking) with decrementing the counter. With that being the case, would my current problem not be with how the FOR command is nested within my IF statement? Aug 30 '20 at 15:03
  • Did you place both for loops without the rest? Or just the one direction. Difficult to say, what exactly your code will do. Possible, that the problem is only with the one for loop in ccv direction. Please also provide the complete code for your working test case (where it actually worked). Otherwise I can only point to the problems, that I see in your current code
    – chrisl
    Aug 30 '20 at 15:14
  • Sorry, I added the code with the just the FOR command in the VOID loop to my original post, but not couldn't do it in the correct format. However, the simpler code allowed for correct back and forth rotation. Aug 30 '20 at 16:39
0

Note, that this is not a complete answer, but just helping with the obvious problems inside your code. These might not cause your current problem, but you will stumble upon them very soon, if you don't correct them now.

  • The variable counter as position: You obviously intend the variable counter to be your current position. In that case you must not change it without a proper reason. You are using a for loop to move to your position. The motor spins and the Arduino gets the encoder pulses through the interrupts. And that is exactly the place, where the position variable has to be changed. You are additionally incrementing or decrementing it in the for loop. That does not make sense, since the execution time of the loop does not have anything to do with the actual position. You can solve this by simply removing the in-/decrementing and the first setting of the variable in the for loop. This

      for(counter = 0; counter < 2000; counter++)
    

    becomes

      for(; counter < 2000; )
    

    Though this now is not so much a for loop, but a while loop. So you can instead just write

      while(counter < 2000)
    

    For the CCV rotation correspondingly.

  • Using variables, that get changed in an ISR: You need to be careful with reading a variable, that gets changed in an ISR. The ISR can happen at any time in the execution of your main code. The Arduino Uno needs multiple commands to handle a multi-byte variable type, such as long, which has 4 bytes. So the following could easily happen: The code starts to read the counter variable to check, if it is lower than 2000, and starts with the first byte, then the second. But directly after the second byte was processed, the ISR kicks in, because a pulse happened. The ISR then changes the counter variable, that means, that the third and forth byte can now be different. The main code will read these bytes with their new values. So now you have 2 bytes from the old value and 2 bytes of the new value. That can mean, that your data might be garbled and you cannot be sure, if that is anything like correct.

    For handling multi-byte variables with ISRs, you need to enclose the corresponding part of the main code in a critical section. In a critical section, interrupts are disabled. Of course you want these sections to be as short as possible. So mostly you are just copying the value from the variable in question into a local variable and then use that in the further calculations. So that would look somewhat like this:

      noInterrupts();
      long local_counter = counter;
      interrupts();
      while(local_counter < 2000){
          clockwise();
          noInterrupts();
          local_counter = counter;
          interrupts();
      }
    
  • Reading the rotary encoder: A normal rotary encoder has two pins, which on rotation will give you a signal like this Link to wikipedia image

Two square waves in quadrature.

One pulse corresponds to one tick. If you use 2 interrupts, each for one line, you are reacting twice for the same pulse. That means you get twice the number of ticks. You really only need the interrupt for one pin; the other then just determines the direction of the rotation.


Besides these points, you can try to print the current value of counter inside the for/while loop to the Serial Monitor. Then you can see, what happens inside that loop.

1
  • Your explanation helps and is very appreciated. I'm starting over with the basics and rewrote my void loop with a while loop; however, the motor continually revolves, not stopping at 2000. I will post the new void loop in my original post. Sep 2 '20 at 1:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.