Need help with attachinterrupt, it affects other functions under void loop

Hello everyone, any help would do thanks in advance...

So I am working on a project that uses encoded motors.
It is actually a winch that should be remote controlled. So, the winch have three state or function... that is to drop, hoist and stop. Drop makes the winch rotate as to dropdown an item, hoist is to pull up an item and Stop is its idle state.

The main function is this:
1.) The motor has to rotate for a specific amount of rotation, then it will stop.
2.) The motor has to rotate in reverse after the first function then it will stop.
3.) The motor has to stop whenever transmitter is set to "Stop".

Take note that I worked on separate windows then add the code from time to time so that I don't have to change the whole thing whenever I mess up on the codes (I don't know if it has effect on anything please let me know).

Now, I started the code with the basic motor control like forward, stop and reverse, it works fine.

Then I added remote control functions, it works fine too. Responded to Drop, Stop and Hoist accordingly.

Lastly I added the encoder count function, where it starts to mess things around...

When I set the transmitter to "Drop", I was expecting that it should rotate continuously until it reaches the required rotation. But it only rotates and stops periodically by itself until it reaches the required amount of rotation.

When I set the transmitter to "Hoist" it never wants to stop even though I set the transmitter to "Stop"

I doubled, tripled check just in case there are typo or misspelled or missing characters i don't find any because the compiler would tell me if I have one. But I think it has to do with the way the code was written.

I have tried taking away functions one by one, it always leads me back to the encoder function. When I try to disable one of the "attachInterrupt (digitalPinToInterrupt (encoder_PinA), ISR_EncoderA, CHANGE);"...removing the encoder count function... the code works fine when I try to enable it back, problem comes back too.

Here's the code:

[code]
//-------Receiver Pin-------//
unsigned long ch4 = 7;
//-------ENCODER PIN-----
volatile long encoder_Pos = 0;
#define encoder_PinA 2 //encoder pin assignment
#define encoder_PinB 3

//-------MOTOR DRIVER PIN---------
int enA = 10; //Purple
int in1 = 11; //Gray
int in2 = 12; //White

void setup() {
  Serial.begin (9600);
  //-------MOTOR PARAMETERS-----
  digitalWrite (enA, HIGH);
  pinMode(enA, OUTPUT);
  pinMode(in1, OUTPUT);
  pinMode(in2, OUTPUT);
  //-------ENCODER PARAMETERS-------
  pinMode (encoder_PinA, INPUT_PULLUP);
  pinMode (encoder_PinB, INPUT_PULLUP);
  attachInterrupt (digitalPinToInterrupt (encoder_PinA), ISR_EncoderA, CHANGE);
  attachInterrupt (digitalPinToInterrupt (encoder_PinB), ISR_EncoderB, CHANGE);

}
void loop() {

  Receiver ();

  //------STOOP FUNCTION-------
  if (ch4 <= 1550 && ch4 >= 1300) {
    Stop();
    delay(100);
  }

  //------DROP FUNCTION-------
  if (ch4 >= 1800) {
    if (encoder_Pos > 0) {
      Drop ();
      Serial.println (encoder_Pos);
    }
    else {
      Stop ();
    }
    delay(100);
  }

  //------HOIST FUNCTION-------
  if (ch4 <= 1100) {
    if (encoder_Pos < 20000) {
      Hoist ();
      Serial.println (encoder_Pos);
    }
    else {
      Stop ();
    }
    delay(100);
  }
}

//--------TRANSMITTER FUNCTION--------
void Receiver() {
  while (digitalRead(7));
  ch4 = pulseIn (7, HIGH, 21000);

  //Serial.println (ch4);
  delay(100);
}
//---------MOTOR FUNCTION-------------
void Stop() {
  digitalWrite (in1, LOW);
  digitalWrite (in2, LOW);
  Serial.println ("Stop");
  delay (1000);
}
void Drop() {
  digitalWrite (in1, LOW);
  digitalWrite (in2, HIGH);
  Serial.println ("Drop");
  delay (1000);
}
void Hoist() {
  digitalWrite (in1, HIGH);
  digitalWrite (in2, LOW);
  Serial.println ("Hoist");
  delay (1000);

}

//---------ENCODER FUNCTION-------------
void ISR_EncoderA () {
  if (digitalRead (encoder_PinA) == HIGH) {
    if (digitalRead(encoder_PinB) == LOW) {
      encoder_Pos = encoder_Pos + 1; //CW
    } else {
      encoder_Pos = encoder_Pos - 1; //CCW
    }
  }
  else {
    if (digitalRead (encoder_PinB) == HIGH) {
      encoder_Pos = encoder_Pos + 1; //CW
    } else {
      encoder_Pos = encoder_Pos - 1; //CCW
    }
  }
}

void ISR_EncoderB () {
  if (digitalRead (encoder_PinB) == HIGH) {
    if (digitalRead(encoder_PinA) == HIGH) {
      encoder_Pos = encoder_Pos + 1; //CW
    } else {
      encoder_Pos = encoder_Pos - 1; //CCW
    }
  } else {
    if (digitalRead (encoder_PinA) == LOW) {
      encoder_Pos = encoder_Pos + 1; //CW
    } else {
      encoder_Pos = encoder_Pos - 1; //CCW
    }
  }
}
[/code]

any help is highly appreciated

Whenever you use encoder_pos outside of your interrupt routines you need to disable interrupts long enough to either use it or take a copy.

1 Like

Why do users choose to ignore the advice on posting code ?

The easier you make it to read and copy your code the more likely it is that you will get help

Please follow the advice given in the link below when posting code , use code tags and post the code here to make it easier to read and copy for examination

1 Like

Hi @wildbill thanks for your reply,
How am I going to disable interrupts long enough? which side of the code do i need to remove or what do I need to add to disable interrupts and where? sorry i'm new to interrupts

You can use noInterrupts() and Interrupts(). A common strategy is to use noInterrupts() to turn them off, make a copy of the variable to use in your if statements and immediately use Interrupts() to turn them back on.

@UKHeliBob I have edited the post. Thank you for the correction.

I'm going to add this at the bottom:

[code]
int Pos(){
  noInterrupts();
 unsigned long Pos = encoder_Pos;
  interrupts ();

}

[/code]

Am I doing it right?

Well you're turning Interrupts off and on ok, but I would expect the compiler to have something to say about your function having no return value.

Also, having Pos as the name of the function and the local variable seems questionable.

Even when the variable name issue is solved, it's local, so it's of little value - you could make it global instead.

I don't Know how to make it global (currently reading article for how to), but for now I tried adding the noInterrupts () and Interrupst() inside the void loop. now it doesn't respond or do anything.

[code]
void loop() {
  noInterrupts();
  unsigned long Pos_en = encoder_Pos;
  interrupts ();
  Receiver ();

  //------STOOP FUNCTION-------
  if (ch4 > 1200 && ch4 < 1700) {
    Stop();
    //Serial.println ("Stop");
    Serial.println (Pos_en);
  }
  //------DROP FUNCTION-------
  if (ch4 >= 1800) {
    Drop();
    Serial.println (Pos_en);
  }



  //------HOIST FUNCTION-------
  if (ch4 <= 1100) {
    Hoist();
    //Serial.println ("Hoist");
    Serial.println (Pos_en);
  }
}

[/code]

also I dont see the reason why the compiler doesn't say anything about the return value earlier

You can change the level of compiler warnings in IDE preferences.

1 Like

I dont know how to make it global but ive tried this:

[code]
int Pos (){
  noInterrupts();
  Pos_en ;
  interrupts ();
  return Pos_en; 
}

[/code]

and tried to insert it inside void loop like this:

[code]
void loop() {
  Pos();
  Receiver ();

  //------STOOP FUNCTION-------
  if (ch4 > 1200 && ch4 < 1700) {
    Stop();
    //Serial.println ("Stop");
    Serial.println (Pos_en);
  }
  //------DROP FUNCTION-------
  if (ch4 >= 1800) {
    Drop();
    Serial.println (Pos_en);
  }



  //------HOIST FUNCTION-------
  if (ch4 <= 1100) {
    Hoist();
    //Serial.println ("Hoist");
    Serial.println (Pos_en);
  }
}

[/code]

I can see from the serial monitor that it changes from Hoist Stop and Drop, but the motor doesn't move

Because you defined Pos_en local to the loop() function, other functions won't see it.

While the compiler doesn't care, it's good practice to start variable names in lower case. (camelCase is preferred). Upper case usually indicates a constant. This way when someone is analyzing your code, they won't have to stop to think, "is this a variable or constant?"

Hi @SteveMann Thank you for your insight, I have changed the Pos_en to posEn. now I have created this:

[code]
int Pos () {
  noInterrupts();
    posEn ;
  interrupts ();

  return posEn;
}

[/code]

let me know if I'm doing it right, Next is I inserted the Pos to void loop like this,

[code]
void loop() {
  Pos();
  Receiver ();

  //------STOOP FUNCTION-------
  if (ch4 > 1200 && ch4 < 1700) {
    Stop();
    //Serial.println ("Stop");
    Serial.println (posEn);
  }
  //------DROP FUNCTION-------
  if (ch4 >= 1800) {
    Drop();
    Serial.println (posEn);
  }



  //------HOIST FUNCTION-------
  if (ch4 <= 1100) {
    Hoist();
    //Serial.println ("Hoist");
    Serial.println (posEn);
  }
}


//--------TRANSMITTER FUNCTION--------
void Receiver() {
  while (digitalRead(7));
  ch4 = pulseIn (7, HIGH, 21000);

  //Serial.println (ch4);
  delay(100);
}

[/code]

From the serial monitor it changes from Hoist, Drop, and Stop accordingly, but the only problem is the motor doesn't move

What are you trying to do with:
posEn ;
?

I am surprised that the compiler doesn't complain.

I am also confused with your use of the word "function" in your comments. When I see "function", I am looking for

functionName(){
  //function code
}

If statements are not functions.

Again, the compiler doesn't care what you put into comments- they are all ignored. But someone analyzing you code may be confused by the comment that describes an if statement as a function. I know this sounds anal, but learning common terminology and coding practices will serve you well in the future.

I would change your comments to:

//------STOOP ?-------
//------DROP ?-------

Stoop?

If your logic works, then the motor is a separate issue. I don't see where you have described the motor or how you are driving it. For this we need the specs of the motor and a schematic. (Please, not a Fritzing picture which are generally useless).

Thank you for your suggestions, really learning a lot. Without the whole attatch interrupt, the code for the motor works perfectly fine... it starts to mess up when I added the attatchinterupt to the code

Here's the current code I've been working on...

[code]

//-------Receiver Pin-------//
unsigned long ch4 = 7;
//-------ENCODER PIN-----
volatile long encoder_Pos = 0;
#define encoder_PinA 2 //encoder pin assignment
#define encoder_PinB 3
unsigned long posEn = encoder_Pos;
//-------MOTOR DRIVER PIN---------
int enA = 10;  //Purple
int in1 = 11;   //Gray
int in2 = 12;  //White



void setup() {
  Serial.begin (9600);
  //-------MOTOR PARAMETERS-----
  digitalWrite (enA, HIGH);
  pinMode(enA, OUTPUT);
  pinMode(in1, OUTPUT);
  pinMode(in2, OUTPUT);
  //-------ENCODER PARAMETERS-------
  pinMode (encoder_PinA, INPUT_PULLUP);
  pinMode (encoder_PinB, INPUT_PULLUP);
  attachInterrupt (digitalPinToInterrupt (encoder_PinA), ISR_EncoderA, CHANGE);
  attachInterrupt (digitalPinToInterrupt (encoder_PinB), ISR_EncoderB, CHANGE);

}

int Pos () {
  noInterrupts();
    posEn ;
  interrupts ();

  return posEn;
}
void loop() {
  Pos();
  Receiver ();

  //------STOP-------
  if (ch4 > 1200 && ch4 < 1700) {
    Stop();
    //Serial.println ("Stop");
    Serial.println (posEn);
  }
  //------DROP-------
  if (ch4 >= 1800) {
    Drop();
    Serial.println (posEn);
  }



  //------HOIST-------
  if (ch4 <= 1100) {
    Hoist();
    //Serial.println ("Hoist");
    Serial.println (posEn);
  }
}


//--------TRANSMITTER--------
void Receiver() {
  while (digitalRead(7));
  ch4 = pulseIn (7, HIGH, 21000);

  //Serial.println (ch4);
  delay(100);
}
//---------MOTOR-------------
void Stop() {
  digitalWrite (in1, LOW);
  digitalWrite (in2, LOW);
  Serial.println ("Stop");
  delay (1000);
}
void Drop() {
  digitalWrite (in1, LOW);
  digitalWrite (in2, HIGH);
  Serial.println ("Drop");
  delay (1000);
}
void Hoist() {
  digitalWrite (in1, HIGH);
  digitalWrite (in2, LOW);
  Serial.println ("Hoist");
  delay (1000);

}


//---------ENCODER-------------
void ISR_EncoderA () {
  if (digitalRead (encoder_PinA) == HIGH) {
    if (digitalRead(encoder_PinB) == LOW) {
      encoder_Pos = encoder_Pos + 1; //CW
    } else {
      encoder_Pos = encoder_Pos - 1; //CCW
    }
  }
  else {
    if (digitalRead (encoder_PinB) == HIGH) {
      encoder_Pos = encoder_Pos + 1; //CW
    } else {
      encoder_Pos = encoder_Pos - 1; //CCW
    }
  }
}

void ISR_EncoderB () {
  if (digitalRead (encoder_PinB) == HIGH) {
    if (digitalRead(encoder_PinA) == HIGH) {
      encoder_Pos = encoder_Pos + 1; //CW
    } else {
      encoder_Pos = encoder_Pos - 1; //CCW
    }
  } else {
    if (digitalRead (encoder_PinA) == LOW) {
      encoder_Pos = encoder_Pos + 1; //CW
    } else {
      encoder_Pos = encoder_Pos - 1; //CCW
    }
  }
}



[/code]

I dont know if I get it right but from what I understood from @wildbill... I have to make a copy of the variable to use in the if statements...

You do indeed need to make a copy and earlier, you had code that did. Your latest version however just mentions posen, it doesn't set it to anything.

ah you mean this?

I decleared it on top most, should I include it inside Pos(); ?

Yes, but drop the unsigned long bit.

1 Like