From f4e974f46c241d8cba22f81e42c3ee5320aa248b Mon Sep 17 00:00:00 2001 From: Markus Zolliker Date: Thu, 19 Jan 2023 11:03:04 +0100 Subject: [PATCH] improve parameter initialisation - make 'value' a Parameter property instead of an attribute - use 'value' instead of 'default' property for setting the initial value in the config file - removal of initwrite parameter property this change is the basis of a better implementation for change 30041 (PersistentParam property 'override_cfg') Change-Id: I2b82bdd54c2dacb87dcd2b3472004d2f0a730cf0 --- frappy/config.py | 8 ++-- frappy/features.py | 2 +- frappy/modules.py | 86 +++++++++++++----------------------------- frappy/params.py | 42 ++++++++++++++------- frappy/server.py | 3 -- frappy_psi/trinamic.py | 8 ++-- test/test_handler.py | 2 +- test/test_modules.py | 32 ++++++++-------- test/test_poller.py | 1 + 9 files changed, 82 insertions(+), 102 deletions(-) diff --git a/frappy/config.py b/frappy/config.py index 64022bc..1618a4d 100644 --- a/frappy/config.py +++ b/frappy/config.py @@ -47,9 +47,9 @@ class Node(dict): ) class Param(dict): - def __init__(self, default=Undef, **kwds): - if default is not Undef: - kwds['default'] = default + def __init__(self, value=Undef, **kwds): + if value is not Undef: + kwds['value'] = value super().__init__(**kwds) class Group(tuple): @@ -71,7 +71,7 @@ class Mod(dict): elif isinstance(val, Group): groups[key] = val else: - # shortcut to only set default + # shortcut to only set value self[key] = Param(val) for group, members in groups.items(): for member in members: diff --git a/frappy/features.py b/frappy/features.py index 3588bb8..1ff8878 100644 --- a/frappy/features.py +++ b/frappy/features.py @@ -63,7 +63,7 @@ class HasLimits(Feature): """ abslimits = Property('abs limits (raw values)', default=(-9e99, 9e99), extname='abslimits', export=True, datatype=TupleOf(FloatRange(unit='deg'), FloatRange(unit='deg'))) - limits = PersistentParam('user limits', readonly=False, default=(-9e99, 9e99), initwrite=True, + limits = PersistentParam('user limits', readonly=False, default=(-9e99, 9e99), datatype=TupleOf(FloatRange(unit='deg'), FloatRange(unit='deg'))) _limits = None diff --git a/frappy/modules.py b/frappy/modules.py index 4a0f36f..09360c2 100644 --- a/frappy/modules.py +++ b/frappy/modules.py @@ -328,7 +328,7 @@ class Module(HasAccessibles): if value is not None: try: if isinstance(value, dict): - self.setProperty(key, value['default']) + self.setProperty(key, value['value']) else: self.setProperty(key, value) except BadValueError: @@ -372,14 +372,12 @@ class Module(HasAccessibles): self.commands = {k: v for k, v in accessibles.items() if isinstance(v, Command)} # 2) check and apply parameter_properties - for aname in list(cfgdict): # keep list() as dict may change during iter + bad = [] + for aname, cfg in cfgdict.items(): aobj = self.accessibles.get(aname, None) if aobj: try: - for propname, propvalue in cfgdict[aname].items(): - # defaults are applied later - if propname == 'default': - continue + for propname, propvalue in cfg.items(): aobj.setProperty(propname, propvalue) except KeyError: errors.append("'%s' has no property '%s'" % @@ -388,23 +386,17 @@ class Module(HasAccessibles): errors.append('%s.%s: %s' % (aname, propname, str(e))) else: - errors.append('%r not found' % aname) - # 3) commands do not need a default, remove them from cfgdict: - for aname in list(cfgdict): - if aname in self.commands: - cfgdict.pop(aname) + bad.append(aname) - # 3) check config for problems: - # only accept remaining config items specified in parameters - bad = [k for k in cfgdict if k not in self.parameters] + # 3) complain about names not found as accessible or property names if bad: errors.append( '%s does not exist (use one of %s)' % - (', '.join(bad), ', '.join(list(self.parameters) + - list(self.propertyDict)))) - - # 4) complain if a Parameter entry has no default value and - # is not specified in cfgdict and deal with parameters to be written. + (', '.join(bad), ', '.join(list(self.accessibles) + + list(self.propertyDict)))) + # 4) register value for writing, if given + # apply default when no value is given (in cfg or as Parameter argument) + # or complain, when cfg is needed self.writeDict = {} # values of parameters to be written for pname, pobj in self.parameters.items(): self.valueCallbacks[pname] = [] @@ -414,54 +406,28 @@ class Module(HasAccessibles): errors.append('%s needs a datatype' % pname) continue - if pname in cfgdict and 'default' in cfgdict[pname]: - if pobj.initwrite is not False and hasattr(self, 'write_' + pname): - # parameters given in cfgdict have to call write_ - try: - pobj.value = pobj.datatype(cfgdict[pname]['default']) - self.writeDict[pname] = pobj.value - except BadValueError as e: - errors.append('%s: %s' % (pname, e)) - else: + if pobj.value is None: + if pobj.needscfg: + errors.append('%r has no default ' + 'value and was not given in config!' % pname) if pobj.default is None: - if pobj.needscfg: - errors.append('%r has no default ' - '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('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) - else: - try: - value = pobj.datatype(pobj.default) - except BadValueError as e: - # this should not happen, as the default is already checked in Parameter - raise ProgrammingError('bad default for %s:%s: %s' % (name, pname, e)) from None - if pobj.initwrite and hasattr(self, 'write_' + pname): - # we will need to call write_ - pobj.value = value - self.writeDict[pname] = value - else: - # dict to fit in with parameters coming from config - cfgdict[pname] = { 'default' : value } + pobj.default = pobj.datatype.default + pobj.value = pobj.default + else: + # value given explicitly, either by cfg or as Parameter argument + if hasattr(self, 'write_' + pname): + self.writeDict[pname] = pobj.value + if pobj.default is None: + pobj.default = pobj.value + # this checks again for datatype and sets the timestamp + setattr(self, pname, pobj.value) - # 5) 'apply' config: - # pass values through the datatypes and store as attributes - for k, v in list(cfgdict.items()): - try: - # this checks also for the proper datatype - # note: this will NOT call write_* methods! - if k in self.parameters or k in self.propertyDict: - if 'default' in v: - setattr(self, k, v['default']) - cfgdict.pop(k) - except (ValueError, TypeError) as e: - # self.log.exception(formatExtendedStack()) - errors.append('parameter %s: %s' % (k, e)) - - # ensure consistency + # 5) ensure consistency for aobj in self.accessibles.values(): aobj.finish() diff --git a/frappy/params.py b/frappy/params.py index 08a7e15..7fa36d8 100644 --- a/frappy/params.py +++ b/frappy/params.py @@ -102,6 +102,16 @@ class Parameter(Accessible): :param datatype: the datatype :param inherit: whether properties not given should be inherited :param kwds: optional properties + + Usage of 'value' and 'default': + - if a value is given for a parameter in the config file, and if the write_ + method is given, it is called on startup with this value as argument + - if a value should be written to the HW on startup, even when not given in the config + add the value argument to the Parameter definition + - for parameters which are not polling the HW, either a default should be given + as a Parameter argument, or, when needscfg is set to True, a configured value is required + - when default instead of value is given in the cfg file, it is assigne to the parameter + but not written to the HW """ # storage for Parameter settings + value + qualifiers @@ -128,6 +138,11 @@ class Parameter(Accessible): if it can not be read from the hardware''', ValueType(), export=False, default=None) + value = Property( + '''[internal] configured value of this parameter + + if given, write to the hardware''', ValueType(), + export=False, default=None) export = Property( '''[internal] export settings @@ -141,14 +156,9 @@ class Parameter(Accessible): optional = Property( '[internal] is this parameter optional?', BoolType(), export=False, settable=False, default=False) - initwrite = Property( - '''[internal] write this parameter on initialization - - default None: write if given in config''', NoneOr(BoolType()), - export=False, default=None, settable=False) # used on the instance copy only - value = None + # value = None timestamp = 0 readerror = None @@ -171,6 +181,8 @@ class Parameter(Accessible): self.datatype = datatype if 'default' in kwds: self.default = datatype(kwds['default']) + if 'value' in kwds: + self.value = datatype(kwds['value']) if description is not None: kwds['description'] = inspect.cleandoc(description) @@ -228,7 +240,7 @@ class Parameter(Accessible): def override(self, value): """override default""" - self.default = self.datatype(value) + self.value = self.datatype(value) def merge(self, merged_properties): """merge with inherited properties @@ -251,13 +263,15 @@ class Parameter(Accessible): # serialised version of the constant, or unset self.constant = self.datatype.export_value(constant) self.readonly = True - if 'default' in self.propertyValues: - # fixes in case datatype has changed - try: - self.default = self.datatype(self.default) - except BadValueError: - # clear default, if it does not match datatype - self.propertyValues.pop('default') + for propname in 'default', 'value': + if propname in self.propertyValues: + value = self.propertyValues.pop(propname) + # fixes in case datatype has changed + try: + self.propertyValues[propname] = self.datatype(value) + except BadValueError: + # clear, if it does not match datatype + pass def export_value(self): return self.datatype.export_value(self.value) diff --git a/frappy/server.py b/frappy/server.py index e05801b..a3fd319 100644 --- a/frappy/server.py +++ b/frappy/server.py @@ -198,9 +198,6 @@ class Server: else: try: modobj = cls(modname, self.log.getChild(modname), opts, self) - # all used args should be popped from opts! - if opts: - errors.append(self.unknown_options(cls, opts)) self.modules[modname] = modobj except ConfigError as e: errors.append('error creating module %s:' % modname) diff --git a/frappy_psi/trinamic.py b/frappy_psi/trinamic.py index 28d9254..f51d1a6 100644 --- a/frappy_psi/trinamic.py +++ b/frappy_psi/trinamic.py @@ -78,7 +78,7 @@ STEPPOS_ADR = 1 def writable(*args, **kwds): """convenience function to create writable hardware parameters""" - return PersistentParam(*args, readonly=False, initwrite=True, **kwds) + return PersistentParam(*args, readonly=False, **kwds) # TODO: check impact of 'initwrite' removal class Motor(PersistentMixin, HasIO, Drivable): @@ -88,9 +88,9 @@ class Motor(PersistentMixin, HasIO, Drivable): needscfg=False) zero = PersistentParam('zero point', FloatRange(unit='$'), readonly=False, default=0) encoder = PersistentParam('encoder reading', FloatRange(unit='$', fmtstr='%.1f'), - readonly=True, initwrite=False) + readonly=True) steppos = PersistentParam('position from motor steps', FloatRange(unit='$', fmtstr='%.3f'), - readonly=True, initwrite=False) + readonly=True) target = Parameter('', FloatRange(unit='$'), default=0) move_limit = Parameter('max. angle to drive in one go when current above safe_current', @@ -121,7 +121,7 @@ class Motor(PersistentMixin, HasIO, Drivable): error_bits = Parameter('', IntRange(0, 255), group='hwstatus') home = Parameter('state of home switch (input 3)', BoolType()) has_home = Parameter('enable home and activate pullup resistor', BoolType(), - default=True, initwrite=True, group='more') + default=True, group='more') auto_reset = Parameter('automatic reset after failure', BoolType(), readonly=False, default=False) free_wheeling = writable('', FloatRange(0, 60., unit='sec', fmtstr='%.2f'), default=0.1, group='motorparam') diff --git a/test/test_handler.py b/test/test_handler.py index 8879bba..7ed9770 100644 --- a/test/test_handler.py +++ b/test/test_handler.py @@ -62,7 +62,7 @@ class ServerStub: class ModuleTest(Module): def __init__(self, updates=None, **opts): opts['description'] = '' - opts = {p: {'default': val} for p, val in opts.items()} + opts = {p: {'value': val} for p, val in opts.items()} super().__init__('mod', logger, opts, ServerStub(updates or {})) diff --git a/test/test_modules.py b/test/test_modules.py index 404dd8a..6864c4e 100644 --- a/test/test_modules.py +++ b/test/test_modules.py @@ -150,8 +150,8 @@ def test_ModuleMagic(): a1 = Parameter(datatype=FloatRange(unit='$/s'), readonly=False) # remark: it might be a programming error to override the datatype # and not overriding the read_* method. This is not checked! - b2 = Parameter('', datatype=StringType(), default='empty', - readonly=False, initwrite=True) + b2 = Parameter('', datatype=StringType(), value='empty', + readonly=False) def write_a1(self, value): self._a1_written = value @@ -186,12 +186,14 @@ def test_ModuleMagic(): params_found.add(o) assert list(obj.accessibles) == sortcheck + updates.clear() # check for inital updates working properly o1 = Newclass1('o1', logger, {'description':''}, srv) expectedBeforeStart = {'target': '', 'status': (Drivable.Status.IDLE, ''), - 'param1': False, 'param2': 1.0, 'a1': 0.0, 'a2': True, 'pollinterval': 5.0, + 'param1': False, 'param2': 1.0, 'a1': False, 'a2': True, 'pollinterval': 5.0, 'value': 'first'} - assert updates.pop('o1') == expectedBeforeStart + for k, v in expectedBeforeStart.items(): + assert getattr(o1, k) == v o1.earlyInit() event = DummyMultiEvent() o1.initModule() @@ -204,11 +206,11 @@ def test_ModuleMagic(): assert updates.pop('o1') == expectedAfterStart # check in addition if parameters are written - o2 = Newclass2('o2', logger, {'description':'', 'a1': {'default': 2.7}}, srv) - # no update for b2, as this has to be written - expectedBeforeStart['a1'] = 2.7 - expectedBeforeStart['target'] = 0.0 - assert updates.pop('o2') == expectedBeforeStart + assert not updates + o2 = Newclass2('o2', logger, {'description':'', 'a1': {'value': 2.7}}, srv) + expectedBeforeStart.update(a1=2.7, b2='empty', target=0, value=0) + for k, v in expectedBeforeStart.items(): + assert getattr(o2, k) == v o2.earlyInit() event = DummyMultiEvent() o2.initModule() @@ -240,7 +242,7 @@ def test_ModuleMagic(): 'cmd2', 'value', 'a1'} assert set(cfg['value'].keys()) == { 'group', 'export', 'relative_resolution', - 'visibility', 'unit', 'default', 'datatype', 'fmtstr', + 'visibility', 'unit', 'default', 'value', 'datatype', 'fmtstr', 'absolute_resolution', 'max', 'min', 'readonly', 'constant', 'description', 'needscfg'} @@ -412,14 +414,14 @@ def test_mixin(): MixedDrivable('o', logger, { 'description': '', - 'param1': {'default': 0, 'description': 'param1'}, + 'param1': {'value': 0, 'description': 'param1'}, 'param2': {'datatype': {"type": "double"}}, }, srv) with pytest.raises(ConfigError): MixedReadable('o', logger, { 'description': '', - 'param1': {'default': 0, 'description': 'param1'}, + 'param1': {'value': 0, 'description': 'param1'}, 'param2': {'datatype': {"type": "double"}}, }, srv) @@ -431,7 +433,7 @@ def test_override(): def stop(self): """no decorator needed""" - assert Mod.value.default == 5 + assert Mod.value.value == 5 assert Mod.stop.description == "no decorator needed" class Mod2(Drivable): @@ -526,7 +528,7 @@ def test_generic_access(): updates = {} srv = ServerStub(updates) - obj = Mod('obj', logger, {'description': '', 'param': {'default':'initial value'}}, srv) + obj = Mod('obj', logger, {'description': '', 'param': {'value':'initial value'}}, srv) assert obj.param == 'initial value' assert obj.write_param('Cheese') == 'cheese' assert obj.write_unhandled('Cheese') == 'Cheese' @@ -587,7 +589,7 @@ def test_no_read_write(): updates = {} srv = ServerStub(updates) - obj = Mod('obj', logger, {'description': '', 'param': {'default': 'cheese'}}, srv) + obj = Mod('obj', logger, {'description': '', 'param': {'value': 'cheese'}}, srv) assert obj.param == 'cheese' assert obj.read_param() == 'cheese' assert updates == {'obj': {'param': 'cheese'}} diff --git a/test/test_poller.py b/test/test_poller.py index 6a08131..b5a682e 100644 --- a/test/test_poller.py +++ b/test/test_poller.py @@ -132,6 +132,7 @@ def test_poll(ncycles, pollinterval, slowinterval, mspan, pspan, monkeypatch): m.pollinterval = pollinterval m.slowInterval = slowinterval m.run(ncycles) + print(getattr(m.parameters['param4'], 'stat', None)) assert not hasattr(m.parameters['param4'], 'stat') for pname in ['value', 'status']: pobj = m.parameters[pname]