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

Positioner interface and PositionerManager class #39

Open
mgstingl opened this issue Feb 24, 2023 · 0 comments
Open

Positioner interface and PositionerManager class #39

mgstingl opened this issue Feb 24, 2023 · 0 comments

Comments

@mgstingl
Copy link

Hi @beniroquai !
A couple of things I noticed which might improve maintainability and readability. Happy to further discuss any of these suggestions:

  • Positioner interface and PositionerManager class
  1. In the initialization of the class, if we need to pass initialPosition to the constructor, wouldn't it make sense to also include the initialSpeed? I find it clearer to avoid it and define it either through the positionerInfo or, for stages that store the position in some internal memory (like the StandaStage), by directly "asking" the stage for its initial position/speed. I saw it is meant to be implemented, as the initial position gets overwritten by:
    self._position = self.getPosition() overwrites initialPosition in the PositionerManager parent initialization:
class ESP32StageManager(PositionerManager):
    def __init__(self, positionerInfo, name, **lowLevelManagers):
        super().__init__(positionerInfo, name, initialPosition={
            axis: 0 for axis in positionerInfo.axes
        }) 

Shouldn't it then be completely avoided in the PositionerManager class?

  1. setPosition should also be an abstract method (set_position), maybe as well as set_speed, which should also be addressable by axis. The abstract method should be:
    @abstractmethod
    def set_position(self, pos, axis):
        pass

And an example for an inherited class like the ESP32StageManager could look like:

    def set_position(self, value, axis):
        self._motor.set_position(axis=axis, position=value)
        self._position[axis] = value
  1. The home property became a bit problematic: I would change its name to is_homed(self) -> Dict[str, bool], which is again more "pythonic".
    def is_homed(self) -> Dict[str, bool]:
        """ The home of each axis. This is a dict in the format
        ``{ axis: homed }``. """
        return self._is_homed

home() should be a method to do what doHome() does in the ESP32StageManager. Same goes for stop which should be an is_stopped property, while stop() should be an abstract method like home().

In general, there are many if-clauses that depend on the axes dictionary and are called with just one axis. Like for example again the home method: this should home the complete stage, while another more specific method like home_axis(self, axis) could be used instead:

def home(self):
      self.home_xyz() # or actually self._stage.home()

Kind regards!

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

No branches or pull requests

1 participant