improve persistent parameters

A value given in config overrides values read from the persistent data file.
To let the loaded parameter have precedence, configure a default only.
The write_<param> method of a persistent parameter is now always called
on startup.

- add tests for persistent behaviour
+ simplify Modules.writeInitParams: remove started_callback argument

Change-Id: I08b49de52e9d9a2ed0918018eb2fe538141a4f5e

frappy/modules.py

@@ -420,6 +420,7 @@ class Module(HasAccessibles):
                 pobj.value = pobj.default
             else:
                 # value given explicitly, either by cfg or as Parameter argument
+                pobj.given = True  # for PersistentMixin
                 if hasattr(self, 'write_' + pname):
                     self.writeDict[pname] = pobj.value
                 if pobj.default is None:
@@ -706,11 +707,12 @@ class Module(HasAccessibles):
                     else:
                         loop = False  # no slow polls ready
 
-    def writeInitParams(self, started_callback=None):
+    def writeInitParams(self):
         """write values for parameters with configured values
 
-        this must be called at the beginning of the poller thread
+        - does proper error handling
-        with proper error handling
+
+        called at the beginning of the poller thread and for writing persistent values
         """
         for pname in list(self.writeDict):
             value = self.writeDict.pop(pname, Done)
@@ -730,8 +732,6 @@ class Module(HasAccessibles):
                             self.log.error('%s: %s', pname, str(e))
                     except Exception:
                         self.log.error(formatException())
-        if started_callback:
-            started_callback()
 
     def setRemoteLogging(self, conn, level):
         if self.remoteLogHandler is None:

frappy/persistent.py

@@ -21,7 +21,7 @@
 # *****************************************************************************
 """Mixin for keeping parameters persistent
 
-For hardware not keeping parameters persistent, we might want to store them in Frappy.
+For hardware not keeping parameters persistent, we might want to store them in a file.
 
 The following example will make 'param1' and 'param2' persistent, i.e. whenever
 one of the parameters is changed, either by a change command or when reading back
@@ -58,66 +58,79 @@ import json
 from frappy.lib import generalConfig
 from frappy.datatypes import EnumType
 from frappy.params import Parameter, Property, Command
-from frappy.modules import HasAccessibles
+from frappy.modules import Module
 
 
 class PersistentParam(Parameter):
     persistent = Property('persistence flag (auto means: save automatically on any change)',
                           EnumType(off=0, on=1, auto=2), default=1)
+    given = False
 
 
-class PersistentMixin(HasAccessibles):
+class PersistentMixin(Module):
-    def __init__(self, *args, **kwds):
+    persistentData = None  # dict containing persistent data after startup
-        super().__init__(*args, **kwds)
+
+    def __init__(self, name, logger, cfgdict, srv):
+        super().__init__(name, logger, cfgdict, srv)
         persistentdir = os.path.join(generalConfig.logdir, 'persistent')
         os.makedirs(persistentdir, exist_ok=True)
         self.persistentFile = os.path.join(persistentdir, '%s.%s.json' % (self.DISPATCHER.equipment_id, self.name))
         self.initData = {}  # "factory" settings
+        loaded = self.loadPersistentData()
         for pname in self.parameters:
             pobj = self.parameters[pname]
-            flag = getattr(pobj, 'persistent', 0)
+            flag = getattr(pobj, 'persistent', False)
             if flag:
                 if flag == 'auto':
                     def cb(value, m=self):
                         m.saveParameters()
                     self.valueCallbacks[pname].append(cb)
                 self.initData[pname] = pobj.value
-        self.writeDict.update(self.loadParameters(write=False))
+                if not pobj.given:
+                    if pname in loaded:
+                        pobj.value = loaded[pname]
+                    if hasattr(self, 'write_' + pname):
+                        # a persistent parameter should be written to HW, even when not yet in persistentData
+                        self.writeDict[pname] = pobj.value
+        self.__save_params()
 
-    def loadParameters(self, write=True):
+    def loadPersistentData(self):
-        """load persistent parameters
-
-        :return: persistent parameters which have to be written
-
-        is called upon startup and may be called from a module
-        when a hardware powerdown is detected
-        """
         try:
             with open(self.persistentFile, 'r', encoding='utf-8') as f:
                 self.persistentData = json.load(f)
-        except Exception:
+        except (FileNotFoundError, ValueError):
             self.persistentData = {}
-        writeDict = {}
+        result = {}
-        for pname in self.parameters:
+        for pname, value in self.persistentData.items():
-            pobj = self.parameters[pname]
-            if getattr(pobj, 'persistent', False) and pname in self.persistentData:
             try:
-                    value = pobj.datatype.import_value(self.persistentData[pname])
+                pobj = self.parameters[pname]
+                if getattr(pobj, 'persistent', False):
+                    result[pname] = self.parameters[pname].datatype.import_value(value)
+            except Exception as e:
+                # ignore invalid persistent data (in case parameters have changed)
+                self.log.warning('can not restore %r to %r (%r)' % (pname, value, e))
+        return result
+
+    def loadParameters(self):
+        """load persistent parameters
+
+        and write them to the HW, in case a write_<param> method is available
+        may be called from a module when a hardware power down is detected
+        """
+        loaded = self.loadPersistentData()
+        for pname, value in loaded.items():
+            pobj = self.parameters[pname]
             pobj.value = value
             pobj.readerror = None
-                    if not pobj.readonly:
+            if hasattr(self, 'write_' + pname):
-                        writeDict[pname] = value
+                self.writeDict[pname] = value
-                except Exception as e:
-                    self.log.warning('can not restore %r to %r (%r)' % (pname, value, e))
-        if write:
-            self.writeDict.update(writeDict)
         self.writeInitParams()
-        return writeDict
+        return loaded
 
     def saveParameters(self):
         """save persistent parameters
 
-        - to be called regularely explicitly by the module
+        - to be called regularly explicitly by the module
         - the caller has to make sure that this is not called after
           a power down of the connected hardware before loadParameters
         """
@@ -125,6 +138,9 @@ class PersistentMixin(HasAccessibles):
             # do not save before all values are written to the hw, as potentially
             # factory default values were read in the mean time
             return
+        self.__save_params()
+
+    def __save_params(self):
         data = {k: v.export_value() for k, v in self.parameters.items()
                 if getattr(v, 'persistent', False)}
         if data != self.persistentData:

frappy_psi/trinamic.py

@@ -270,7 +270,7 @@ class Motor(PersistentMixin, HasIO, Drivable):
         elif not self._loading:  # just powered up
             try:
                 self._loading = True
-                # get persistent values
+                # get persistent values and write to HW
                 writeDict = self.loadParameters()
             finally:
                 self._loading = False

test/test_persistent.py

@@ -0,0 +1,112 @@
+#  -*- 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>
+#
+# *****************************************************************************
+
+import json
+import os
+from os.path import join
+import pytest
+from frappy.config import Param
+from frappy.core import Module, ScaledInteger, IntRange, StringType, StructOf
+from frappy.lib import generalConfig
+from frappy.persistent import PersistentParam, PersistentMixin
+
+
+class DispatcherStub:
+    def announce_update(self, modulename, pname, pobj):
+        pass
+
+
+class LoggerStub:
+    def debug(self, fmt, *args):
+        print(fmt % args)
+    info = warning = exception = error = debug
+    handlers = []
+
+
+logger = LoggerStub()
+
+
+class ServerStub:
+    def __init__(self, equipment_id):
+        self.dispatcher = DispatcherStub()
+        self.dispatcher.equipment_id = equipment_id
+
+
+class Mod(PersistentMixin, Module):
+    flt = PersistentParam('', ScaledInteger(0.1), default=1.0)
+    stc = PersistentParam('', StructOf(i=IntRange(0, 10), s=StringType()))
+
+    def write_flt(self, value):
+        return value
+
+    def write_stc(self, value):
+        return value
+
+
+save_tests = [
+    ({'flt': Param(5.5), 'stc': Param({'i': 3, 's': 'test'})},
+     {'flt': 55, 'stc': {'i': 3, 's': 'test'}}),
+    ({'flt': Param(5.5)},
+     {'flt': 55, 'stc': {'i': 0, 's': ''}}),  # saved default values
+]
+@pytest.mark.parametrize('cfg, data', save_tests)
+def test_save(tmpdir, cfg, data):
+    generalConfig.logdir = tmpdir
+
+    cfg['description'] = ''
+    m = Mod('m', logger, cfg, ServerStub('savetest'))
+    assert m.writeDict == {k: getattr(m, k) for k in data}
+    m.writeDict.clear()  # clear in order to indicate writing has happened
+    m.saveParameters()
+    with open(join(tmpdir, 'persistent', 'savetest.m.json'), encoding='utf-8') as f:
+        assert json.load(f) == data
+
+
+load_tests = [
+    # check that value from cfg is overriding value from persistent file
+    ({'flt': Param(5.5), 'stc': Param({'i': 3, 's': 'test'})},
+     {'flt': 33, 'stc': {'i': 1, 's': 'bar'}},
+     {'flt': 5.5, 'stc': {'i': 3, 's': 'test'}}),
+    # check that value from file is taken when not in cfg
+    ({'flt': Param(3.5)},
+     {'flt': 35, 'stc': {'i': 2, 's': ''}},
+     {'flt': 3.5, 'stc': {'i': 2, 's': ''}}),
+    # check default is written when neither cfg is given nor persistent values present
+    ({},
+     {},
+     {'flt': 1.0, 'stc': {'i': 0, 's': ''}}),
+]
+@pytest.mark.parametrize('cfg, data, written', load_tests)
+def test_load(tmpdir, cfg, data, written):
+    generalConfig.logdir = tmpdir
+
+    os.makedirs(join(tmpdir, 'persistent'), exist_ok=True)
+    with open(join(tmpdir, 'persistent', 'loadtest.m.json'), 'w', encoding='utf-8') as f:
+        json.dump(data, f)
+    cfg['description'] = ''
+    m = Mod('m', logger, cfg, ServerStub('loadtest'))
+    assert m.writeDict == written
+    # parameter given in config must override values from file
+    for k, v in cfg.items():
+        assert getattr(m, k) == v['value']
+    for k, v in written.items():
+        assert getattr(m, k) == v