Conflict between motor library and external ISR

Projects General Guidance
1

Hello everyone, has anyone used the FastAccelStepper motor library before? I've been using this library for some project testing, but I've found that it conflicts with external interrupts. Let me briefly explain the situation. I'm using PIN2 and PIN3 as limit switches for two directions, implemented as external interrupts. When my ISR (Interrupt Service Routine) is triggered, it means the switch is closed. Buttons 18 and 19 control the movement in these two directions; as long as I keep one of them pressed, the motor keeps moving, and it stops when I release the button. Ideally, when an ISR is triggered, the motor should also be forcibly stopped, which is the expected and correct
behavior.

However, in reality, when the ISR is occasionally triggered, the motor sometimes continues to move uncontrollably forward and doesn't stop even when released. This happens at a frequency of about 1/80. It seems like a conflict between the external interrupt and the library, and I've ruled out any issues with my hardware. Inside my external ISR, I have the statement stepper->forceStopAndNewPosition(0);. The author of the library clearly states that this statement can be used within an interrupt, with the original sentence being: " // abruptly stop the running stepper without deceleration.
// This can be called from an interrupt !."

#include "FastAccelStepper.h"
#define dirPinStepper 5
#define stepPinStepper 7 
volatile byte direction = 1;  //1 forward， 0 backward
volatile byte fLimiter = 0;
volatile byte bLimiter = 0;
volatile long backwardCounter = 0;
volatile long forwardCounter = 0;
FastAccelStepperEngine engine = FastAccelStepperEngine();
FastAccelStepper *stepper = NULL;
void setup() {
  Serial.begin(9600);
  Serial.println("Begin");
  pinMode(3, INPUT);
  pinMode(2, INPUT);
  pinMode(18, INPUT);
  pinMode(19, INPUT);
  attachInterrupt(digitalPinToInterrupt(2), stop1, FALLING);
  attachInterrupt(digitalPinToInterrupt(3), stop2, FALLING);
  engine.init();
  stepper = engine.stepperConnectToPin(stepPinStepper);
  if (stepper) {
    stepper->setDirectionPin(dirPinStepper);
    stepper->setSpeedInHz(8000); 
    stepper->setAcceleration(3000);  
  }
}
void loop() {
    if ((digitalRead(18) == LOW) && (bLimiter == 0))
    {
      direction = 0;
      if (direction == 0) {
        backwardCounter++;
        Serial.println(backwardCounter);
      }
      if (backwardCounter == 500) {
        backwardCounter = 0;
        fLimiter = 0;
      }
      stepper->runForward();
    } 
    else if ((digitalRead(19) == LOW) && (fLimiter == 0))
    {
      direction = 1;
      if (direction == 1) {
        forwardCounter++;
        Serial.println(forwardCounter);
      }
      if (forwardCounter == 300) {
        forwardCounter = 0;
        bLimiter = 0;
      }
      stepper->runBackward();
    } else {
      stepper->forceStopAndNewPosition(0);
      backwardCounter = 0;
      forwardCounter = 0;
    }
}
void stop1() {
  if (direction == 0) {  //lmiter
    stepper->forceStopAndNewPosition(0);
    bLimiter = 1;
    forwardCounter = 0;
    direction = 1;
  }
}
void stop2() {  
  if (direction == 1) { //lmiter2
    stepper->forceStopAndNewPosition(0);
    fLimiter = 1;
    backwardCounter = 0;
    direction = 0;
  }
}
2

If your code is written correctly, there is no need to use interrupts with the limit switches. Use other pins instead, and digitalRead() to read them, (or direct port access, which is actually faster than an interrupt) and creates many fewer problems with other code.

2 Likes
4

Hello, it's nice to see you again. Last time I also asked some questions, and you gave me a lot of help. Thank you very much. To get back to the point, using ISR in some emergency situations is a safe way, and this is just a simplified situation for me. Do you think polling is faster and more effective in handling urgent matters than ISR? I don't quite think so. But I think your situation may occur when the code volume is very small.

5

Any variable that is used in both an ISR and the main program needs to be declared as volatile.

1 Like
6

I do, as do lots of folks on the forum. Your MCU can do 1000s of things per second which is plenty fast enough to detect a limit switch and stop movement given a properly designed sketch that doesn't have any blocking code. As @jim-p points out, you also didn't declare your variable volatile and you didn't make local copies when using them in your main code - both issues that go away when you forget about ISRs and just poll the limit switches each time through loop.

2 Likes
7

Yes. This instruction to read digital pin 2 on an Uno R3 takes 1 machine cycle, or 62.5 nanoseconds.

byte pin_value = PIND&(1<<2);

And, as pointed out by @blh64, with the typical mistake of forgetting to set the shared variable volatile, there is no guarantee that main program will see the value changed by the ISR. This is hardly a good idea for an emergency stop or limit switch.

2 Likes
8

@arduinoer_cc
Have you made the necessary changes I suggested in your code?

1 Like
9

Thank you very much for your reply. I just saw your suggestion and I think it is very useful. I am currently making code changes and will conduct testing after the changes are completed. I will inform you of the test results. Thank you again

10

Thank you very much. I will follow your advice and add volatile before my variables. In fact, I did use a non ISR version to implement the content of this code in a loop manner, which can indeed solve the problem as you said. However, I am simulating an emergency situation, and I think you are right. It is indeed fast enough in a loop, but I think ISR is very necessary in very urgent situations. What do you think?

11

Thank you very much. I will first modify and test the volatile keyword, and then inform you of the results. Thank you again.

12

@jim-p @blh64 @jremington
After following your advice, I added the keyword 'volatile', but the error still occurs with a probability of around 1/80. However, I am still very grateful to you, and adding this keyword is very necessary. At present, it seems that this issue is a conflict between ISR and the motor library. The external interrupt priority of Mega 2560 is very high, and this library uses timer interrupts. However, the author also proposed “stepper ->forceStopAndNewPosition (0); ” It can be used in ISR, otherwise I wouldn't have used it this way. This statement is for emergency stop, and I have tried to make the ISR statement as short as possible

13

Still not convinced. What a human may perceive as "urgent" can be thought of as painfully slow in terms of an MCU. We operate on different time scales. Do you think your car does most of its work with ISR since you are travelling down the road at a potentially lethal speed? [hint: it does not. the CAN bus sends messages all around the vehicle and certain, dedicated controllers do take care of critical tasks, but also participate on the CAN bus and other things].

If your application is more "urgent" than that, then maybe you need an ISR, but I am still not convinced.

15

Thank you very much for your reply. What you said is very reasonable. But in some special cases, such as when my teacher or client requires me to use ISR to achieve my situation, that's another situation. I have to use ISR to simulate this very urgent situation. What do you think is the problem with my code? Is it a conflict between my external interrupt and the motor library?

16

HI, @arduinoer_cc
Have you tried to use polling method, before trying ISR?

If not, then it might be worth a try.

What is your overall project and if the ISR are for Emergency Stop then that is not the way to do it.

The end stops should not be connected to ANY software application, get the endstops to keep a relay ON to supply motor current, an endstop tripped will cause the relay to turn off the motor current.

Tom.. :smiley: :+1: :coffee: :australia:

17

That is a completely different reason. Something you should have mentioned a few posts ago :slight_smile:

So now we are required to use an ISR and make it work.

First off, the variables you use in the ISR and the main program must be declared as volatile so the compiler does not optimize their access incorrectly.

Second, for an 8 bit MCU (which board are you using?), you can not simply access those volatile variables that are larger than 8 bits in the main program since that access can not be done atomically. You will need to turn off interrupts, make a local copy, turn interrupts back on, and then proceed to use the copy. If you do not, there is a chance that the variable will be corrupted.

Apply those learnings to your code. Test it out. Report back with both the new code and the results and we can proceed.

18

Explain... simulate..

Tom.. :smiley: :+1: :coffee: :australia:

19

hi， Thank you very much for your reply. I have tried using polling to implement it in my other version, and it was not a problem. There were no bugs like using ISR But my current situation is that I have to use ISR to complete it. But thank you very much for your advice.

20

Thank you again for your reply. I added volatile before my variable, but the error still occurred. I am using mega 2560. As for the second point you mentioned, I will try it now and let you know the result later

21 
  pinMode(3, INPUT);
  pinMode(2, INPUT);
  pinMode(18, INPUT);
  pinMode(19, INPUT);

INPUT_PULLUP maybe ?

23

Hi,Thank you for your reply. The hardware issue has been ruled out. I have connected a resistor to the external circuit.