laurb9 / StepperDriver

Arduino library for A4988, DRV8825, DRV8834, DRV8880 and generic two-pin (DIR/STEP) stepper motor drivers
MIT License
551 stars 230 forks source link

DRV8825 is using wrong ms_table[] #7

Closed ZuljinSBK closed 8 years ago

ZuljinSBK commented 8 years ago

It is using ms_table[] that is defined in A4988.cpp. It is using this table, because implementation of method setMicrostep() is in A4988.cpp and this table is static. If I copy whole setMicrostep() method to DRV8825.cpp and that way overwrite A4988 method it start to work. There is few method to resolve this issue like removing static and moving initialization of ms_table to constructor or overwriting setMicrostep() in DRV8825.

Additionally I found few smaller cosmetic issues :) 1) In few places there is this kind of if statment if (!IS_CONNECTED(ms1_pin) || !IS_CONNECTED(ms1_pin) || !IS_CONNECTED(ms1_pin)) I think it shouldn't have only ms1_pin there. 2) Following members are not used in DRV8825.h: int mode0_pin = PIN_UNCONNECTED; int mode1_pin = PIN_UNCONNECTED; int mode2_pin = PIN_UNCONNECTED;

laurb9 commented 8 years ago

You're right. I'll have to brush up on my C++ to find an elegant solution to this, but it looks like wrapping the table in a getter method might be the way to do it.

hosadoya commented 8 years ago

I have locally fixed this issue, but can't push to a remote git repo for Pull Request with this error: Permission to laurb9/StepperDriver.git denied to hosadoya. Attached is the fix files, can you please check-it in? src.zip

laurb9 commented 8 years ago

Thanks, but I decided a shorter change would be to have a _setMicrostep(microsteps, table, size) in A4988, it looks like it should work but haven't tested it yet.

Btw, the change workflow is like this: fork the repo, make changes, push a branch into your repo then create a PR from there to this one. You do want to make sure you don't do reformatting, since otherwise a lot more changes are shown than actually there.

laurb9 commented 8 years ago

Hmm. While this did fix the problem, there's a deeper change that needs to be made, as max_microstep isn't respected in any subclass.

laurb9 commented 8 years ago

I pushed another branch with a modified version of @hosadoya's changes which addresses the max_microstep issue, but is quite verbose.