ReadHandler and WriteHandler decorators

modules with a couple of parameters with similar read_* or
write_* methods may handle them by generic methods wrapped
with decorators ReadHandler / WriteHandler

The trinamic driver is included in this change for demonstrating
how it works.

In a further step, the special handling for the iohandler stuff can
be moved away from secop.server and secop.params, using this feature.

+ fix problem on startup of trinamic driver (needs MultiEvent.queue)
+ some other small fixes
+ apply recommended functools.wraps for wrapping

Change-Id: Ibfeff9209f53c47194628463466cee28366e17ac
Reviewed-on: https://forge.frm2.tum.de/review/c/sine2020/secop/playground/+/27460
Tested-by: Jenkins Automated Tests <pedersen+jenkins@frm2.tum.de>
Reviewed-by: Enrico Faulhaber <enrico.faulhaber@frm2.tum.de>
Reviewed-by: Markus Zolliker <markus.zolliker@psi.ch>

secop/io.py

@@ -86,7 +86,7 @@ class IOBase(Communicator):
     uri = Property('hostname:portnumber', datatype=StringType())
     timeout = Parameter('timeout', datatype=FloatRange(0), default=2)
     wait_before = Parameter('wait time before sending', datatype=FloatRange(), default=0)
-    is_connected = Parameter('connection state', datatype=BoolType(), readonly=False, poll=REGULAR)
+    is_connected = Parameter('connection state', datatype=BoolType(), readonly=False, default=False, poll=REGULAR)
     pollinterval = Parameter('reconnect interval', datatype=FloatRange(0), readonly=False, default=10)
 
     _reconnectCallbacks = None
@@ -95,6 +95,7 @@ class IOBase(Communicator):
     _lock = None
 
     def earlyInit(self):
+        super().earlyInit()
         self._lock = threading.RLock()
 
     def connectStart(self):

secop/lib/__init__.py

@@ -323,4 +323,4 @@ class UniqueObject:
         self.name = name
 
     def __repr__(self):
-        return 'UniqueObject(%r)' % self.name
+        return self.name

secop/lib/multievent.py

@@ -59,6 +59,7 @@ class MultiEvent(threading.Event):
         self._lock = threading.Lock()
         self.default_timeout = default_timeout or None  # treat 0 as None
         self.name = None  # default event name
+        self._actions = []  # actions to be executed on trigger
         super().__init__()
 
     def new(self, timeout=None, name=None):
@@ -81,6 +82,12 @@ class MultiEvent(threading.Event):
             self.events.discard(event)
             if self.events:
                 return
+            try:
+                for action in self._actions:
+                    action()
+            except Exception:
+                pass  # we silently ignore errors here
+            self._actions = []
             super().set()
 
     def clear_(self, event):
@@ -116,3 +123,19 @@ class MultiEvent(threading.Event):
         as a convenience method
         """
         return self.new(timeout, name).set
+
+    def queue(self, action):
+        """add an action to the queue of actions to be executed at end
+
+        :param action: a function, to be executed after the last event is triggered,
+            and before the multievent is set
+
+        - if no events are waiting, the actions are executed immediately
+        - if an action raises an exception, it is silently ignore and further
+          actions in the queue are skipped
+        - if this is not desired, the action should handle errors by itself
+        """
+        with self._lock:
+            self._actions.append(action)
+            if self.is_set():
+                self.set_(None)

secop/modules.py

@@ -26,6 +26,7 @@
 import sys
 import time
 from collections import OrderedDict
+from functools import wraps
 
 from secop.datatypes import ArrayOf, BoolType, EnumType, FloatRange, \
     IntRange, StatusType, StringType, TextType, TupleOf
@@ -38,6 +39,7 @@ from secop.poller import BasicPoller, Poller
 from secop.properties import HasProperties, Property
 from secop.logging import RemoteLogHandler, HasComlog
 
+
 Done = UniqueObject('already set')
 """a special return value for a read/write function
 
@@ -107,12 +109,14 @@ class HasAccessibles(HasProperties):
             # XXX: create getters for the units of params ??
 
             # wrap of reading/writing funcs
-            if isinstance(pobj, Command):
+            if not isinstance(pobj, Parameter):
-                # nothing to do for now
+                # nothing to do for Commands
                 continue
+
             rfunc = getattr(cls, 'read_' + pname, None)
+            # TODO: remove handler stuff here
             rfunc_handler = pobj.handler.get_read_func(cls, pname) if pobj.handler else None
-            wrapped = hasattr(rfunc, '__wrapped__')
+            wrapped = getattr(rfunc, 'wrapped', False)  # meaning: wrapped or auto generated
             if rfunc_handler:
                 if 'read_' + pname in cls.__dict__:
                     if pname in cls.__dict__:
@@ -126,73 +130,78 @@ class HasAccessibles(HasProperties):
             # create wrapper except when read function is already wrapped
             if not wrapped:
 
-                def wrapped_rfunc(self, pname=pname, rfunc=rfunc):
+                if rfunc:
-                    if rfunc:
+
-                        self.log.debug("call read_%s" % pname)
+                    @wraps(rfunc)  # handles __wrapped__ and __doc__
+                    def new_rfunc(self, pname=pname, rfunc=rfunc):
                         try:
                             value = rfunc(self)
-                            if value is Done:  # the setter is already triggered
+                            self.log.debug("read_%s returned %r", pname, value)
-                                value = getattr(self, pname)
-                                self.log.debug("read_%s returned Done (%r)" % (pname, value))
-                                return value
-                            self.log.debug("read_%s returned %r" % (pname, value))
                         except Exception as e:
-                            self.log.debug("read_%s failed %r" % (pname, e))
+                            self.log.debug("read_%s failed with %r", pname, e)
-                            self.announceUpdate(pname, None, e)
                             raise
-                    else:
+                        if value is Done:
-                        # return cached value
+                            return getattr(self, pname)
-                        value = self.accessibles[pname].value
+                        setattr(self, pname, value)  # important! trigger the setter
-                        self.log.debug("return cached %s = %r" % (pname, value))
+                        return value
-                    setattr(self, pname, value)  # important! trigger the setter
+                else:
-                    return value
 
-                if rfunc:
+                    def new_rfunc(self, pname=pname):
-                    wrapped_rfunc.__doc__ = rfunc.__doc__
+                        return getattr(self, pname)
-                setattr(cls, 'read_' + pname, wrapped_rfunc)
+
-                wrapped_rfunc.__wrapped__ = True
+                    new_rfunc.__doc__ = 'auto generated read method for ' + pname
+
+                new_rfunc.wrapped = True  # indicate to subclasses that no more wrapping is needed
+                setattr(cls, 'read_' + pname, new_rfunc)
 
             if not pobj.readonly:
                 wfunc = getattr(cls, 'write_' + pname, None)
-                wrapped = hasattr(wfunc, '__wrapped__')
+                wrapped = getattr(wfunc, 'wrapped', False)  # meaning: wrapped or auto generated
                 if (wfunc is None or wrapped) and pobj.handler:
                     # ignore the handler, if a write function is present
+                    # TODO: remove handler stuff here
                     wfunc = pobj.handler.get_write_func(pname)
                     wrapped = False
 
                 # create wrapper except when write function is already wrapped
                 if not wrapped:
 
-                    def wrapped_wfunc(self, value, pname=pname, wfunc=wfunc):
+                    if wfunc:
-                        pobj = self.accessibles[pname]
+
-                        if wfunc:
+                        @wraps(wfunc)  # handles __wrapped__ and __doc__
-                            self.log.debug("check and call write_%s(%r)" % (pname, value))
+                        def new_wfunc(self, value, pname=pname, wfunc=wfunc):
+                            pobj = self.accessibles[pname]
+                            self.log.debug('validate %r for %r', value, pname)
+                            # we do not need to handle errors here, we do not
+                            # want to make a parameter invalid, when a write failed
                             value = pobj.datatype(value)
                             returned_value = wfunc(self, value)
-                            if returned_value is Done:  # the setter is already triggered
+                            self.log.debug('write_%s(%r) returned %r', pname, value, returned_value)
+                            if returned_value is Done:
+                                # setattr(self, pname, getattr(self, pname))
                                 return getattr(self, pname)
-                            if returned_value is not None:  # goodie: accept missing return value
+                            setattr(self, pname, value)  # important! trigger the setter
-                                value = returned_value
+                            return value
-                        else:
+                    else:
-                            self.log.debug("check %s = %r" % (pname, value))
-                            value = pobj.datatype(value)
-                        setattr(self, pname, value)
-                        return value
 
-                    if wfunc:
+                        def new_wfunc(self, value, pname=pname):
-                        wrapped_wfunc.__doc__ = wfunc.__doc__
+                            setattr(self, pname, value)
-                    setattr(cls, 'write_' + pname, wrapped_wfunc)
+                            return value
-                    wrapped_wfunc.__wrapped__ = True
+
+                        new_wfunc.__doc__ = 'auto generated write method for ' + pname
+
+                    new_wfunc.wrapped = True  # indicate to subclasses that no more wrapping is needed
+                    setattr(cls, 'write_' + pname, new_wfunc)
 
         # check for programming errors
-        for attrname in cls.__dict__:
+        for attrname, attrvalue in cls.__dict__.items():
             prefix, _, pname = attrname.partition('_')
             if not pname:
                 continue
             if prefix == 'do':
                 raise ProgrammingError('%r: old style command %r not supported anymore'
                                        % (cls.__name__, attrname))
-            if prefix in ('read', 'write') and not isinstance(accessibles.get(pname), Parameter):
+            if prefix in ('read', 'write') and not getattr(attrvalue, 'wrapped', False):
                 raise ProgrammingError('%s.%s defined, but %r is no parameter'
                                        % (cls.__name__, attrname, pname))
 
@@ -394,7 +403,7 @@ class Module(HasAccessibles):
                                       'value and was not given in config!' % pname)
                     # we do not want to call the setter for this parameter for now,
                     # this should happen on the first read
-                    pobj.readerror = ConfigError('not initialized')
+                    pobj.readerror = ConfigError('parameter %r not initialized' % pname)
                     # above error will be triggered on activate after startup,
                     # when not all hardware parameters are read because of startup timeout
                     pobj.value = pobj.datatype(pobj.datatype.default)
@@ -459,11 +468,14 @@ class Module(HasAccessibles):
 
     def announceUpdate(self, pname, value=None, err=None, timestamp=None):
         """announce a changed value or readerror"""
+
+        # TODO: remove readerror 'property' and replace value with exception
         pobj = self.parameters[pname]
         timestamp = timestamp or time.time()
         changed = pobj.value != value
         try:
             # store the value even in case of error
+            # TODO: we should neither check limits nor convert string to float here
             pobj.value = pobj.datatype(value)
         except Exception as e:
             if not err:  # do not overwrite given error

secop/params.py

@@ -196,7 +196,6 @@ class Parameter(Accessible):
             self.ownProperties = {k: getattr(self, k) for k in self.propertyDict}
 
     def __get__(self, instance, owner):
-        # not used yet
         if instance is None:
             return self
         return instance.parameters[self.name].value

secop/persistent.py

@@ -103,6 +103,7 @@ class PersistentMixin(HasAccessibles):
                 try:
                     value = pobj.datatype.import_value(self.persistentData[pname])
                     pobj.value = value
+                    pobj.readerror = None
                     if not pobj.readonly:
                         writeDict[pname] = value
                 except Exception as e:
@@ -144,5 +145,6 @@ class PersistentMixin(HasAccessibles):
 
     @Command()
     def factory_reset(self):
+        """reset to values from config / default values"""
         self.writeDict.update(self.initData)
         self.writeInitParams()

secop/protocol/dispatcher.py

@@ -238,13 +238,9 @@ class Dispatcher:
 
         # validate!
         value = pobj.datatype(value)
-        writefunc = getattr(moduleobj, 'write_%s' % pname, None)
         # note: exceptions are handled in handle_request, not here!
-        if writefunc:
+        getattr(moduleobj, 'write_' + pname)(value)
-            # return value is ignored here, as it is automatically set on the pobj and broadcast
+        # return value is ignored here, as already handled
-            writefunc(value)
-        else:
-            setattr(moduleobj, pname, value)
         return pobj.export_value(), dict(t=pobj.timestamp) if pobj.timestamp else {}
 
     def _getParameterValue(self, modulename, exportedname):
@@ -261,11 +257,9 @@ class Dispatcher:
             # raise ReadOnlyError('This parameter is constant and can not be accessed remotely.')
             return pobj.datatype.export_value(pobj.constant)
 
-        readfunc = getattr(moduleobj, 'read_%s' % pname, None)
+        # note: exceptions are handled in handle_request, not here!
-        if readfunc:
+        getattr(moduleobj, 'read_' + pname)()
-            # should also update the pobj (via the setter from the metaclass)
+        # return value is ignored here, as already handled
-            # note: exceptions are handled in handle_request, not here!
-            readfunc()
         return pobj.export_value(), dict(t=pobj.timestamp) if pobj.timestamp else {}
 
     #

secop/rwhandler.py

@@ -0,0 +1,135 @@
+#!/usr/bin/env python
+#  -*- coding: utf-8 -*-
+# *****************************************************************************
+# This program is free software; you can redistribute it and/or modify it under
+# the terms of the GNU General Public License as published by the Free Software
+# Foundation; either version 2 of the License, or (at your option) any later
+# version.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+# FOR A PARTICULAR PURPOSE.  See the GNU General Public License for more
+# details.
+#
+# You should have received a copy of the GNU General Public License along with
+# this program; if not, write to the Free Software Foundation, Inc.,
+# 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+#
+# Module authors:
+#   Markus Zolliker <markus.zolliker@psi.ch>
+# *****************************************************************************
+
+"""decorator class for common read_/write_ methods
+
+Usage:
+
+Example 1: combined read/write for multiple parameters
+
+    PID_PARAMS = ['p', 'i', 'd']
+
+    @ReadHandler(PID_PARAMS)
+    def read_pid(self, pname):
+        self.p, self.i, self.d = self.get_pid_from_hw()
+        return Done  # Done is indicating that the parameters are already assigned
+
+    @WriteHandler(PID_PARAMS)
+    def write_pid(self, pname, value):
+        pid = self.get_pid_from_hw()  # assume this returns a list
+        pid[PID_PARAMS.index(pname)] = value
+        self.put_pid_to_hw(pid)
+        return self.read_pid()
+
+Example 2: addressable HW parameters
+
+    HW_ADDR = {'p': 25, 'i': 26, 'd': 27}
+
+    @ReadHandler(HW_ADDR)
+    def read_addressed(self, pname):
+        return self.get_hw_register(HW_ADDR[pname])
+
+    @WriteHandler(HW_ADDR)
+    def write_addressed(self, pname, value):
+        self.put_hw_register(HW_ADDR[pname], value)
+        return self.get_hw_register(HW_ADDR[pname])
+"""
+
+from functools import wraps
+from secop.modules import Done
+from secop.errors import ProgrammingError
+
+
+class Handler:
+    func = None
+    method_names = set()  # this is shared among all instances of handlers!
+    wrapped = True  # allow to use read_* or write_* as name of the decorated method
+
+    def __init__(self, keys):
+        """initialize the decorator
+
+        :param keys: parameter names (an iterable)
+        """
+        self.keys = set(keys)
+
+    def __call__(self, func):
+        """decorator call"""
+        self.func = func
+        if func.__qualname__ in self.method_names:
+            raise ProgrammingError('duplicate method %r' % func.__qualname__)
+        func.wrapped = False
+        # __qualname__ used here (avoid conflicts between different modules)
+        self.method_names.add(func.__qualname__)
+        return self
+
+    def __get__(self, obj, owner=None):
+        """allow access to the common method"""
+        if obj is None:
+            return self
+        return self.func.__get__(obj, owner)
+
+
+class ReadHandler(Handler):
+    """decorator for read methods"""
+
+    def __set_name__(self, owner, name):
+        """create the wrapped read_* methods"""
+
+        self.method_names.discard(self.func.__qualname__)
+        for key in self.keys:
+
+            @wraps(self.func)
+            def wrapped(module, pname=key, func=self.func):
+                value = func(module, pname)
+                if value is not Done:
+                    setattr(module, pname, value)
+                return value
+
+            wrapped.wrapped = True
+            method = 'read_' + key
+            if hasattr(owner, method):
+                raise ProgrammingError('superfluous method %s.%s (overwritten by ReadHandler)'
+                                       % (owner.__name__, method))
+            setattr(owner, method, wrapped)
+
+
+class WriteHandler(Handler):
+    """decorator for write methods"""
+
+    def __set_name__(self, owner, name):
+        """create the wrapped write_* methods"""
+
+        self.method_names.discard(self.func.__qualname__)
+        for key in self.keys:
+
+            @wraps(self.func)
+            def wrapped(module, value, pname=key, func=self.func):
+                value = func(module, pname, value)
+                if value is not Done:
+                    setattr(module, pname, value)
+                return value
+
+            wrapped.wrapped = True
+            method = 'write_' + key
+            if hasattr(owner, method):
+                raise ProgrammingError('superfluous method %s.%s (overwritten by WriteHandler)'
+                                       % (owner.__name__, method))
+            setattr(owner, method, wrapped)

secop_psi/ppms.py

@@ -320,7 +320,7 @@ class Chamber(PpmsBase, Drivable):
         general_failure=15,
     )
 
-    value = Parameter(description='chamber state', handler=chamber,
+    value = Parameter(description='chamber state',  # handler=chamber,
                       datatype=EnumType(StatusCode))
     target = Parameter(description='chamber command', handler=chamber,
                        datatype=EnumType(Operation))
@@ -482,7 +482,7 @@ class Temp(PpmsBase, Drivable):
         if workingramp != 2 or not self._ramp_at_limit:
             self.log.debug('read back ramp %g %r' % (workingramp, self._ramp_at_limit))
             self.ramp = workingramp
-        result = dict(setpoint=setpoint, workingramp=workingramp)
+        result = dict(setpoint=setpoint, workingramp=workingramp, approachmode=approachmode)
         self.log.debug('analyze_temp %r %r' % (result, (self.target, self.ramp)))
         return result
 

secop_psi/trinamic.py

@@ -24,13 +24,12 @@
 
 import time
 import struct
-from math import log10
 
 from secop.core import BoolType, Command, EnumType, FloatRange, IntRange, \
-    HasIodev, Parameter, Property, Drivable, PersistentMixin, PersistentParam
+    HasIodev, Parameter, Property, Drivable, PersistentMixin, PersistentParam, Done
 from secop.io import BytesIO
 from secop.errors import CommunicationFailedError, HardwareError, BadValueError, IsBusyError
-
+from secop.rwhandler import ReadHandler, WriteHandler
 
 MOTOR_STOP = 3
 MOVE = 4
@@ -52,26 +51,30 @@ MAX_ACCEL = 2047 * ACCEL_SCALE
 CURRENT_SCALE = 2.8/250
 ENCODER_RESOLUTION = 360 / 1024
 
+HW_ARGS = {
+    # <parameter name>: (address, scale factor)
+    'encoder_tolerance': (212, ANGLE_SCALE),
+    'speed': (4, SPEED_SCALE),
+    'minspeed': (130, SPEED_SCALE),
+    'currentspeed': (3, SPEED_SCALE),
+    'maxcurrent': (6, CURRENT_SCALE),
+    'standby_current': (7, CURRENT_SCALE,),
+    'acceleration': (5, ACCEL_SCALE),
+    'target_reached': (8, 1),
+    'move_status': (207, 1),
+    'error_bits': (208, 1),
+    'free_wheeling': (204, 0.01),
+    'power_down_delay': (214, 0.01),
+}
 
-class HwParam(PersistentParam):
+# special handling (adjust zero):
-    adr = Property('parameter address', IntRange(0, 255), export=False)
+ENCODER_ADR = 209
-    scale = Property('scale factor (physical value / unit)', FloatRange(), export=False)
+STEPPOS_ADR = 1
 
-    def __init__(self, description, datatype, adr, scale=1, poll=True,
-                 readonly=True, persistent=None, **kwds):
-        """hardware parameter"""
-        if persistent is None:
-            persistent = not readonly
-        if isinstance(datatype, FloatRange) and datatype.fmtstr == '%g':
-            datatype.fmtstr = '%%.%df' % max(0, 1 - int(log10(scale) + 0.01))
-        super().__init__(description, datatype, poll=poll, adr=adr, scale=scale,
-                         persistent=persistent, readonly=readonly, **kwds)
 
-    def copy(self):
+def writable(*args, **kwds):
-        res = HwParam(self.description, self.datatype.copy(), self.adr)
+    """convenience function to create writable hardware parameters"""
-        res.name = self.name
+    return PersistentParam(*args, readonly=False, poll=True, initwrite=True, **kwds)
-        res.init(self.propertyValues)
-        return res
 
 
 class Motor(PersistentMixin, HasIodev, Drivable):
@@ -79,41 +82,36 @@ class Motor(PersistentMixin, HasIodev, Drivable):
 
     value = Parameter('motor position', FloatRange(unit='deg', fmtstr='%.3f'))
     zero = PersistentParam('zero point', FloatRange(unit='$'), readonly=False, default=0)
-    encoder = HwParam('encoder reading', FloatRange(unit='$', fmtstr='%.1f'),
+    encoder = PersistentParam('encoder reading', FloatRange(unit='$', fmtstr='%.1f'),
-                      209, ANGLE_SCALE, readonly=True, initwrite=False, persistent=True)
+                              readonly=True, initwrite=False, poll=True)
-    steppos = HwParam('position from motor steps', FloatRange(unit='$'),
+    steppos = PersistentParam('position from motor steps', FloatRange(unit='$', fmtstr='%.3f'),
-                      1, ANGLE_SCALE, readonly=True, initwrite=False)
+                              readonly=True, initwrite=False, poll=True)
     target = Parameter('', FloatRange(unit='$'), default=0)
 
-    movelimit = Parameter('max. angle to drive in one go', FloatRange(unit='$'),
+    move_limit = Parameter('max. angle to drive in one go', FloatRange(unit='$'),
-                          readonly=False, default=360, group='more')
+                           readonly=False, default=360, group='more')
     tolerance = Parameter('positioning tolerance', FloatRange(unit='$'),
                           readonly=False, default=0.9)
-    encoder_tolerance = HwParam('the allowed deviation between steppos and encoder\n\nmust be > tolerance',
+    encoder_tolerance = writable('the allowed deviation between steppos and encoder\n\nmust be > tolerance',
-                                FloatRange(0, 360., unit='$'),
+                                 FloatRange(0, 360., unit='$', fmtstr='%.3f'), group='more')
-                                212, ANGLE_SCALE, readonly=False, group='more')
+    speed = writable('max. speed', FloatRange(0, MAX_SPEED, unit='$/sec', fmtstr='%.1f'), default=40)
-    speed = HwParam('max. speed', FloatRange(0, MAX_SPEED, unit='$/sec'),
+    minspeed = writable('min. speed', FloatRange(0, MAX_SPEED, unit='$/sec', fmtstr='%.1f'),
-                    4, SPEED_SCALE, readonly=False)
+                        default=SPEED_SCALE, group='motorparam')
-    minspeed = HwParam('min. speed', FloatRange(0, MAX_SPEED, unit='$/sec'),
+    currentspeed = Parameter('current speed', FloatRange(-MAX_SPEED, MAX_SPEED, unit='$/sec', fmtstr='%.1f'),
-                       130, SPEED_SCALE, readonly=False, default=SPEED_SCALE, group='motorparam')
+                             poll=True, group='motorparam')
-    currentspeed = HwParam('current speed', FloatRange(-MAX_SPEED, MAX_SPEED, unit='$/sec'),
+    maxcurrent = writable('', FloatRange(0, 2.8, unit='A', fmtstr='%.2f'),
-                           3, SPEED_SCALE, readonly=True, group='motorparam')
+                          default=1.4, group='motorparam')
-    maxcurrent = HwParam('', FloatRange(0, 2.8, unit='A'),
+    standby_current = writable('', FloatRange(0, 2.8, unit='A', fmtstr='%.2f'),
-                         6, CURRENT_SCALE, readonly=False, group='motorparam')
+                               default=0.1, group='motorparam')
-    standby_current = HwParam('', FloatRange(0, 2.8, unit='A'),
+    acceleration = writable('', FloatRange(4.6 * ACCEL_SCALE, MAX_ACCEL, unit='deg/s^2', fmtstr='%.1f'),
-                              7, CURRENT_SCALE, readonly=False, group='motorparam')
+                            default=150., group='motorparam')
-    acceleration = HwParam('', FloatRange(4.6 * ACCEL_SCALE, MAX_ACCEL, unit='deg/s^2'),
+    target_reached = Parameter('', BoolType(), poll=True, group='hwstatus')
-                           5, ACCEL_SCALE, readonly=False, group='motorparam')
+    move_status = Parameter('', IntRange(0, 3), poll=True, group='hwstatus')
-    target_reached = HwParam('', BoolType(), 8, group='hwstatus')
+    error_bits = Parameter('', IntRange(0, 255), poll=True, group='hwstatus')
-    move_status = HwParam('', IntRange(0, 3),
+    free_wheeling = writable('', FloatRange(0, 60., unit='sec', fmtstr='%.2f'),
-                          207, readonly=True, group='hwstatus')
+                             default=0.1, group='motorparam')
-    error_bits = HwParam('', IntRange(0, 255),
+    power_down_delay = writable('', FloatRange(0, 60., unit='sec', fmtstr='%.2f'),
-                         208, readonly=True, group='hwstatus')
+                                default=0.1, group='motorparam')
-    # the doc says msec, but I believe the scale is 10 msec
-    free_wheeling = HwParam('', FloatRange(0, 60., unit='sec'),
-                            204, 0.01, default=0.1, readonly=False, group='motorparam')
-    power_down_delay = HwParam('', FloatRange(0, 60., unit='sec'),
-                               214, 0.01, default=0.1, readonly=False, group='motorparam')
     baudrate = Parameter('', EnumType({'%d' % v: i for i, v in enumerate(BAUDRATES)}),
                          readonly=False, default=0, poll=True, visibility=3, group='more')
     pollinterval = Parameter(group='more')
@@ -133,7 +131,7 @@ class Motor(PersistentMixin, HasIodev, Drivable):
         :param value: if given, the parameter is written, else it is returned
         :return: the returned value
         """
-        if self._calcTimeout:
+        if self._calcTimeout and self._iodev._conn:
             self._calcTimeout = False
             baudrate = getattr(self._iodev._conn.connection, 'baudrate', None)
             if baudrate:
@@ -165,33 +163,19 @@ class Motor(PersistentMixin, HasIodev, Drivable):
             raise CommunicationFailedError('bad reply %r to command %s %d' % (reply, cmd, adr))
         return result
 
-    def get(self, pname):
-        """get parameter"""
-        pobj = self.parameters[pname]
-        value = self.comm(GET_AXIS_PAR, pobj.adr)
-        # do not apply scale when 1 (datatype might not be float)
-        return value if pobj.scale == 1 else value * pobj.scale
-
-    def set(self, pname, value, check=True):
-        """set parameter and check result"""
-        pobj = self.parameters[pname]
-        scale = pobj.scale
-        rawvalue = round(value / scale)
-        self.comm(SET_AXIS_PAR, pobj.adr, rawvalue)
-        if check:
-            result = self.comm(GET_AXIS_PAR, pobj.adr)
-            if result != rawvalue:
-                raise HardwareError('result does not match %d != %d' % (result, rawvalue))
-            value = result * scale
-        return value
-
     def startModule(self, start_events):
-        # get encoder value from motor. at this stage self.encoder contains the persistent value
-        encoder = self.get('encoder')
-        encoder += self.zero
-        self.fix_encoder(encoder)
         super().startModule(start_events)
 
+        def fix_encoder(self=self):
+            try:
+                # get encoder value from motor. at this stage self.encoder contains the persistent value
+                encoder = self._read_axispar(ENCODER_ADR, ANGLE_SCALE) + self.zero
+                self.fix_encoder(encoder)
+            except Exception as e:
+                self.log.error('fix_encoder failed with %r', e)
+
+        start_events.queue(fix_encoder)
+
     def fix_encoder(self, encoder_from_hw):
         """fix encoder value
 
@@ -204,14 +188,52 @@ class Motor(PersistentMixin, HasIodev, Drivable):
         # calculate nearest, most probable value
         adjusted_encoder = encoder_from_hw + round((self.encoder - encoder_from_hw) / 360.) * 360
         if abs(self.encoder - adjusted_encoder) >= self.encoder_tolerance:
-            # encoder module0 360 has changed
+            # encoder modulo 360 has changed
             self.log.error('saved encoder value (%.2f) does not match reading (%.2f %.2f)',
                            self.encoder, encoder_from_hw, adjusted_encoder)
             if adjusted_encoder != encoder_from_hw:
                 self.log.info('take next closest encoder value (%.2f)' % adjusted_encoder)
             self._need_reset = True
             self.status = self.Status.ERROR, 'saved encoder value does not match reading'
-        self.set('encoder', adjusted_encoder - self.zero, check=False)
+        self._write_axispar(adjusted_encoder - self.zero, ENCODER_ADR, ANGLE_SCALE, readback=False)
+
+    def _read_axispar(self, adr, scale=1):
+        value = self.comm(GET_AXIS_PAR, adr)
+        # do not apply scale when 1 (datatype might not be float)
+        return value if scale == 1 else value * scale
+
+    def _write_axispar(self, value, adr, scale=1, readback=True):
+        rawvalue = round(value / scale)
+        self.comm(SET_AXIS_PAR, adr, rawvalue)
+        if readback:
+            result = self.comm(GET_AXIS_PAR, adr)
+            if result != rawvalue:
+                raise HardwareError('result for adr=%d scale=%g does not match %g != %g'
+                                    % (adr, scale, result * scale, value))
+            return result * scale
+        return rawvalue * scale
+
+    @ReadHandler(HW_ARGS)
+    def read_hwparam(self, pname):
+        """handle read for HwParam"""
+        args = HW_ARGS[pname]
+        reply = self._read_axispar(*args)
+        try:
+            value = getattr(self, pname)
+        except Exception:
+            return reply
+        if reply != value:
+            if not self.parameters[pname].readonly:
+                # this should not happen
+                self.log.warning('hw parameter %s has changed from %r to %r, write again', pname, value, reply)
+                self._write_axispar(value, *args, readback=False)
+                reply = self._read_axispar(*args)
+        return reply
+
+    @WriteHandler(HW_ARGS)
+    def write_hwparam(self, pname, value):
+        """handler write for HwParam"""
+        return self._write_axispar(value, *HW_ARGS[pname])
 
     def read_value(self):
         encoder = self.read_encoder()
@@ -233,7 +255,7 @@ class Motor(PersistentMixin, HasIodev, Drivable):
                 self._need_reset = True
                 self.status = self.Status.ERROR, 'power loss'
                 # or should we just fix instead of error status?
-                # self.set('steppos', self.steppos - self.zero, check=False)
+                # self._write_axispar(self.steppos - self.zero, readback=False)
             self.comm(SET_GLOB_PAR, 255, 1, bank=2)  # set initialized flag
             self._started = 0
 
@@ -252,6 +274,7 @@ class Motor(PersistentMixin, HasIodev, Drivable):
         if oldpos != self.steppos or not (self.read_target_reached() or self.read_move_status()
                                           or self.read_error_bits()):
             return self.Status.BUSY, 'moving'
+        # TODO: handle the different errors from move_status and error_bits
         diff = self.target - self.encoder
         if abs(diff) <= self.tolerance:
             self._started = 0
@@ -262,8 +285,8 @@ class Motor(PersistentMixin, HasIodev, Drivable):
 
     def write_target(self, target):
         self.read_value()  # make sure encoder and steppos are fresh
-        if abs(target - self.encoder) > self.movelimit:
+        if abs(target - self.encoder) > self.move_limit:
-            raise BadValueError('can not move more than %s deg' % self.movelimit)
+            raise BadValueError('can not move more than %s deg' % self.move_limit)
         diff = self.encoder - self.steppos
         if self._need_reset:
             raise HardwareError('need reset (%s)' % self.status[1])
@@ -272,7 +295,7 @@ class Motor(PersistentMixin, HasIodev, Drivable):
                 self._need_reset = True
                 self.status = self.Status.ERROR, 'encoder does not match internal pos'
                 raise HardwareError('need reset (encoder does not match internal pos)')
-            self.set('steppos', self.encoder - self.zero)
+            self._write_axispar(self.encoder - self.zero, STEPPOS_ADR, ANGLE_SCALE)
         self._started = time.time()
         self.log.info('move to %.1f', target)
         self.comm(MOVE, 0, (target - self.zero) / ANGLE_SCALE)
@@ -280,80 +303,19 @@ class Motor(PersistentMixin, HasIodev, Drivable):
         return target
 
     def write_zero(self, value):
-        diff = value - self.zero
+        self.zero = value
-        self.encoder += diff
+        self.read_value()  # apply zero to encoder, steppos and value
-        self.steppos += diff
+        return Done
-        self.value += diff
-        return value
 
     def read_encoder(self):
-        return self.get('encoder') + self.zero
+        return self._read_axispar(ENCODER_ADR, ANGLE_SCALE) + self.zero
 
     def read_steppos(self):
-        return self.get('steppos') + self.zero
+        return self._read_axispar(STEPPOS_ADR, ANGLE_SCALE) + self.zero
-
-    def read_encoder_tolerance(self):
-        return self.get('encoder_tolerance')
-
-    def write_encoder_tolerance(self, value):
-        return self.set('encoder_tolerance', value)
-
-    def read_target_reached(self):
-        return self.get('target_reached')
-
-    def read_speed(self):
-        return self.get('speed')
-
-    def write_speed(self, value):
-        return self.set('speed', value)
-
-    def read_minspeed(self):
-        return self.get('minspeed')
-
-    def write_minspeed(self, value):
-        return self.set('minspeed', value)
-
-    def read_currentspeed(self):
-        return self.get('currentspeed')
-
-    def read_acceleration(self):
-        return self.get('acceleration')
-
-    def write_acceleration(self, value):
-        return self.set('acceleration', value)
-
-    def read_maxcurrent(self):
-        return self.get('maxcurrent')
-
-    def write_maxcurrent(self, value):
-        return self.set('maxcurrent', value)
-
-    def read_standby_current(self):
-        return self.get('standby_current')
-
-    def write_standby_current(self, value):
-        return self.set('standby_current', value)
-
-    def read_free_wheeling(self):
-        return self.get('free_wheeling')
-
-    def write_free_wheeling(self, value):
-        return self.set('free_wheeling', value)
-
-    def read_power_down_delay(self):
-        return self.get('power_down_delay')
-
-    def write_power_down_delay(self, value):
-        return self.set('power_down_delay', value)
-
-    def read_move_status(self):
-        return self.get('move_status')
-
-    def read_error_bits(self):
-        return self.get('error_bits')
 
     @Command(FloatRange())
     def set_zero(self, value):
+        """adjust zero"""
         self.write_zero(value - self.read_value())
 
     def read_baudrate(self):
@@ -374,7 +336,7 @@ class Motor(PersistentMixin, HasIodev, Drivable):
                 self._need_reset = False
                 self.status = self.Status.IDLE, 'ok'
                 return
-            self.set('steppos', self.encoder - self.zero, check=False)
+            self._write_axispar(self.encoder - self.zero, STEPPOS_ADR, ANGLE_SCALE, readback=False)
             self.comm(MOVE, 0, (self.encoder - self.zero) / ANGLE_SCALE)
             time.sleep(0.1)
             if itry > 5:
@@ -410,7 +372,7 @@ class Motor(PersistentMixin, HasIodev, Drivable):
         """get arbitrary motor parameter"""
         return self.comm(GET_AXIS_PAR, adr)
 
-    @Command((IntRange(), FloatRange()), result=IntRange())
+    @Command((IntRange(), IntRange()), result=IntRange())
     def set_axis_par(self, adr, value):
         """set arbitrary motor parameter"""
         return self.comm(SET_AXIS_PAR, adr, value)

test/test_modules.py

@@ -30,6 +30,7 @@ from secop.modules import Communicator, Drivable, Readable, Module
 from secop.params import Command, Parameter
 from secop.poller import BasicPoller
 from secop.lib.multievent import MultiEvent
+from secop.rwhandler import ReadHandler, WriteHandler
 
 
 class DispatcherStub:
@@ -51,8 +52,8 @@ class DispatcherStub:
 
 
 class LoggerStub:
-    def debug(self, *args):
+    def debug(self, fmt, *args):
-        print(*args)
+        print(fmt % args)
     info = warning = exception = debug
     handlers = []
 
@@ -110,6 +111,9 @@ def test_ModuleMagic():
         def read_value(self):
             return 'second'
 
+        def read_status(self):
+            return 'IDLE', 'ok'
+
     with pytest.raises(ProgrammingError):
         class Mod1(Module):  # pylint: disable=unused-variable
             def do_this(self):  # old style command
@@ -179,7 +183,7 @@ def test_ModuleMagic():
     o1.startModule(event)
     event.wait()
     # should contain polled values
-    expectedAfterStart = {'status': (Drivable.Status.IDLE, ''),
+    expectedAfterStart = {'status': (Drivable.Status.IDLE, 'ok'),
             'value': 'second'}
     assert updates.pop('o1') == expectedAfterStart
 
@@ -479,3 +483,95 @@ def test_bad_method():
         class Mod3(Drivable):  # pylint: disable=unused-variable
             def read_valu(self, value):
                 pass
+
+
+def test_generic_access():
+    class Mod(Module):
+        param = Parameter('handled param', StringType(), readonly=False)
+        unhandled = Parameter('unhandled param', StringType(), default='', readonly=False)
+        data = {'param': ''}
+
+        @ReadHandler(['param'])
+        def read_handler(self, pname):
+            value = self.data[pname]
+            setattr(self, pname, value)
+            return value
+
+        @WriteHandler(['param'])
+        def write_handler(self, pname, value):
+            value = value.lower()
+            self.data[pname] = value
+            setattr(self, pname, value)
+            return value
+
+    updates = {}
+    srv = ServerStub(updates)
+
+    obj = Mod('obj', logger, {'description': '', 'param': 'initial value'}, srv)
+    assert obj.param == 'initial value'
+    assert obj.write_param('Cheese') == 'cheese'
+    assert obj.write_unhandled('Cheese') == 'Cheese'
+    assert updates == {'obj': {'param': 'cheese', 'unhandled': 'Cheese'}}
+    updates.clear()
+    assert obj.write_param('Potato') == 'potato'
+    assert updates == {'obj': {'param': 'potato'}}
+    updates.clear()
+    assert obj.read_param() == 'potato'
+    assert obj.read_unhandled()
+    assert updates == {'obj': {'param': 'potato'}}
+    updates.clear()
+    assert updates == {}
+
+
+def test_duplicate_handler_name():
+    with pytest.raises(ProgrammingError):
+        class Mod(Module):  # pylint: disable=unused-variable
+            param = Parameter('handled param', StringType(), readonly=False)
+
+            @ReadHandler(['param'])
+            def handler(self, pname):
+                pass
+
+            @WriteHandler(['param'])
+            def handler(self, pname, value):  # pylint: disable=function-redefined
+                pass
+
+
+def test_handler_overwrites_method():
+    with pytest.raises(RuntimeError):
+        class Mod1(Module):  # pylint: disable=unused-variable
+            param = Parameter('handled param', StringType(), readonly=False)
+
+            @ReadHandler(['param'])
+            def read_handler(self, pname):
+                pass
+
+            def read_param(self):
+                pass
+
+    with pytest.raises(RuntimeError):
+        class Mod2(Module):  # pylint: disable=unused-variable
+            param = Parameter('handled param', StringType(), readonly=False)
+
+            @WriteHandler(['param'])
+            def write_handler(self, pname, value):
+                pass
+
+            def write_param(self, value):
+                pass
+
+
+def test_no_read_write():
+    class Mod(Module):
+        param = Parameter('test param', StringType(), readonly=False)
+
+    updates = {}
+    srv = ServerStub(updates)
+
+    obj = Mod('obj', logger, {'description': '', 'param': 'cheese'}, srv)
+    assert obj.param == 'cheese'
+    assert obj.read_param() == 'cheese'
+    assert updates == {'obj': {'param': 'cheese'}}
+    assert obj.write_param('egg') == 'egg'
+    assert obj.param == 'egg'
+    assert updates == {'obj': {'param': 'egg'}}