gin66 / FastAccelStepper

A high speed stepper library for Atmega 168/328p (nano), Atmega32u4, Atmega 2560, ESP32, ESP32S2, ESP32S3, ESP32C3 and Atmel SAM Due
MIT License
286 stars 67 forks source link

Best way to use the function: forceStopAndNewPosition()? #111

Closed JaspervanKol closed 2 years ago

JaspervanKol commented 2 years ago

Hi, what is the best way to use the function: forceStopAndNewPosition()? I want to home my stepper motor but after calling the above function the motor will stop but after that it will not listen to any position control. It will also say that the motor is running after homing with this function: isRunning(). I am using the Arduino Due with this code:

//Libraries
#include "FastAccelStepper.h"

FastAccelStepperEngine engine = FastAccelStepperEngine();

FastAccelStepper *Stepper1 = NULL;

//Functions
void LimitSwitchStepper() 
{
  Stepper1->forceStopAndNewPosition(0);
}

void setup() 
{
  engine.init();

  Stepper1 = engine.stepperConnectToPin(9);

  Stepper1->setDirectionPin(21);
  Stepper1->setEnablePin(27);
  Stepper1->setAutoEnable(false);
  Stepper1->enableOutputs();

  Stepper1->setSpeedInHz(2000);
  Stepper1->setAcceleration(4000);

  pinMode(32, INPUT_PULLUP);

  attachInterrupt(digitalPinToInterrupt(32), LimitSwitchStepper, FALLING);

  Stepper1->runBackward();
}

void loop() 
{   
  if (Stepper1->isRunning() == false)
  {
    Stepper1->moveTo(2000);
    while(1);
  }
}

With this code the stepper motor will home to the limit switch but it will never call: moveTo(2000).

gin66 commented 2 years ago

The code looks OK. Could you please state, which of the supported devices are you using ?

JaspervanKol commented 2 years ago

I am using the TB6600 (TB67S109A) driver with the Arduino Due.

gin66 commented 2 years ago

Sorry, I have missed the reference to Arduino Due. This is SAM based and that code is from @clazarowitz. But I suspect to know the issue.

And while reading the implementation of forceStopAndNewPosition() I have seen a potential issue, that calling from an interrupt at the wrong moment may restart the stepper on an esp32.

gin66 commented 2 years ago

Please try this latest version, if this works. I cannot test it.

@clazarowitz Please review the change

gin66 commented 2 years ago

The latest update hopefully fixes the mentioned potential issue.

Only tested with simavr. So due/esp32/atmega is not tested.

JaspervanKol commented 2 years ago

Thanks for your fast reply, but unfortunately the fix is not working. This is the new code I'm using with a few Serial.print's.

//Libraries
#include "FastAccelStepper.h"

FastAccelStepperEngine engine = FastAccelStepperEngine();

FastAccelStepper *Stepper1 = NULL;

//Functions
void LimitSwitchStepper() 
{
  Stepper1->forceStopAndNewPosition(0);
}

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

  engine.init();

  Stepper1 = engine.stepperConnectToPin(9);

  Stepper1->setDirectionPin(21);
  Stepper1->setEnablePin(27);
  Stepper1->setAutoEnable(false);
  Stepper1->enableOutputs();

  Stepper1->setSpeedInHz(2000);
  Stepper1->setAcceleration(4000);

  pinMode(32, INPUT_PULLUP);

  attachInterrupt(digitalPinToInterrupt(32), LimitSwitchStepper, FALLING);

  Serial.println("Homing");

  Stepper1->runBackward();
}

void loop() 
{   
  if (Stepper1->isRunning() == false)
  {
    Serial.println("Moving");
    Serial.println(Stepper1->getCurrentPosition());
    Stepper1->moveTo(2000);
    Serial.println(Stepper1->getCurrentPosition());
    while(1);
  }
}

The function isRunning() is now returning false after homing but it will still not move the stepper to a new position.

image

clazarowitz commented 2 years ago

Unfortunately, this week is a very busy one for me. I’ll probably spend the weekend recovering, and will try to get to this next week. That or maybe this weekend if I get to where my brain isn’t broken then!

I’m just excited that I’m not the only one using the due code. The bug is probably not with the ISR’s, it’s probably just the setup. I don’t think I ever used the runBackwards function. I did use the moveTo pretty extensively though. (I’m pretty sure).

gin66 commented 2 years ago

@clazarowitz I have added some comments to the due code and in general reworked the library. Please have a look and provide your comments. BTW: I have followed your proposal to use volatile for read_idx and next_write_idx.

gin66 commented 2 years ago

@JaspervanKol: The two println() will be executed immediately after each other because moveTo() does not block.

    Serial.println(Stepper1->getCurrentPosition());
    Stepper1->moveTo(2000);
    Serial.println(Stepper1->getCurrentPosition());

Just to mention that, but I understand the problem is, that the stepper is not moving to position 2000.

While clazarowitz is checking the driver, there may be another root cause, which you please check: The switch may be bouncing (giving more than one signal). That's why it may better to disable the interrupt like this (as example):

void loop() 
{   
  if (Stepper1->isRunning() == false)
  {
    detachInterrupt(digitalPinToInterrupt(32));
    Serial.println("Moving");
    Serial.println(Stepper1->getCurrentPosition());
    Stepper1->moveTo(2000);
    while(Stepper1->isRunning());
    Serial.println(Stepper1->getCurrentPosition());
    while(1);
  }
}
gin66 commented 2 years ago

@JaspervanKol Have poked around a bit more in clazarowitz code. Perhaps now is ok ? Even I doubt it

@clazarowitz Sorry for poking around

clazarowitz commented 2 years ago

Ok, so, I decided that my mind is working well enough from having yesterday off...of course, when one is tired, one doesn't know when one is working at full capacity :) So, maybe I'll do more harm than good! kidding....I slept 13 hours yesterday, and 11 last night/this morning. I should be at least ok!

So, this is more of a stream of consciousness sort of comment in that I pulled the latest, put it in visual studio, and started following the function calls down to my code. I suspect your instinct that the issue was in forceStop is correct. _hasISRActive is one piece of the puzzle. I ended up getting rid of most of the PWMC calls because they either did too much or too little. I didn't scan through to find all uses of them, and look at what's in forceStop! I suspect this leaves the PWM interface in a state where it will not be properly re-enabled by the rest of the code.

As I had said, I was pretty sure it wouldn't be the ISR causing the issue, just the setup code. I suspect I'll have a fix to push in a few hours max.

clazarowitz commented 2 years ago

I keep trying to be short, and failing. So I apologize for the length. First, I didn't test out Jochen's changes beyond the eyeball test. Does it look like its doing the right thing. Yes....no counting of pulses, no long term test.

@JaspervanKol's test code. First, it did nothing for me. More often than not the Interrupt Status Register is full of 1's for most pins on startup. So the second you call attachInterrupt, you will get an interrupt. So on my due, using pin 6 for the step pin, 42 for dir, and 46 for en, I got an interrupt right away. So I used a cheat - moving the attachInterrupt call to before the Serial.begin (so Stepper1 would still be NULL), and added a NULL guard to the ISR. Not the "right way" but it works. That got it moving on startup. Then, as Jochen suggested, debouncing is necessary. Sometimes I could detect a move, other times it happened so fast, it looked like the moveTo was being ignored. I cheated and added a static bool to only allow the ISR to be called once. Last - With the loop function running full speed, the call to moveTo would come while the rotor was still moving. So it would not make it 2000 steps from the stop location. I am using a rather large stepper - nema23 thats probably 5 inches long (13cm), so it has a "high rotor inertia", but nothing attached. Your motor may need more or less delay before the moveTo call. You may also want to move more slowly towards the switch, or you could use the fast code, smash the switch, back off to 2000, then more slowly approach the switch, and back off again once triggered for a nice accurate/repeatable 0. :)

clazarowitz commented 2 years ago

ok, I should have got my scope up here sooner. We have an extra pulse now....and also, sometimes the pulse generator doesn't start at all. I'll try to fix all of it rather than reverting anything...

clazarowitz commented 2 years ago

I pushed an update to deal with the extra pulse issue, and commented things a bit better :)

@JaspervanKol if you could test the code with the suggested changes and let us know how it goes we can move on from here :)

Again, I'm just excited to not be the only one using the due code! In fact, I have a few weeks now that my main project cannot be worked, so I think I may go back out to the mower. Its still cold outside, but spring will be here soon, and I have working limit switches for it, and parts for a better implementation of the limit switches available...I just need to design the PCB. So I think I'll work on that, which will mean more use/testing of this code :)

JaspervanKol commented 2 years ago

Thank you for all the help so far. I'm working on a big project with 5 stepper motors with high speeds. That's why I'm using the Arduino Due as controller. I will try the new code on monday and let you know if it is working.

JaspervanKol commented 2 years ago

Sorry, I missed clicked and closed the issue. Now it is open again.

clazarowitz commented 2 years ago

Be sure to let me know how it goes. I ran tests with 2 steppers. I'm a little worried about the number of interrupts being generated already. I tried to keep the ISR code as small as possible to support more steppers, but I have not tested with more than 2. I'm really curious to know how the Due handles it. On one hand, we're running a max of 100kHz per stepper, so that's 500kHz of interrupts, and even if each interrupt service takes 100 cycles that's 60% of the due's processing power. (I don't know how many cycles the interrupt takes on average, but I suspect much less than 100, but RISC sometimes uses inordinate numbers of cycles because of the reduced instruction set :) ) so we should be able to handle it with time left over for processing. On the other, interrupts have to be handled in a timely manner, so if it's too busy servicing one stepper, you could get extra pulses on another stepper. Be aware of the limitation, and let me know how it goes!

gin66 commented 2 years ago

@clazarowitz Thanks for fixing my mess and make it work again in such short time.

And a comment to the 100kHz: If on samd the interrupts are not nested, then all stepper ISR's must be executed sequential. Assume one stepper running at 100 kHz and needs 10us service time. Then second stepper's ISR may cause issues in these cases: 1.) If pause is requested, there is a delayMicroseconds(7); 2.) if direction is changing, there is a delayMicroseconds(30); 3.) if queue is empty, then there is a delayMicroseconds(10);

for 1+3: perhaps those can be just removed for 2: can be removed, if dir_change_delay_us of setDirectionPin() is applied

With another fix for my esp32 regression, I have pushed out new release 0.25.3

gin66 commented 2 years ago

BTW: Reading your comment:

  //Somewhere there must be a call to nointerrupts, because addinf this solved
   //the no-start issue.
   interrupts();
   _hasISRactive=true;

The manageSteppers() is calling addQueueEntry(), which calls startQueue(). And manageSteppers() is called from TC5_Handler(). Does this handler leave interrupts enabled ? If no, then better to enable interrupts and disable afterwards, just like the avr code.

clazarowitz commented 2 years ago

The interrupt controller is nested, but I'm not certain how it works with multiple pins on the same port, since they call the same ISR. On one hand I can see it intelligently calling interrupts for different pins of the same port, on the other, its far more simple to just nest at the port level. I'll bet its in the docs somewhere :)

1: I think I could rework the code to use the falling edge as opposed to the rising edge to deal with this. I had honestly not expected the ISR to finish in just a few microseconds. If I recall correctly, I set the pulses to 10 microsecond width, and the interrupt can cut of the pulse to where it wasn't triggering my stepper controller which requires 5 microsecond pulses. I don't remember ever characterizing the time the ISR took. I guess I can say it's less than 5 microseconds :) I should probably figure that out though, as simply shifting to the falling edge of the pulse will set the minimum time between pulses, but remove a delay. There are always limitations. 2 can almost certainly be removed.

  1. This one really sucks. I actually commented it rather well, and I do remember it. What I don't remember, and I'd have to look at the data sheet, is if I can switch the interrupt to the "falling edge" the same way as I can the PIO interrupt. I think I remember it being able to interrupt on all 3 counters, but I don't remember. Again, I'll need to characterize the ISR time to see if it's even feasible to switch, but I suspect it is.

Characterizing the ISR time should also give a good idea of max frequency and number of steppers.

For the interrupts...This probably needs a lot more debugging. Here is what I witnessed, maybe it will trigger something for you to point out. The code without the noInterrupts/interrupts call would generate a single pulse, then stop. Repeated resets of the Due could sometimes get things running, and it seemed like if it got to the second pulse, everything would work properly. This would happen on maybe 20% of the times I programmed the Due. Adding the lines, and reprogramming 10 times in a row and the problem did not come up. This seems to imply that startQueue worked, we got 1 pulse, and then went into a delay. On my test program, I think this was 3 delays, wone at 65535, then two at 37xxx (Don't remember those digits). I think that's what I remember seeing. What I know for sure is that the first pulse happened, and since the pulse generator is disconnected, that interrupt was serviced. After that, it seems like the delay interrupts were not serviced. How the noInterrupts/interrupts in startQueue affect this, I'm not yet sure. It could be a weird race condition where the noInterrupts/interrupts calls force the schedule to be one that works. It deserves some more effort. What I will say is I believe I remember from my old testing that on the startup of the ramp, sometimes the queue "empties" and "stops" and has to be "restarted" at the beginning. That scenario fits with what is being seen, but may involve more than the simple manageSteppers->fillQueue->addQueueEntry->startQueue path.

gin66 commented 2 years ago

The change to the single startQueue() has caused a regression in esp32, that only one step was output, the queue was in standstill and isRunning() has returned true. In the former code, the queue was eventually kicked again. If you check the updated code in addQueueEntry(), it looks like this (actually the if/if should be compacted):

    // Advance write pointer
    fasDisableInterrupts();
    if (!ignore_commands) {
      next_write_idx++;
      queue_end = next_queue_end;
    }
    fasEnableInterrupts();

    if (isRunning()) {
      // stepper is already running, so nothing else to do
      return AQE_OK;
    }
    // stepper is not yet running. Shall we start ?
    if (start) {
      startQueue();
    }
    return AQE_OK;

This means:

  1. New command is added to the queue
  2. if isRunning() is false, the queue gets started

Consequently the isRunning-flag must not be false, if any lately added command will not be processed. And it too may not be false, but still process another command. In order to note this, the following comment has been added:

  // a word to isRunning():
  //    if isRunning() is false, then the _QUEUE_ is not running.
  //
  // For esp32 this does NOT mean, that the HW is finished.
  // The timer is still counting down to zero until it stops at 0.
  // But there will be no interrupt to process another command.
  // So the queue requires startQueue() again

Checking the due-code, there is one interesting place:

    /*This case should hopefully be elimiated....Unfortunately, it is not :(*/ \
    /*It is quite rare though.*/                                               \
    if (rp == q->next_write_idx) {                                             \
      /*Double interrupt - bail before we destroy the read index!*/            \
      return;                                                                  \
    }                                                                          \

Reaching this code, then _hasISRactive is true, but the queue is actually empty. So I wonder, if _hasISRactive should be set to false before the return - besides, that the code should not be reached anyway.

For the unexplainable noInterrupts()/interrupts() fix. If only interrupts() is added, does it work ? Moving the interrupts() down in the code, may help to identify the critical code part. Besides it would be interesting to output the status of the interrupt flag on entry to startQueue().

JaspervanKol commented 2 years ago

Sorry for my late response but I'm really busy at the moment with school, work and other stuff. When I have the time to test it I will let you guys know as soon as possible. I will also test the capability of the Arduino Due with 5 stepper motors when I have the time.