Support generic drivers for Stepper library

[skniazev]

Main features of this version:

1. 100% compatible with the actual master branch
2. generic driver support (see example stepper_oneRevolution_ULN2003 for
   driver ULN2003ERP & Motor_BYJ48)
3. constructors harmonization by introducing the initMotor method,
4. speed and size optimization of method stepMotor by introducing the
   phasesMatrix array

[matthijskooijman]

Thanks for creating this PR. The code looks good, but I do have some remarks:

• As discussed in the other issue, I think using PROGMEM would be a good idea.

• You make changes to the step() function, which are not explained in the commit message AFAICS. Also, I think your changes subtly change the behaviour of the function (by always delaying the full step delay), and I do not really see why they are needed?

• I don't think the generic constructor's arguments are really easy to use, having to pass zeroes for unused pins. Why not just add versions of the current constructors, but with the added phases matrix arguments? E.g., you'd have:

  void Stepper::initMotor(int number_of_steps, int motor_pin_1, int motor_pin_2, int motor_pin_3, int motor_pin_4, int motor_pin_5, unsigned char *phasesMatrix, int phase_count)
  void Stepper::initMotor(int number_of_steps, int motor_pin_1, int motor_pin_2, int motor_pin_3, int motor_pin_4, unsigned char *phasesMatrix, int phase_count)
  void Stepper::initMotor(int number_of_steps, int motor_pin_1, int motor_pin_2, unsigned char *phasesMatrix, int phase_count)
  

  The compiler will be able to tell from the type of the arguments what constructor to use. If we keep the current generic constructor form, I would move the pin count a bit further to the front, to keep it closer to the pin numbers, and to keep the phases matrix and phase count close together. So either:

  void Stepper::initMotor(int number_of_steps, int motor_pin_1, int motor_pin_2, int motor_pin_3, int motor_pin_4, int motor_pin_5, int pin_count, unsigned char *phasesMatrix, int phase_count)
  

  or

  void Stepper::initMotor(int number_of_steps, int pin_count, int motor_pin_1, int motor_pin_2, int motor_pin_3, int motor_pin_4, int motor_pin_5, unsigned char *phasesMatrix, int phase_count)
  

• In initMotor(), you assign the motor_pin array one by one in a loop, using a switch to select the right argument. The advantage of that approach is that you only assign the pin numbers that are actually used, but I think that it also results in fairly complex code (both source and compiled binary), which I think isn't even faster than the simpler alternative:

  this->motor_pin[0] = motor_pin_1;
  this->motor_pin[1] = motor_pin_2;
  this->motor_pin[2] = motor_pin_3;
  this->motor_pin[3] = motor_pin_4;
  this->motor_pin[4] = motor_pin_5;
  

• Perhaps it would make sense to renumber the pin number argument names, e.g. start at motor_pin_0, to prevent confusing between the argument names and the array positions?

• I think it might be clearer to right-align the patterns instead of left-align. E.g., looking at the first line from the 4-phase pattern, that looks like 0b10100000. Here, the first 4 bits are meaningful, whereas the last 4 bits are zero padding. This makes it harder to read this table. By right-aligning, this line would look like 0b00001010, or even simpler 0b1010, which makes it a lot more clear. I think the only changes for this would be that stepMotor() will look like:

  unsigned char running_one = 1;
  for (int i = pin_count; i > 0; i--, running_one <<= 1) {
          digitalWrite(motor_pin[i - 1], (phasesMatrix[thisStep] & running_one));
  }
  

  (Note that the loop is a bit more complicated than you'd expect, since I'm avoiding running_one = (1 << pin_count);, which is slow on AVR boards)

[richardwillars]

+1 for this - should fix a few issues!

[skniazev]

Hi Matthijs!

As discussed in the other issue, I think using PROGMEM would be a good idea.

fixed (see commit "PROGMEM approach" skniazev@c046a3f)

Please also consider the commit "unsigned char approach" (skniazev@9f55a75)

You make changes to the step() function, which are not explained in the commit message AFAICS. Also, I think your changes subtly change the behaviour of the function (by always delaying the full step delay), and I do not really see why they are needed?

There are no functional changes in this method, only optimization. The delay was too complex implemented with the following construction:

    unsigned long now = micros();
    // move only if the appropriate delay has passed:
    if (now - this->last_step_time >= this->step_delay)
    {
      // get the timeStamp of when you stepped:
      this->last_step_time = now;

    //...

    }

this was not very clever for many reasons. I believe it is evidently.

I don't think the generic constructor's arguments are really easy to use, having to pass zeroes for unused pins. Why not just add versions of the current constructors, but with the added phases matrix arguments?

You are right, this can be optimized. It was fist quick&dirty approach to reach the point. I'll consider your proposals to find the best solution. I'll fix it in the next commit.

In initMotor(), you assign the motor_pin array one by one in a loop, using a switch to select the right argument. The advantage of that approach is that you only assign the pin numbers that are actually used, but I think that it also results in fairly complex code (both source and compiled binary), which I think isn't even faster than the simpler alternative:

this->motor_pin[0] = motor_pin_1;
this->motor_pin[1] = motor_pin_2;
this->motor_pin[2] = motor_pin_3;
this->motor_pin[3] = motor_pin_4;
this->motor_pin[4] = motor_pin_5;

You are right, but we need a loop for pinMode(this->motor_pin[i], OUTPUT);:

  this->motor_pin[0] = motor_pin_1;
  this->motor_pin[1] = motor_pin_2;
  this->motor_pin[2] = motor_pin_3;
  this->motor_pin[3] = motor_pin_4;
  this->motor_pin[4] = motor_pin_5;
  for (unsigned char i = 0; i < this->pin_count; i++){
    pinMode(this->motor_pin[i], OUTPUT);  // setup the pins on the microcontroller
  }

I'll fix it in the next commit.

Perhaps it would make sense to renumber the pin number argument names, e.g. start at motor_pin_0, to prevent confusing between the argument names and the array positions?

You are right. It was also my idea, but initially I wanted to hold the changes on the minimal level. I'll fix it in the next commit.

I think it might be clearer to right-align the patterns instead of left-align.

You are right. It was also my idea, but initially I wanted to reflect patterns, which were documented in the source code:

/*
 * The sequence of control signals for 5 phase, 5 control wires is as follows:
 *
 * Step C0 C1 C2 C3 C4
 *    1  0  1  1  0  1
 *    2  0  1  0  0  1
 *    3  0  1  0  1  1
 *    4  0  1  0  1  0
 *    5  1  1  0  1  0
 *    6  1  0  0  1  0
 *    7  1  0  1  1  0
 *    8  1  0  1  0  0
 *    9  1  0  1  0  1
 *   10  0  0  1  0  1
 *
 * The sequence of control signals for 4 control wires is as follows:
 *
 * Step C0 C1 C2 C3
 *    1  1  0  1  0
 *    2  0  1  1  0
 *    3  0  1  0  1
 *    4  1  0  0  1
 *
 * The sequence of controls signals for 2 control wires is as follows
 * (columns C1 and C2 from above):
 *
 * Step C0 C1
 *    1  0  1
 *    2  1  1
 *    3  1  0
 *    4  0  0
 *
 */

To optimize the code for the right-aligned patterns, the patterns must be mirrored. In this case it would be reasonable to change also the documentation in the source code. But initially I wanted to hold the changes on the minimal level. I'll fix it in the next commit.

I think that is meaningful to make one commit per one improvement. Have you any priority?

[skniazev]

Hi Matthijs!

please consider my commit for preventing of dividing by zero in method setSpeed() (skniazev@a6e3b3c)

You are right, but we need a loop for pinMode(this->motor_pin[i], OUTPUT);:

  this->motor_pin[0] = motor_pin_1;
  this->motor_pin[1] = motor_pin_2;
  this->motor_pin[2] = motor_pin_3;
  this->motor_pin[3] = motor_pin_4;
  this->motor_pin[4] = motor_pin_5;
  for (unsigned char i = 0; i < this->pin_count; i++){
    pinMode(this->motor_pin[i], OUTPUT);  // setup the pins on the microcontroller
  }

fixed, consider the commit "Optimization of method initMotor()" (skniazev@a9278c5)

[matthijskooijman]

There are no functional changes in this method, only optimization. The delay was too complex implemented with the following construction:

I actually think that the original delay implementation is better than your version. Consider a stepper motor with a max speed of 1000 steps/s (step time is 1ms). Now consider that the main loop calls step(1) about every millisecond. With the original code, the motor would turn at full speed. With your code, the motor will turn at half speed, since for every step there is 1ms of delayMilliseconds() and (around) 1ms of delay in the loop while it does other things.

You are right, but we need a loop for pinMode(this->motor_pin[i], OUTPUT);

Ah, missed that. What you propose looks best to me.

To optimize the code for the right-aligned patterns, the patterns must be mirrored. In this case it would be reasonable to change also the documentation in the source code.

I do think that mirroring the patterns might create confusing. You are right that mirroring them makes right-aligning optimal, but it is also possible to right-align without mirroring. See the code I proposed in my earlier comment for this. Note that it writes pins from pin_count to 0, but reads the bits from 1 up to 1<<pin_count, effectively doing the mirroring in the code.

I think that is meaningful to make one commit per one improvement. Have you any priority?

Yes, though some of these commits should probably be squashed into the original commit later (in particular commits that only change an approach in the first commit, without making any relevant changes to the original code, are candidates for squashing).

I looked through your added commits too, they look good to me. I left a few minor remarks inline. Keep them coming :-D

[skniazev]

I actually think that the original delay implementation is better than your version. Consider a stepper motor with a max speed of 1000 steps/s (step time is 1ms). Now consider that the main loop calls step(1) about every millisecond. With the original code, the motor would turn at full speed. With your code, the motor will turn at half speed, since for every step there is 1ms of delayMilliseconds() and (around) 1ms of delay in the loop while it does other things.

Ah ja, multitasking. Fixed in commit skniazev@5b947dc

[skniazev]

• I don't think the generic constructor's arguments are really easy to use, having to pass zeroes for unused pins. Why not just add versions of the current constructors, but with the added phases matrix arguments?

Fixed with commit skniazev@c1442a5

[matthijskooijman]

Except for my last remark, I think you've now responded to all my comments, right? Perhaps this would be the moment to clean up the branch by merging some commits? Do you have a clear idea of what should be merged or otherwise changed (and how to do it - git rebase is your friend)? Or should I have a look and propose something?

[skniazev]

Except for my last remark, I think you've now responded to all my comments, right? 

Yes, I also think so.

Do you have a clear idea of what should be merged or otherwise changed (and how to do it - git rebase is your friend)? 

I have clear idea, what should be merged, but I have no experience in terms of github. git rebase
I also didn't make for a long time.

Or should I have a look and propose something?

I think for the first time it will be good.

[skniazev]

Is it the right way?

1. Update (fetch) my branch from master
2. (Optionally, but I think it is a bad idea) Squash my changes
3. Rebase (can you please provide the right command line for this, I'm not sure)

[matthijskooijman]

I'm mostly suggesting interactive rebase as a tool to rewrite your history and clean up the commits. You can also use regular rebase to bring your branch up to speed with the latest master, but since I think no changes have been made to the Stepper library recently, that's not so important now.

As for the interactive rebase, here's some guides about how to use that tool:

• https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History
• https://www.atlassian.com/git/tutorials/rewriting-history

I'm out of time for now, let me know how far these suggestions get you :-)

[skniazev]

You can also use regular rebase to bring your branch up to speed with the latest master, but since I think no changes have been made to the Stepper library recently, that's not so important now.

I fetched the latest version of master. You are right, where is no conflicts with my branch. So I would prefer just to rebase with all my commits. Should I make it?

[matthijskooijman]

Rebasing onto the current master is probably easiest. Add --interactive to allow editing your commits (and you can rebase again on the same master revision afterwards to just edit more commits, without actually rebasing onto a different base).

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}