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.

Sign up for GitHub

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

Jump to bottom

Fix for issue 1184 #1185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix for issue 1184 #1185

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions ophyd/positioner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import functools
import logging
import time
from abc import ABCMeta, abstractmethod
from collections import OrderedDict
from typing import Any, Callable

Expand All @@ -13,18 +14,11 @@
logger = logging.getLogger(__name__)


class PositionerBase(OphydObject):
class PositionerBase(OphydObject, metaclass=ABCMeta):
"""The positioner base class

Subclass from this to implement your own positioners.

.. note ::

Subclasses should add an additional 'wait' keyword argument on
the move method. The `MoveStatus` object returned from
`PositionerBase` can then be waited on after the subclass
finishes the motion configuration.

"""

SUB_START = "start_moving"
Expand Down Expand Up @@ -136,9 +130,10 @@ def report(self):
return rep

@property
@abstractmethod
def egu(self):
"""The engineering units (EGU) for positions"""
raise NotImplementedError("Subclass must implement egu")
pass

@property
def limits(self):
Expand All @@ -152,7 +147,8 @@ def low_limit(self):
def high_limit(self):
return self.limits[1]

def move(self, position, moved_cb=None, timeout=None):
@abstractmethod
def move(self, position, wait=False, moved_cb=None, timeout=None):
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 sure what to do about the argument order here.

Adding wait as the second positional arguement matches the order of EpicmMotor.move and PseudoPositioner.move but doing so is a major API breakage on this method (as if anyone was passing moved_cb positionally then they would both not get there callback run but wait for the motion to finish!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same dilemma. I thought already implemented move show how everybody is supposed to have it.

What about adding * after position to force use of keyword args ?

I agree it is an API change. On the other hand current situation is not ideal neither.

Another possibility is to move wait argument at the end...

"""Move to a specified position, optionally waiting for motion to
complete.

Expand All @@ -175,6 +171,8 @@ def move(self, position, moved_cb=None, timeout=None):
timeout : float, optional
Maximum time to wait for the motion. If None, the default timeout
for this positioner is used.
wait : bool, optional
Wait for the Status object to be complete (False by default)

Returns
-------
Expand Down Expand Up @@ -208,6 +206,9 @@ def move(self, position, moved_cb=None, timeout=None):

self.subscribe(status._finished, event_type=self._SUB_REQ_DONE, run=False)

if wait:
status.wait()

return status

def _done_moving(self, success=True, timestamp=None, value=None, **kwargs):
Expand Down
Loading