search

mxcube / mxcubecore

Backend used by MXCuBE
http://mxcube.github.io/mxcube/
GNU Lesser General Public License v3.0
13 stars 53 forks source link

SardanaMotor states are redefined #723

Closed meghdadyazdi closed 1 year ago

meghdadyazdi commented 1 year ago

States for motors defined in AbstractMotor is a little bit weird to me like <MotorStates.HOME: (<HardwareObjectState.READY: 3>, 5)> that type(MotorStates.HOME.value) is a tuple and makes it a little bit verbose to work with it. Maybe a better definition for States can be fine in ExporterStaes. On the other hand class MotorStates(Enum) seems suffering from lack of states.

Is there any reason to define these sates like that? Can we make it like one in class ExporterStates(Enum)

meghdadyazdi commented 1 year ago

@beteva Could you please have a look? or ping someone who is working with Sardana motors?

beteva commented 1 year ago

Hi @meghdadyazdi.

States for motors defined in AbstractMotor is a little bit weird to me like <MotorStates.HOME: (<HardwareObjectState.READY: 3>, 5)

In mxcubecore we've implemented a concept, with two states - the generic state, shared by all the hardware objects, defined in HardwareObjectState and a specific state, which can differ between different hardware objects and fit the needs of a particular implementation. We've deliberately defined only few generic states, applicable to any type of hardware or action. In case you need more states (as for the motors), you define this in the specific states part. Moreover, we specify which specific state is associated to which generic one, In the above line the idea is that the specific HOME state corresponds to the generic state READY (HardwareObjectState.READY) , so you can move the motor, but it also indicates that the switch is activated (5), in case you need this information. 5 is an arbitrary number in this case, but may correspond to a real value, returned by the specific hardware implementation.

On the other hand class MotorStates(Enum) seems suffering from lack of states. Is there any reason to define these sates like that? Can we make it like one in class ExporterStates(Enum)

MotorStates is only an abstract implementation, just to give an idea how a concrete implementation can be done. What really is important is that you are accessing the generic state via self.STATES and the specific one via self.SPECIFIC_STATES. So your SPECIFIC_STATES can be MotorStates, ExporterStates, BlissMotorStates or any other enum, corresponding to your specific implementation (SardanaStates in you case). And it is not necessarily a tuple. In ExportStates we just make a conversion between the specific exporter states and the generic states, while in MotorStates this is all in one - specific to generic state (first member of the tuple) and specific state value (the second member of the tuple). This is very versatile, as the only obligation is that the state is an Enum. The specific implementation can differ. In the case of ExporterMotor, it was more useful to define ExporterStates, used not just for the motors, but for other hardware, controlled via the exporter protocol. In the case of BlissMotor, the definition is in the HardwareObject itself.

It is up to you to see what is more convenient for Sardana - implement wider SardanaStates or more specific SardanaMotorStates.

beteva commented 1 year ago

@meghdadyazdi, concerning the changes you've made:

  1. Sardana.py - it seems odd to me to have the door implementation in a generic file, but I cannot judge, as I am not using Sardana and am not aware of the specificities. Maybe somebody from ALBA could have a look.
  2. SardanaMotor.py - my comments are only on the methods and signals generalities, not the calls Sardana:
    • what you've done in state_map looks correct 👍. This is one way to implement the specific states.
    • in the init, you are emitting deviceReady and deviceNotReady. I do not know who catches this, but something like self.emit("stateChanged", self.motor_state,) is closer to the current standards.
    • only cosmetic, but as we've change from camelCase to snake_case, updateState should be update_state. Probably it is there for historic reasons.
    • sync_move - maybe you can simply use set_value(value, timeout=None) instead of sync_move. As a general remark, we consider the motors to be a subset of actuators. That's why there is now set_value, get_value and not move any more.
    • There is a missing self._nominal_limits = (self.limit_lower, self.limit_upper) in the get_limits method. This is needed for update_limits