Stepper Library bug?

I've been experimenting with the stepper library in the 0011 build, and I seem to have a problem I cannot explain. I'm using 4 phase, unipolar motor that has 20 steps/revolution and, as a test, I wrote code to spin the motor clockwise 1 turn (20 steps), pause, then spin it counterclockwise 1 turn, and so on. However, try as I might, the motor would not return to the same spot. If I changed the step counts to 22 clockwise, then 22 counterclockwise, the motor would do as I expected, but this is a 20 step motor, not a 22 step motor. Finally, I decided to slow down the step rate so I could watch each step and noticed a very curious thing. Each time the motor pasued before reversing direction, the next step after the pause would be one step more in the direction the motor had been turning before it would start moving in the new direction... So, now it was clear why adding 2 extra steps balanced out this error. Can enyone explain this behavior?

Wayne

OK. I’ve looked into this a bit more (once I found where the library code was located) and it appears to me that the stepper library, as coded in the 0011 build, is flawed. However, let me start by showing my test code:

#include <Stepper.h>

Stepper stepper(20, 2, 3, 4, 5);

void setup() {
  stepper.setSpeed(5);
}

void loop() {
    stepper.step(20);
    delay(4000);
    stepper.step(-20);
    delay(4000);
}

To understand the problem in context, here’s the boiled down library source for the “step()” function:

void Stepper::step (int steps_to_move) {  
  int steps_left = abs(steps_to_move);  // how many steps to take
  while(steps_left > 0) {
  if (millis() - last_step_time >= step_delay) {
      stepMotor(step_number % 4);
      last_step_time = millis();
      if (steps_to_move > 0) {
        step_number++;
        if (step_number == number_of_steps) {
          step_number = 0;
        }
      } else { 
        if (step_number == 0) {
          step_number = number_of_steps;
        }
        step_number--;
      }
      steps_left--;
    }
  }
}

I’ve omitted comments (and some other aspects of the code that make it hard to read), but the gist of the problem results from the placement of the call to the stepMotor() function. As you can see, stepMotor() is called at the top of the loop, which means that, when the loop completes, the state of the “step_number” variable will leave it pointing to the next step beyond where the the motor actually stopped. So, he next call to step() will move to the position during the first pass through the loop, then reverse direction and start back in the new direction.

Off hand, it seems to me that this code could never have worked correctly in any application where precise, reversible control of a motor is required. So, I’d be curious to know if anyone else has noticed this problem.

Also, since I’m new to the Arduinio world, I’m not sure exactly how one goes about getting this fixed in the next release so that others don’t get confused the way I did?

Wayne

Thank you for tracking down this bug and figuring out its cause. It's very helpful (both for me as a developer and for other users). If you have time to find a solution, that would be even better. The easiest thing for me is if you post a patch here (or send it via email to me or the developers list. If you don't have time, maybe some else does; otherwise I can take a shot at it when I have a chance. Help testing once we have a patch is also great.

I would humbly suggest that from a design standpoint, step() has no business touching step_number. step_number really belongs to stepMotor(), and that the parameter to stepMotor is redunant. stepMotor is correctly private, and there doesn't seem to be a need to call it with an argument of any sort since the class already knows the required parameter -- step_number.

I would argue the following:

The motor should know about what step it is at, not whomever is telling the motor to step. The stepper class can maintain the step_number for the motor, but nobody else needs to touch it, generally. This would make it easy and neat for a motor to manage its own step_number.

Even for maybe future fractional stepping, that detail should be hidden from step() anyway, so again, no need for the parameter to the method (but it is need in the class, of course).

Anyway, doing it this way would simplify the code in step() and if coded properly in stepMotor() ensure that the step_number always pointed to the current step, not one that has yet to be requested.

My (perhaps twisted, hopefully not out to lunch) way of seeing it. If I overlooked something hopefully someone will point it out.

:)

wholder sent me a simple patch (moving the actual call to stepMotor() to the end of the step() function), which I've applied and checked in. Thanks. If anyone wants to give it a shot, I'd appreciate it. Just update Stepper.cpp from here: http://svn.berlios.de/viewcvs/arduino/trunk/hardware/libraries/Stepper/ and delete Stepper.o (in the hardware/libraries/Stepper folder).

mewControl: I agree, although I think the patch we've already applied might be good enough. If you want to create a patch to clean things up even more, that'd be great. I'd be happy to make those sorts of improvements.

Cool. I did already come up with some ideas actually, and made note of them. I have never tested this on an Arduino or compiled it for that matter, just notes off top of head. I did throw some comments in below just now to convey intent.

One problem with my approach is that it breaks existing code. Maybe not cool. That could be addressed with overloading, but makes the class messy.

Anyway, here’s another approach. Makes it neat, compact and tidy IMHO.

void Stepper::step(int steps_to_move) 
{
      int steps_left = abs(steps_to_move);  // how many steps to take

        // forward is 1, backwards is -1
        // easier to read version is simple if...else block if desired
      int stepDirection = ((steps_to_move >= 0) ? 1 : -1);

        while (steps_left > 0) 
        {
              if (millis() - last_step_time >= step_delay) 
              {
                      stepMotor(stepDirection);
                          last_step_time = millis();
                          steps_left--;
              }
        }
}

//
// Could make this more fool-proof, but likely not needed because we are private 
// and own it, and read the method documentation. :)
//
// stepMotor is private so no external caller can mess it up we hope
// take the direction parameter as a argument value = -1 for reverse step or 
// argument value = 1 for forward step.  Caller establishes direction.
//
void Stepper::stepMotor(int direction)
{
      //  "advance" the step
      int thisStep = step_number + direction;

      //  0 and number_of_steps are one in the same
      // since steps are in the domain {0..number_of_steps-1}
      if (thisStep < 0)
      {
            // could store this at ctor time rather than calculating it all the time depending on goals
            // this is likely fine
            thisStep = (number_of_steps-1);
      }
      else if (thisStep >= number_of_steps) // probably can make this == but >= is maybe safer?
      {
            thisStep = 0;
      }

    ...
   rest of Stepper::stepMotor goes here unchanged
    ...
}

Does anyone have time to test this? (Or the changes that I've already made?)

I have time but as you may know from my other thread, my sole board is misbehaving. I would be happy to test this once I am back up and running.

Barry

Last night I had noticed the problem that I couldn't precisely position my stepper motor. A quick search showed up this thread.

I just installed the patch and tried it. From my limited testing, it seems to have fixed the problem.

tempalte: which patch?

The one in http://svn.berlios.de/viewcvs/arduino/trunk/hardware/libraries/Stepper/