Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ability to set a stepper's rpm via float #3853

Open
wants to merge 1 commit into
base: master
from

Conversation

@ccoenen
Copy link
Contributor

commented Sep 23, 2015

Stepper can currently only set rotations per minute as integer. This is not a problem for fast motors, as 1200 rpm is not much different from 1200.5 rpm. For slow motors it's a big deal.

Having no way to go slower than 1 rpm or nothing in between 1 rpm and 2 rpm means people have to resort to workarounds like setting a different (wrong) number of steps instead. To illustrate this more clearly: right now, i have to choose if a single revolution takes 60 seconds or 30 seconds or 20 seconds. There is nothing in between.

I added setSpeed(float) as a second signature to not break backwards compatibility.

@cmaglie

This comment has been minimized.

Copy link
Member

commented Sep 24, 2015

Adding this method breaks the old sketches that uses int for setting the speed:

stepper_oneRevolution.ino: In function 'void setup()':
stepper_oneRevolution.ino:28:24: error: call of overloaded 'setSpeed(int)' is ambiguous
stepper_oneRevolution.ino:28:24: note: candidates are:
In file included from stepper_oneRevolution.ino:18:0:
libraries/Stepper/src/Stepper.h:94:10: note: void Stepper::setSpeed(long int)
     void setSpeed(long whatSpeed);
          ^
libraries/Stepper/src/Stepper.h:95:10: note: void Stepper::setSpeed(float)
     void setSpeed(float whatSpeed);
          ^
exit status 1
call of overloaded 'setSpeed(int)' is ambiguous

I don't know what the proper fix can be here. Changing the function name seems odd, also adding 10 different overloads. Maybe method templates can be of help here?

@ccoenen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2015

Awww damn, I thought I had prevented that with the overloaded version. I'll take a closer look.

@ccoenen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2015

Apparently it can't decide which typecast is better. So it's probably better to just offer one thing (I'd opt for the float). I'm preparing another commit, that will replace the previous one.

@ccoenen ccoenen force-pushed the ccoenen:rpm-float-stepper branch from 6a0525a to c927722 Sep 24, 2015

@ccoenen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2015

I think it would also be another improvement to guard this against very small values. But i haven't included it, yet. I would love to discuss this with someone first.

@cmaglie

This comment has been minimized.

Copy link
Member

commented Sep 25, 2015

Using float force the compiler to always link floating-point math subroutines that are huge and slow: someone may want to use only integer math.

My suggestion is to use Method Templates:

http://www.codeproject.com/Articles/257589/An-Idiots-Guide-to-Cplusplus-Templates-Part#MethodTemplates

the setSpeed function may be implemented as:

class Stepper {
  public:
    [...]

    template<typename T>
    void setSpeed(T whatSpeed) {
      step_delay = 60L * 1000L * 1000L / this->number_of_steps / whatSpeed;
    }

(this is just a draft, I've not tried it)

so the setSpeed methods with the correct type are instantiated "on-demand" only when used.

@ccoenen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2015

That's interesting. It will be a while before i can try that. My suggestion is to leave this pull request open, i'll provide another commit in a few days. Would that be alright with you?

Thanks also for the hint regarding float's problems. I was not aware of that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.