Support generic drivers for Stepper library

[skniazev]

Hello,

please consider my changes to the Stepper library (see attachments). 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

Thank you in advance, Stanislav Kniazev

Stepper.h.txt Stepper.cpp.txt stepper_oneRevolution_ULN2003.ino.txt

[skniazev]

update of example stepper_oneRevolution_ULN2003 for standard library

stepper_oneRevolution_ULN2003.ino.txt

[matthijskooijman]

At intial glance, this looks like a useful generalization of the Stepper library, and I'm all for applying it. It would be easier to merge and review if you could make a pull request out of it, though. Are you willing to investigate how git and pullrequests work and turn this into a proper pull request? I think this is a good place to start: https://help.github.com/articles/fork-a-repo/

As for the actual code, I think it looks nice and clean. One remark though, it would probably better to require the phase matrix variables to be stored in PROGMEM. Normal variables are stored in Flash/PROGMEM, but copied into RAM at startup. In this case, keeping them in PROGMEM and just loading each byte when you need it will save a bit of previous RAM. http://gammon.com.au/progmem has good introduction about this.

Finally, since it seems you're familiar with stepper motor operations, would you perhaps care to have a look at #4153 and comment if you can figure out what is wrong there (since I do not believe the proposed fix is the right one).

[skniazev]

Take a look at file stepper_oneRevolution_ULN2003.ino.txt The motor, I used, seems to be very similar to the one from issue #4153. My version of the library and the example mentioned above must fix the issue. I have already made a local git repository for the project and a new branch 'skniazev'. If you give me rights to make PRs, I will upload it.

[matthijskooijman]

The motor, I used, seems to be very similar to the one from issue #4153. My version of the library and the example mentioned above must fix the issue.

Does that mean that, in addition to making the driver patterns more flexible, you also changed them? If so, it would be best to separate both changes into 2 separate commits, if you did not already do this.

I have already made a local git repository for the project and a new branch 'skniazev'. If you give me rights to make PRs, I will upload it.

You should make a fork in your github account (by clicking the fork button in the arduino/Arduino repository here), and push your branch to your own fork. Github should then allow you to create a pullrequest from that branch (no special rights are required).

Thanks!

[skniazev]

Does that mean that, in addition to making the driver patterns more flexible, you also changed them? If so, it would be best to separate both changes into 2 separate commits, if you did not already do this.

I didn't make any changes in driver patterns. As I wrote, the version is 100% compatible with the actual master branch. I made an ADDITIONAL generic constructor, that makes possible to set a custom driver pattern. As an example of using this new constructor (and as example how the issue #4153 can be fixed) I wrote an example stepper_oneRevolution_28BYJ48_ULN2003 (see my PR: https://github.com/arduino/Arduino/pull/4257), which was originally forked from the example stepper_oneRevolution. This example adapted for the combination of Motor 28BYJ-48 and driver ULN2003e (see http://www.amazon.de/gp/product/B00ATA5MFE?keywords=ULN2003%2028BYJ48).

As for the actual code, I think it looks nice and clean. One remark though, it would probably better to require the phase matrix variables to be stored in PROGMEM. Normal variables are stored in Flash/PROGMEM, but copied into RAM at startup. In this case, keeping them in PROGMEM and just loading each byte when you need it will save a bit of previous RAM. http://gammon.com.au/progmem has good introduction about this.

I've tried to use PROGMEM. Fist of all, the using of PROGMEM has saved only 8 bytes of dynamic memory. Secondly, after using of PROGMEM for unknown reasons the code doesn't work. That's why I've refused at the moment of using PROGMEM.

You should make a fork in your github account (by clicking the fork button in the arduino/Arduino repository here), and push your branch to your own fork. Github should then allow you to create a pullrequest from that branch (no special rights are required).

Thank you for the advice. As mentioned above, I made a PR: https://github.com/arduino/Arduino/pull/4257 Please consider to apply it.

Thank you in advance!

[matthijskooijman]

Thanks for the pullrequest!

As an example of using this new constructor (and as example how the issue #4153 can be fixed) I wrote an example stepper_oneRevolution_28BYJ48_ULN2003

Ah yes, of course, should have realized that that example passes in a custom driver pattern, my bad :-)

I've tried to use PROGMEM. Fist of all, the using of PROGMEM has saved only 8 bytes of dynamic memory. Secondly, after using of PROGMEM for unknown reasons the code doesn't work. That's why I've refused at the moment of using PROGMEM.

Hm, did you, in addition to adding the PROGMEM keyword, also modify the code that reads from the array (e.g. by calling pgm_read_byte or similar)? If not, that's probably why it didn't work, unfortunately PROGMEM isn't as magical as you'd want it to be. However, since switching from regular memory to PROGMEM later isn't particularly easy, I'd say we should try to get it working now (@cmaglie, agreed?).

[skniazev]

I've made 2 additional commits to the PR https://github.com/arduino/Arduino/pull/4257:

1. First one is PROGMEM solution for the driver Patterns. It saves 28 bytes of the dynamic memory.
2. Second one is further dynamic memory optimization by using 'unsigned char' instead of 'int' in private methods and members where it is reasonable. So the library remains 100% compatible with the actual master branch.

Please consider to apply.

Thank you in advance!

[skniazev]

Hello Matthijs!

I guess that I fulfill all your wishes for changing the code. Please make one more review of the code (https://github.com/arduino/Arduino/pull/4257). Please also inform what still to be done to merge my code with the current version.

[matthijskooijman]

I'll go ahead and close this issue, let's keep all discussion in the PR now. I'll respond there as well, but might take a few days (no time now).