-
Notifications
You must be signed in to change notification settings - Fork 80
Allow move_indefinitely to move in the 'negative' direction #446
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
base: main
Are you sure you want to change the base?
Conversation
Negative velocities would throw a MAXIMUM VELOCITY EXCEEDED error, and move_indefinitely only sent the "MV" command. This meant there was no was to move_indefinitely in the 'negative' direction. Now, when setting velocity, a direction will be set as well, allowing for indefinite movement in either direction using the "MV+" or "MV-" commands. This is the only area that direction currently applies.
Used _direction when changing and returning value to fix recursion error.
Commented out previous 'direction' code and simply added an argument to "move_indefinitely()" to specify direction. This will allow velocity to be adjusted during a move without problems of changing the sign.
Corrected the syntax for checking if direction is '+' or '-'
for more information, see https://pre-commit.ci
|
Looks pretty good, might be nice to add put make the respective unit test a parameterized one to check for positive and negative direction (test here) A more general note and something that is surely outside of this PR but we should keep in mind: |
|
|
||
| @velocity.setter | ||
| def velocity(self, velocity): | ||
| velocity = abs(velocity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why take abs here?
| def move_indefinitely(self, direction: str = "+"): | ||
| """ | ||
| Move until told to stop | ||
| Move until told to stop in the direction ("+" or "-") passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of magic strings as arguments. I would rather see an enum, or just forward/reverse booleans.
This change allows the NewportESP301 to take a direction argument when calling the 'move_indefinitely' function. Currently, this function only allows movement in the 'positive' direction. By adding an optional 'direction' argument, the function can be used in either direction. This update should not affect any existing code, as the default if no argument is passed will still be positive movement.