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

Should throw error when trying to Stepper.step() when Stepper.direction is undefined #736

Closed
IanLondon opened this issue Apr 17, 2015 · 4 comments · Fixed by #740
Closed

Comments

@IanLondon
Copy link

Right now you can call step() on a Stepper when its direction is undefined, without getting any errors. I spend a few hours trying to figure out why my stepper motor wasn't working, and it was because the direction was undefined. Stepper motors are nontrivial to wire so it would be nice to be alerted when code is broken.

I added a small warning in the Stepper wiki.

Thank you for your work on this awesome module!

@rwaldron
Copy link
Owner

Thanks for the report and for updating the docs.

I wonder if step() should warn the user if no direction is set?

@Resseguie
Copy link
Collaborator

It creates a new Error here, but doesn't look like anything is done with it:
https://github.com/rwaldron/johnny-five/blob/master/lib/stepper.js#L409

Can it just default to either cw or ccw?

Or add a check up front that makes sure one is supplied initially, similar to other opts, like here:
https://github.com/rwaldron/johnny-five/blob/master/lib/stepper.js#L128-L132

But I've got other concerns with the implementation, though maybe I'm just missing something. Where is this.pins first initialized? The error handling checks opts.pins exists, but then right after that, the rest of the constructor uses this.pins. Example:
https://github.com/rwaldron/johnny-five/blob/master/lib/stepper.js#L149

@Resseguie
Copy link
Collaborator

Hmm, I was going to try and dive in on this, but it doesn't look like I've got the right hardware to run and test it. Time to place an order, I suppose!

@Resseguie
Copy link
Collaborator

@hail-seitan Care to have a look at that PR? I don't have hardware, so I just implemented against tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants