Merge branch '2787-closed-loop-management-2' into 'master'

Resolve "closed loop management"

Closes #2787

See merge request !4596

bliss/common/axis.py

@@ -24,6 +24,7 @@ from bliss.common.logtools import log_debug, user_print, log_warning
 from bliss.common.utils import rounder
 from bliss.common.utils import autocomplete_property
 from bliss.comm.exceptions import CommunicationError
+from bliss.common.closed_loop import ClosedLoop
 
 import enum
 import gevent
@@ -677,6 +678,10 @@ class Axis(Scannable, HasMetadataForDataset):
         self._lock = gevent.lock.Semaphore()
         self.__positioner = True
         self._disabled = False
+        if config.get("closed_loop"):
+            self._closed_loop = ClosedLoop(self)
+        else:
+            self._closed_loop = None
 
         try:
             config.parent
@@ -834,6 +839,14 @@ class Axis(Scannable, HasMetadataForDataset):
     def backlash(self, backlash):
         self.settings.set("backlash", backlash)
 
+    @property
+    @lazy_init
+    def closed_loop(self):
+        """
+        Closed loop object associated to axis.
+        """
+        return self._closed_loop
+
     @property
     def tolerance(self):
         """Current Axis tolerance in dial units (:obj:`float`)"""
@@ -1268,7 +1281,16 @@ class Axis(Scannable, HasMetadataForDataset):
             except Exception:
                 info_string += "ERROR: Unable to get encoder info\n"
         else:
-            info_string += "ENCODER:\n     None\n"
+            info_string += "ENCODER:\n     None\n\n"
+
+        # CLOSED_LOOP
+        if self.closed_loop is not None:
+            try:
+                info_string += info(self.closed_loop)
+            except Exception:
+                info_string += "ERROR: Unable to get closed loop info\n"
+        else:
+            info_string += "CLOSED LOOP:\n     None\n"
 
         return info_string
 
@@ -2262,6 +2284,9 @@ class Axis(Scannable, HasMetadataForDataset):
         if reload:
             self.config.reload()
 
+        if self._closed_loop is not None:
+            self._closed_loop.apply_config(reload)
+
         if self.encoder is not None:
             self.encoder.apply_config(reload)
 

bliss/common/closed_loop.py

+# -*- coding: utf-8 -*-
+#
+# This file is part of the bliss project
+#
+# Copyright (c) 2015-2022 Beamline Control Unit, ESRF
+# Distributed under the GNU LGPLv3. See LICENSE for more info.
+
+import enum
+from functools import partial
+
+from bliss.config.beacon_object import BeaconObject, EnumProperty
+
+
+class ClosedLoopState(enum.Enum):
+    UNKNOWN = enum.auto()
+    ON = enum.auto()
+    OFF = enum.auto()
+
+
+def fget_gen(key):
+    def f(key, self):
+        return self.axis.controller.get_closed_loop_param(self.axis, key)
+
+    fget = partial(f, key)
+    fget.__name__ = key
+    return fget
+
+
+def fset_gen(key):
+    def f(key, self, value):
+        if self._setters_on:
+            return self.axis.controller.set_closed_loop_param(self.axis, key, value)
+
+    fset = partial(f, key)
+    fset.__name__ = key
+    return fset
+
+
+class ClosedLoop(BeaconObject):
+    """
+    config example:
+    - name: m1
+      steps_per_unit: 1000
+      velocity: 50
+      acceleration: 1
+      encoder: $m1enc
+      closed_loop:
+          state: on
+          kp: 1
+          ki: 2
+          kd: 3
+          settling_window: 0.1
+          settling_time: 3
+    """
+
+    def __new__(cls, axis):
+        """Make a class copy per instance to allow closed loop objects to own different properties"""
+        cls = type(cls.__name__, (cls,), {})
+        return object.__new__(cls)
+
+    def __init__(self, axis):
+        self._axis = axis
+        name = f"{axis.name}:closed_loop"
+        config = axis.config.config_dict
+        if isinstance(config, dict):
+            super().__init__(config.get("closed_loop"), name=name)
+        else:
+            super().__init__(config, name=name, path=["closed_loop"])
+
+        setattr(
+            self.__class__,
+            "_state",
+            EnumProperty("state", ClosedLoopState, must_be_in_config=True),
+        )
+
+        self._setters_on = False
+        self._init_properties()
+
+    def _init_properties(self):
+        """Instantiate properties depending on the controller requirements"""
+        reqs = self.axis.controller.get_closed_loop_requirements()
+        for key in reqs:
+            if hasattr(self, key):
+                raise Exception(
+                    f"Cannot create closed loop property '{key}', name already exists"
+                )
+            setattr(
+                self.__class__,
+                key,
+                BeaconObject.property(
+                    fget=fget_gen(key), fset=fset_gen(key), must_be_in_config=True
+                ),
+            )
+
+    def __info__(self):
+        info_str = "CLOSED LOOP:\n"
+        info_str += f"     state: {self.state.name}\n"
+        for key in self.axis.controller.get_closed_loop_requirements():
+            info_str += f"     {key}: {getattr(self, key)}\n"
+        return info_str
+
+    @property
+    def axis(self):
+        return self._axis
+
+    @property
+    def state(self):
+        return self._state
+
+    def _activate(self, onoff):
+        new_state = self.axis.controller.activate_closed_loop(self.axis, onoff)
+        if not isinstance(new_state, ClosedLoopState):
+            raise ValueError(
+                f"Controller expected to return ClosedLoopState, got {new_state}"
+            )
+        else:
+            self._state = new_state
+
+    def on(self):
+        self._activate(True)
+
+    def off(self):
+        self._activate(False)
+
+    def _activate_setters(self):
+        self._setters_on = True

bliss/config/beacon_object.py

@@ -136,7 +136,15 @@ class EnumProperty(_Property):
         doc: Documentation
     """
 
-    def __init__(self, name, enum_type, unknown_value=None, default=None, doc=None):
+    def __init__(
+        self,
+        name,
+        enum_type,
+        unknown_value=None,
+        default=None,
+        doc=None,
+        must_be_in_config=False,
+    ):
         def fget(self):
             return self.settings.get(name, default)
 
@@ -162,7 +170,12 @@ class EnumProperty(_Property):
             return result
 
         _Property.__init__(
-            self, fget=fget, fset=fset, doc=doc, set_unmarshalling=set_unmarshalling
+            self,
+            fget=fget,
+            fset=fset,
+            doc=doc,
+            set_unmarshalling=set_unmarshalling,
+            must_be_in_config=must_be_in_config,
         )
 
 

bliss/controllers/motor.py

@@ -21,6 +21,7 @@ import bliss.common.motor_group as motor_group
 from bliss.common.motor_config import MotorConfig
 from bliss.common.motor_settings import ControllerAxisSettings, floatOrNone
 from bliss.common.axis import Trajectory
+from bliss.common.closed_loop import ClosedLoopState
 from bliss.common import event
 from bliss.controllers.counter import SamplingCounterController
 from bliss.physics import trajectory
@@ -353,6 +354,12 @@ class Controller(BlissController):
                     self.initialize_hardware_axis(axis)
                     axis.settings.check_config_settings()
                     axis.settings.init()  # get settings, from config or from cache, and apply to hardware
+                    if axis.closed_loop:
+                        if axis.closed_loop.state != self.get_closed_loop_state(axis):
+                            self.activate_closed_loop(
+                                axis, axis.closed_loop.state == ClosedLoopState.ON
+                            )
+                        axis.closed_loop._activate_setters()
                     axis_initialized.value = 1
 
             except BaseException:
@@ -383,6 +390,22 @@ class Controller(BlissController):
     def finalize_axis(self, axis):
         raise NotImplementedError
 
+    def get_closed_loop_requirements(self):
+        """Return the list of keys this controller expects in a closed loop config"""
+        raise NotImplementedError
+
+    def get_closed_loop_state(self, axis):
+        raise NotImplementedError
+
+    def activate_closed_loop(self, axis, onoff=True):
+        raise NotImplementedError
+
+    def set_closed_loop_param(self, axis, param, value):
+        raise NotImplementedError
+
+    def get_closed_loop_param(self, axis, param):
+        raise NotImplementedError
+
     def get_class_name(self):
         return self.__class__.__name__
 

bliss/controllers/motors/mockup.py

@@ -25,6 +25,7 @@ import numpy as np
 from bliss.physics.trajectory import LinearTrajectory
 from bliss.controllers.motor import Controller, CalcController
 from bliss.common.axis import Axis, AxisState
+from bliss.common.closed_loop import ClosedLoopState
 from bliss.common import event
 from bliss.config.settings import SimpleSetting
 from bliss.common.hook import MotionHook
@@ -85,6 +86,10 @@ class Mockup(Controller):
         self._axis_moves = {}
         self.__encoders = {}
         self.__switches = {}
+
+        self._cloop_state = {}
+        self._cloop_params = {}
+
         self._axes_data = collections.defaultdict(dict)
 
         # Custom attributes.
@@ -130,8 +135,13 @@ class Mockup(Controller):
         if self.read_hw_position(axis) is None:
             self.set_hw_position(axis, 0)
 
+        self._cloop_state[axis] = ClosedLoopState.UNKNOWN
+
     def initialize_hardware_axis(self, axis):
-        pass
+        if axis.closed_loop:
+            self._cloop_params["kp"] = axis.closed_loop.kp
+            self._cloop_params["ki"] = axis.closed_loop.ki
+            self._cloop_params["kd"] = axis.closed_loop.kd
 
     def initialize_axis(self, axis):
         log_debug(self, "initializing axis %s", axis.name)
@@ -478,6 +488,39 @@ class Mockup(Controller):
         """
         self.set_position(axis, self.read_position(axis) + disc)
 
+    """
+    CLOSED LOOP
+    """
+
+    def get_closed_loop_requirements(self):
+        return ["kp", "ki", "kd"]
+
+    def get_closed_loop_state(self, axis):
+        return self._cloop_state[axis]
+
+    def activate_closed_loop(self, axis, onoff=True):
+        if onoff:
+            self._cloop_state[axis] = ClosedLoopState.ON
+        else:
+            self._cloop_state[axis] = ClosedLoopState.OFF
+        return self._cloop_state[axis]
+
+    def set_closed_loop_param(self, axis, param, value):
+        """
+        Hardware specific method to set closed loop parameters.
+        """
+        if param not in self.get_closed_loop_requirements():
+            raise KeyError(f"Unknown closed loop parameter: {param}")
+        self._cloop_params[param] = value
+
+    def get_closed_loop_param(self, axis, param):
+        """
+        Hardware specific method to read closed loop parameters.
+        """
+        if param not in self.get_closed_loop_requirements():
+            raise KeyError(f"Unknown closed loop parameter: {param}")
+        return self._cloop_params[param]
+
     """
     Custom axis methods
     """
@@ -514,11 +557,6 @@ class Mockup(Controller):
     def custom_send_command(self, axis, value):
         log_debug(self, "custom_send_command(axis=%s value=%r):" % (axis.name, value))
 
-    # BOOL NONE
-    @object_method(name="Set_Closed_Loop", types_info=("bool", "None"))
-    def _set_closed_loop(self, axis, onoff=True):
-        pass  # print "I set the closed loop ", onoff
-
     # Types by default (None, None)
     @object_method
     def custom_command_no_types(self, axis):

bliss/controllers/motors/pi_e712.py

@@ -17,6 +17,7 @@ from bliss.common.axis import AxisState, Motion, CyclicTrajectory
 from bliss.config.channels import Cache
 from bliss.common.switch import Switch as BaseSwitch
 from bliss.common.logtools import log_info, log_debug, log_error
+from bliss.common.closed_loop import ClosedLoopState
 
 from . import pi_gcs
 
@@ -39,14 +40,16 @@ config example:
     velocity: 100
     acceleration: 1.
     steps_per_unit: 1
-    servo_mode: 1
+    closed_loop:
+        state: on
 
   - name: px
     channel: 2
     velocity: 100
     acceleration: 1.
     steps_per_unit: 1
-    servo_mode: 1
+    closed_loop:
+        state: on
 """
 
 
@@ -57,7 +60,6 @@ class PI_E712(pi_gcs.Communication, pi_gcs.Recorder, Controller):
         Controller.__init__(self, *args, **kwargs)
 
         self.cname = "E712"
-        self.__axis_closed_loop = weakref.WeakKeyDictionary()
 
     def _create_subitem_from_config(
         self, name, cfg, parent_key, item_class, item_obj=None
@@ -99,13 +101,6 @@ class PI_E712(pi_gcs.Communication, pi_gcs.Recorder, Controller):
 
         self._gate_enabled = False
 
-        # Updates cached value of closed loop status.
-        closed_loop_cache = Cache(axis, "closed_loop")
-        self.__axis_closed_loop[axis] = closed_loop_cache
-        if closed_loop_cache.value is None:
-            closed_loop_cache.value = self._get_closed_loop_status(axis)
-
-        add_property(axis, "closed_loop", lambda x: self.__axis_closed_loop[x].value)
         self.check_power_cut()
 
         log_debug(self, "axis = %r" % axis.name)
@@ -115,12 +110,6 @@ class PI_E712(pi_gcs.Communication, pi_gcs.Recorder, Controller):
         # supposed that we are on target on init
         axis._last_on_target = True
 
-        # check servo mode (default true)
-        servo_mode = axis.config.get("servo_mode", converter=None, default=True)
-        if axis.closed_loop != servo_mode:
-            # spawn if to avoid recursion
-            gevent.spawn(self.activate_closed_loop, axis, servo_mode)
-
     def initialize_encoder(self, encoder):
         pass
 
@@ -503,7 +492,9 @@ class PI_E712(pi_gcs.Communication, pi_gcs.Recorder, Controller):
         """
         return float(self.command("VOL? %s" % axis.channel))
 
-    @object_method(types_info=("bool", "None"))
+    def get_closed_loop_requirements(self):
+        return []
+
     def activate_closed_loop(self, axis, onoff=True):
         """
         Activate/Desactivate closed loop status (Servo state) (SVO command)
@@ -535,17 +526,16 @@ class PI_E712(pi_gcs.Communication, pi_gcs.Recorder, Controller):
                     ),
                 )
 
-        # Updates bliss setting (internal cached) position.
-        self.__axis_closed_loop[axis].value = onoff
-
         axis._update_dial()
+        return ClosedLoopState.ON if onoff else ClosedLoopState.OFF
 
-    def _get_closed_loop_status(self, axis):
+    def get_closed_loop_state(self, axis):
         """
         Returns Closed loop status (Servo state) (SVO? command)
         -> True/False
         """
-        return bool(int(self.command("SVO? %s" % axis.channel)))
+        result = int(self.command("SVO? %s" % axis.channel))
+        return ClosedLoopState.ON if result else ClosedLoopState.OFF
 
     def _get_on_target_status(self, axis):
         """
@@ -658,6 +648,12 @@ class PI_E712(pi_gcs.Communication, pi_gcs.Recorder, Controller):
         """
         self.command("SPA %s 0x2000%d00 %f" % (axis.channel, coeff + 2, value))
 
+    def set_closed_loop_param(self, axis, param, value):
+        raise KeyError(f"Unknown closed loop parameter: {param}")
+
+    def get_closed_loop_param(self, axis, param):
+        raise KeyError(f"Unknown closed loop parameter: {param}")
+
     def _get_tns(self, axis):
         """Get Normalized Input Signal Value. Loop 10 times to straighten out noise"""
         accu = 0

doc/docs/motion_axis.md

@@ -33,6 +33,7 @@ the `Axis` constructor.
 Parameter name                              |  Required | Setting?  | Type   | Description
 ------------------------------------------- |-----------|-----------|--------|------------
 [name](motion_axis.md#name)                 |  yes      | no        | string | An unique name to identify the `Axis` object
+[unit](motion_axis.md#unit)                 |  no       | no        | string | *Informative only* - Unit (for steps per unit), e.g. mm, deg, rad, etc.
 [steps_per_unit](motion_axis.md#position)   |  yes      | no        | float  | Number of steps to send to the controller to make a *move of 1 unit* (eg. 1 mm, 1 rad)
 [velocity](motion_axis.md#velocity)         |  yes      | yes       | float  | Nominal axis velocity in *units.s<sup>-1</sup>*
 [acceleration](motion_axis.md#acceleration) |  yes      | yes       | float  | Nominal acceleration value in *units.s<sup>-2</sup>*
@@ -42,7 +43,7 @@ Parameter name                              |  Required | Setting?  | Type   | D
 [backlash](motion_axis.md#backlash)         |  no       | yes       | float  | Axis backlash in user units ; *defaults to 0*
 [tolerance](motion_axis.md#tolerance)       |  no       | no        | float  | Accepted discrepancy between controller position and last known axis dial position when starting a move ; *defaults to 1E-4*
 [encoder](motion_axis.md#encoder)           |  no       | no        | string | Name of an existing **Encoder** object linked with this axis
-[unit](motion_axis.md#unit)                 | no        | no        | string | *Informative only* - Unit (for steps per unit), e.g. mm, deg, rad, etc.
+[closed loop](motion_axis.md#closed_loop)   |  no       | -         | dict   | Axis subsection to configure closed loop (see [closed loop](motion_closed_loop.md))
 
 !!! note
     Motor controllers with extra features may require more parameters. See the
@@ -426,6 +427,8 @@ Default value is 1E-4
 
 ### encoder
 
+Encoders are standalone objects which can be referred by axes.
+Thus they have a dedicated section here: [Encoder](motion_encoder.md).
 
 ### Axis state
 

doc/docs/motion_closed_loop.md

+# Closed loop
+
+Every axis can define a closed loop, provided they are built on top of a controller which supports it.
+At the moment, only hardware closed loops are supported.
+
+
+## Configuration
+
+A closed loop is defined by creating a **closed_loop** section under an axis.
+
+The section should always contain a **state**.
+Then, each type of motor controller requires a specific set of parameters, known as
+[controller requirements](motion_closed_loop.md#controller-requirements).
+
+```yaml
+controller:
+  - class: Mockup
+    module: mockup
+    axes:
+      - name: servo1
+        steps_per_unit: 1000
+        velocity_high_limit: 500
+        velocity_low_limit: 10
+        velocity: 100
+        acceleration: 200
+        low_limit: -1000
+        high_limit: 1000
+        unit: mm
+        closed_loop:
+            state: on
+            kp: 0.1 # <─┬ controller specific
+            ki: 0.2 # <─┤
+            kd: 0.3 # <─┘
+```
+
+### Controller requirements
+
+Each motor controller provides the list of parameters it needs to instantiate a closed loop.
+One should refer to the particular controller documentation to find the list and description
+for these parameters.
+
+When a parameter is missing, an error provides details on what is missing:
+```
+!!! === RuntimeError: For device servo1:closed_loop configuration must
+contains {'kp', 'ki', 'kd'}. === !!! ( for more details type cmd 'last_error' )
+```
+
+Alternatively, it is possible to ask the controller directly with *get_closed_loop_requirements()*:
+```python
+TEST_SESSION [1]: servo1.controller.get_closed_loop_requirements()
+         Out [1]: ['kp', 'ki', 'kd']
+```
+
+## Usage
+
+Closed loops are always related to an axis, thus they have no name and are located under their axis.
+
+```python
+TEST_SESSION [2]: servo1.closed_loop
+         Out [2]: CLOSED LOOP:
+                       state: ON
+                       kp: 0.1
+                       ki: 0.2
+                       kd: 0.3
+```
+
+They can be turned on and off with the so-named functions. The controller specific parameters
+are also accessible this way.
+```python
+TEST_SESSION [3]: servo1.closed_loop.off()
+TEST_SESSION [4]: servo1.closed_loop.ki = 0.75
+TEST_SESSION [5]: servo1.closed_loop
+         Out [5]: CLOSED LOOP:
+                       state: OFF
+                       kp: 0.1
+                       ki: 0.75
+                       kd: 0.3
+```
+
+**Note:** Every parameters (state included) are stored in redis, consequently the closed loop will always
+start in the same state it has been left in.

doc/mkdocs.yml

@@ -173,6 +173,7 @@ nav:
         - Motion:
                 - Axis: motion_axis.md
                 - Encoder: motion_encoder.md
+                - Closed loop: motion_closed_loop.md
                 - Soft Axis: motion_softaxis.md
                 - Hook: motion_hook.md
                 - Stack: motion_stack.md

tests/emulators/emulator.py

@@ -5,8 +5,8 @@
 # Copyright (c) 2015-2022 Beamline Control Unit, ESRF
 # Distributed under the GNU LGPLv3. See LICENSE for more info.
 
-"""Emulator :mod:`~bliss.controllers.emulator.Server` and \
-:mod:`~bliss.controllers.emulator.BaseDevice`
+"""Emulator :mod:`~tests.emulators.emulator.Server` and \
+:mod:`~tests.emulators.emulator.BaseDevice`
 
 Quick start
 -----------
@@ -24,7 +24,7 @@ To create a server use the following configuration as a starting point:
 
 To start the server you can do something like::
 
-    $ python -m bliss.controllers.emulator my_emulator
+    $ python -m tests.emulators.emulator my_emulator
 
 (bliss also provides a ``bliss-emulator`` script which basically does the same)
 
@@ -97,7 +97,8 @@ class EmulatorServerMixin(object):
             addr tuple): address (tuple of host, port)
         """
         if self.newline == "\n" and not self.special_messages:
-            for line in file_obj:
+            for buf in file_obj:
+                line = buf.decode()
                 self.handle_line(sock, line)
         else:
             # warning: in this mode read will block even if client
@@ -105,7 +106,7 @@ class EmulatorServerMixin(object):
             buff = ""
             finish = False
             while not finish:
-                readout = file_obj.read(1)
+                readout = file_obj.read(1).decode()
                 if not readout:
                     return
                 buff += readout
@@ -129,17 +130,13 @@ class EmulatorServerMixin(object):
             sock (gevent.socket.socket): new socket resulting from an accept
             addr (tuple): address (tuple of host, port)
             line (str): line to be processed
-
-        Returns:
-            str: response to give to client or None if no response
         """
         self.pause(len(line))
         response = self.device.handle_line(line)
         if response is not None: