Skip to content

Conversation

@Cbantz
Copy link

@Cbantz Cbantz commented Jul 15, 2025

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.

Cbantz and others added 6 commits July 15, 2025 11:40
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 '-'
@trappitsch
Copy link
Contributor

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:
Direction calls seem to be different from instrument to instrument... sometimes it is "+", "-" (e.g., Newport PMC8742), "fwd", "rev", (e.g., Thorlabs APT), or mixed with direction and speed... (e.g., Newport Agilis). This could probably be standardized by using a direction enum throughout within ik? Well, would however for sure be breaking the current API...


@velocity.setter
def velocity(self, velocity):
velocity = abs(velocity)
Copy link
Contributor

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.
Copy link
Contributor

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.

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 this pull request may close these issues.

3 participants