Proportional control error - Someone help

Projects Programming
1

my code is working correctly but the thing is, my encoder code is somehow wrong and because of that i am getting only possitive count values for both Clock wise & anti clock wise direction. Someone please correctify it -

code :

int PWM = 10;
#define in1 6
#define in2 7
#define outA 2
#define outB 3
signed volatile long counter ;
// signed volatile long old ;
float u;
float kp = 1;
void setup(){
// counter = old ;
Serial.begin(9600);
pinMode(PWM, OUTPUT);
pinMode(in1, OUTPUT);
pinMode(in2, OUTPUT);
pinMode(outA, INPUT);
pinMode(outB, INPUT);

pinMode(outA, INPUT_PULLUP);
pinMode(outB, INPUT_PULLUP);
attachInterrupt( digitalPinToInterrupt(2) , count, RISING);

}
float reference = 1000;
float error;

bool CW(){
analogWrite(PWM, u);
digitalWrite(in1, HIGH);
digitalWrite(in2, LOW);

}
bool CCW(){
analogWrite(PWM, u);
digitalWrite(in2, HIGH);
digitalWrite(in1, LOW);

}
void PID(){
error = reference - counter;
u = kp*error ;
}

void count(){
if(digitalRead(outB) == digitalRead(outA) ) {
counter = counter + 1;
}
else {
counter = counter - 1;
}
// noInterrupts();
}

void loop(){

PID();

if(u > 255 ){
u =255;
CW();

}

else if(u > 0 ){
u = u;
CW();

}

else if (u < -255){
u = -255;
CCW();

}

else if( u < 0){
u= -u;
CCW();

}
Serial.print(reference);
Serial.print(" ");
Serial.print(counter);
Serial.print(" ");
Serial.print(" ");
Serial.println(u);
// old =counter ;

}

2

Phoenix_26:
my code is working correctly but the thing is, my encoder code is somehow wrong and because of that i am getting only possitive count values for both Clock wise & anti clock wise direction.

Your code can't be working "correctly" then if your encoder code is "somehow wrong" .

Print out values to help you understand what is going on with your NOT-working code.

3

I am trying to rotate the two channel encoder DC motor to a specific position and I am able to do that with my code but the problem is it's printing the counter value wrong at the end. My motor is rotating clockwise direction first and the anti clockwise to come back to the reference point. But after the clockwise rotation when it's turning back to anti clockwise it is not deducting the position value instead it's adding the counter value even in anti clock direction. Please check my code and help me to correct it.

Code :

int PWM = 10;
#define in1 6
#define in2 7
#define outA 2
#define outB 3
volatile signed long counter;
volatile signed long old =0 ;
long reference = 2000;
float u;
float kp = 1;
void setup(){

Serial.begin(9600);
pinMode(PWM, OUTPUT);
pinMode(in1, OUTPUT);
pinMode(in2, OUTPUT);
pinMode(outA, INPUT);
pinMode(outB, INPUT);

pinMode(outA, INPUT_PULLUP);
pinMode(outB, INPUT_PULLUP);
attachInterrupt( digitalPinToInterrupt(2) , count, RISING);

}

float error;

void CW(){
analogWrite(PWM, u);
digitalWrite(in1, HIGH);
digitalWrite(in2, LOW);

}
void CCW(){
analogWrite(PWM, u);
digitalWrite(in2, HIGH);
digitalWrite(in1, LOW);

}
void PID(){
error = reference - counter;
u = kp*error ;
}

void count(){
if(digitalRead(outA) == HIGH) {
counter ++;
}
else {
counter --;
}
}

void loop(){

PID();

if(u > 255 ){
u =255;
CW();

}

else if(u > 0 ){
u = u;
CW();

}

else if (u < -255){
u = -255;
CCW();
}
else if( u < 0){
u= -u;
CCW();

}

Serial.print(reference);
Serial.print(" ");

Serial.print(counter);
Serial.print(" ");
Serial.print(u);
Serial.println(" ");
}

Ar.PNG
Ar.PNG1354×731 23.7 KB

PID5.ino (1.4 KB)

4

How do you expect this code ever to find outA LOW

void count(){
       if(digitalRead(outA) == HIGH) {
           counter ++;
          }
       else {
            counter --;
          }
   }

I suspect you should be checking outB

If this line

attachInterrupt( digitalPinToInterrupt(2) , count, RISING);

was

attachInterrupt( digitalPinToInterrupt(outA) , count, RISING);

the code would be clearer

There is little point in giving names to pins and then not using them :slight_smile:

...R

PS please modify your post and use the code button </> so your code looks like this and is easy to copy to a text editor. See How to use the Forum

5

@Phoenix_26, don't DOUBLE POST. I have already answered this in your other Thread

Seems the Moderators got there before me :slight_smile: (Thanks, Moderator).

...R

6

You had it right the first time:

void count(){
       if(digitalRead(outB) == digitalRead(outA) ) { 
            counter = counter + 1;   
          }
       else {
            counter = counter - 1;
          } 
     // noInterrupts();
   }

Since this is called on the rising edge of A it is equivalent to:

void count(){
       if(digitalRead(outB) ) 
            counter++;   
       else 
            counter --;
   }

If that did not work I would suspect a bad connection to 'outB'.

Your other attempt is wrong and would give you the symptom of always increasing counter:

void count(){
       if(digitalRead(outA) == HIGH) { 
           counter ++;
          }
       else {
            counter --; 
          } 
   }

Since this is called on the rising edge of A it is equivalent to:

void count(){
       if(HIGH == HIGH)   // Always true
            counter++;   
       else 
            counter --;  // Never used
   }
7

Thank you so much for the help. It works now perfectly. I am attaching the correct code here so that everyone can get help from here :smiley:

Correct Code :

int PWM = 10;
#define in1 6   //  defining pins
#define in2 7
#define outB  2
#define outA  3
volatile signed long counter;
volatile signed long old =0 ;
long reference = 2000;                       //It's set-point at which the DC motor will stop
float u;
float kp = 1;   
void setup(){
  
 Serial.begin(9600);
 pinMode(PWM, OUTPUT);
 pinMode(in1, OUTPUT);
 pinMode(in2, OUTPUT);
 pinMode(outA, INPUT);
 pinMode(outB, INPUT);
 
   pinMode(outA, INPUT_PULLUP);
   pinMode(outB, INPUT_PULLUP);
 attachInterrupt( digitalPinToInterrupt(outB) , count, RISING);  //It's for taking outputs as interrupt from encoder pin 

}

float error;


void CW(){                        //ClockWise motor rotation
  analogWrite(PWM, u);
 digitalWrite(in1, HIGH);
 digitalWrite(in2, LOW);
 
}
void CCW(){                       //Counter ClockWise motor rotation 
  analogWrite(PWM, u);
 digitalWrite(in2, HIGH);
 digitalWrite(in1, LOW);
 
}
void PID(){
   error = reference - counter;           
   u = kp*error ;
}

void count(){
      if(digitalRead(outA) == LOW) { 
          counter ++;
         }
      else {
           counter --; 
         } 
  }

void loop(){

  PID();
  
    if(u > 255 ){
           u =255;
           CW();
        
       }
                 
    else if(u > 0 ){
           u = u;
            CW();
          
        }
               
    else if (u < -255){
           u = -255;
           CCW();
         }
     else if( u < 0){ 
         u= -u;
         CCW();

       } 
  
 Serial.print(reference); 
 Serial.print("  ");   
 Serial.print(counter); 
 Serial.println("  ");  
}
8

This is an example of how I might have written this same sketch:

// Define pins

const byte MotorPWMPin = 10;
const byte MotorCWPin = 6;
const byte MotorCCWPin =  7;

const byte EncoderAPin = 2;  // Must be an external interrupt pin.
const byte EncoderAInterrupt = 0;  // External interupt for EncoderAPin
const byte EncoderBPin = 3;

// Paramiter for PID loop
const float kP = 1.0;
const float kI = 0.0;  // Not used
const float kD = 0.0;  // Not used

volatile signed long CurrentPosition = 0;

// volatile signed long old = 0 ;
const long ReferencePosition = 2000;          //It's set-point at which the DC motor will stop


void setup() {
  Serial.begin(9600);

  pinMode(MotorPWMPin, OUTPUT);
  pinMode(MotorCWPin, OUTPUT);
  pinMode(MotorCCWPin, OUTPUT);

  pinMode(EncoderAPin, INPUT_PULLUP);
  pinMode(EncoderBPin, INPUT_PULLUP);
  attachInterrupt(EncoderAInterrupt, EncoderARisingISR, RISING);
}

void EncoderARisingISR() {
  if (digitalRead(EncoderBPin)) {
    CurrentPosition++;
  }
  else {
    CurrentPosition--;
  }
}

float PID() {
  long position;
  // For volatile variables larger than 1 byte, grab them with interrupts
  // disabled so the value doesn't change in the middle of fetching.
  noInterrupts();
  position = CurrentPosition;
  interrupts();
  
  long error = ReferencePosition - position;
  float result = kP * error;
  return result;
}

void loop() {
  float Output = PID();
  int speed = constrain ((int)Output, -255, 255);

  // Positive speed goes CW, negative speed goes CCW
  if (speed >= 0) {
    // ClockWise motor rotation
    digitalWrite(MotorCWPin, HIGH);
    digitalWrite(MotorCCWPin, LOW);
    analogWrite(MotorPWMPin, speed);
  }
  else {
    //Counter ClockWise motor rotation
    digitalWrite(MotorCWPin, LOW);
    digitalWrite(MotorCCWPin, HIGH);
    analogWrite(MotorPWMPin, -speed);
  }

  // Show speed and position while not stopped
  if (speed != 0) {
    long position;
    // For volatile variables larger than 1 byte, grab them with interrupts
    // disabled so the value doesn't change in the middle of fetching.
    noInterrupts();
    position = CurrentPosition;
    interrupts();
    
    Serial.print(speed);
    Serial.print("  ");
    Serial.print(position);
    Serial.println("  ");
  }
}

The main thing is using variables and pin numbers with meaningful names. It is confusing to the reader when you name pins after the name of the pin they are connected to: calling outputs 'in1' and 'in2' and inputs 'outA' and 'outB'. Also, single character names are seldom meaningful unless it's a 'for' loop index.

Also, using function arguments and return values rather than cryptic global variables makes the code easier to read and harder to get wrong.

The use of "const" in place of "#define" allows the compiler to produce more meaningful messages when the name is used incorrectly.

I eliminated the CW() and CCW() functions because they were very short and only used once each.