You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
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?
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:
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
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!
The text was updated successfully, but these errors were encountered:
Hi @beniroquai !
A couple of things I noticed which might improve maintainability and readability. Happy to further discuss any of these suggestions:
self._position = self.getPosition()
overwrites initialPosition in the PositionerManager parent initialization:Shouldn't it then be completely avoided in the PositionerManager class?
And an example for an inherited class like the ESP32StageManager could look like:
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:
Kind regards!
The text was updated successfully, but these errors were encountered: