search

reven / Unistep2

A non-blocking Arduino library for controlling 28BYJ-48 stepper motors
33 stars 8 forks source link

A few improvements #2

Closed Gutza closed 5 years ago

Gutza commented 6 years ago

Hey Robert, I made a few changes to the library, I hope you'll find them useful.

reven commented 6 years ago

Hi @Gutza,

Thanks for taking the time to look over the code. I'm still going through your edits, as I don't have a lot of time at the moment, but there are a few changes that are interesting.

I like the format of the if statements (line 58 src/Unistep2.cpp) and some other formatting you've done. The formatting of the switch(phase) logic is ok, though there are a lot of extra spaces.

I don't understand why you've changed the logic of the if statement on line 40? This negates the whole flow as it was intended.

Also, there is no reason to use long instead of int. That was intentional to minimize memory usage; the way ints overflow still allows for consistent logic (yay modular arithmetics!).

Please correct me if I'm wrong; still kind of new to C.

Gutza commented 6 years ago

Hey @reven,

I'll start at the bottom. I switched to long because the 28BYJ-48 motors take roughly 4000 steps per revolution; with ints, you overflow on commands and status responses on less than 10 revolutions. In my project I need to send move orders beyond 10 revolutions (40k+ steps), and it kept overflowing. In principle, when using a limited number of variables (as is the case for this project), switching from int to long is no biggie – had you been using large arrays or dynamically allocated memory that couldn't been an issue, but here it doesn't really make a difference (try building the original version vs. the modified version, and see how much more memory the new version is using).

Regarding the changes around line 58, take a look at the original source code vs. the new source code (not the diff, but look at them side by side) – you'll notice the new version looks way cleaner, as you don't have to indent as deep; you might want to look up "early return" online for the philosophical principle behind that decision.

Best, Bogdan

reven commented 5 years ago

Hi @Gutza,

Sorry, I really haven't had time to look into this. I was going over the code now now and I've seen a few changes that i really do not agree with. I've added a couple of comments to the specific things.

I'm closing this PR as I think the changes you've implemented go against the design goal for the library. The stylistic changes are actually good, and I wouldn't mind implementing them, but you've changed the logic of a few critical conditionals that were designed that way to make them as quick as possible.

Thanks!