check for problematic value range

A common problematic practice is, to declare the value parameter
with the same FloatRange than the target. Because of measurement
errors, a value might be near, but outside the limit.
In order to avoid this, we force the programmer to declare a
bigger range for the value than for the target, or to
explicitly disable this check on a module property.
It is also fine to declare the value without limits.

This behavior may be disabled via command line option or in the
general config file. For simplicity, FloatRanges inside data
structures are not considered.

+ above command line option is also used to disable the error
  handling on a string to float conversion
+ log appropriate error message for string to float conversion

Change-Id: Ib78ea1fb7c821442bf5847030573c8c27822dea5
Reviewed-on: https://forge.frm2.tum.de/review/c/sine2020/secop/playground/+/27574
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>

bin/secop-server

@@ -75,6 +75,11 @@ def parseArgv(argv):
                         action='store_true',
                         help='check cfg files only',
                         default=False)
+    parser.add_argument('-r',
+                        '--relaxed',
+                        action='store_true',
+                        help='no checking of problematic behaviour',
+                        default=False)
     return parser.parse_args(argv)
 
 
@@ -85,6 +90,9 @@ def main(argv=None):
     args = parseArgv(argv[1:])
 
     loglevel = 'debug' if args.verbose else ('error' if args.quiet else 'info')
+    if args.relaxed:
+        generalConfig.defaults['lazy_number_validation'] = True
+        generalConfig.defaults['disable_value_range_check'] = True
     generalConfig.init(args.gencfg)
     logger.init(loglevel)
 

secop/datatypes.py

@@ -35,25 +35,20 @@ from secop.lib.enum import Enum
 from secop.parse import Parser
 from secop.properties import HasProperties, Property
 
-# Only export these classes for 'from secop.datatypes import *'
-__all__ = [
-    'DataType', 'get_datatype',
-    'FloatRange', 'IntRange', 'ScaledInteger',
-    'BoolType', 'EnumType',
-    'BLOBType', 'StringType', 'TextType',
-    'TupleOf', 'ArrayOf', 'StructOf',
-    'CommandType', 'StatusType',
-]
-
 # *DEFAULT* limits for IntRange/ScaledIntegers transport serialisation
 DEFAULT_MIN_INT = -16777216
 DEFAULT_MAX_INT = 16777216
 UNLIMITED = 1 << 64  # internal limit for integers, is probably high enough for any datatype size
-generalConfig.defaults['lazy_number_validation'] = True  # should be changed to False later
+generalConfig.defaults['lazy_number_validation'] = False
 
 Parser = Parser()
 
 
+class DiscouragedConversion(BadValueError):
+    """the discouraged conversion string - > float happened"""
+    log_message = True
+
+
 # base class for all DataTypes
 class DataType(HasProperties):
     """base class for all data types"""
@@ -64,7 +59,7 @@ class DataType(HasProperties):
     def __call__(self, value):
         """check if given value (a python obj) is valid for this datatype
 
-        returns the value or raises an appropriate exception"""
+        returns the (possibly converted) value or raises an appropriate exception"""
         raise NotImplementedError
 
     def from_string(self, text):
@@ -193,12 +188,15 @@ class FloatRange(DataType):
 
     def __call__(self, value):
         try:
-            if generalConfig.lazy_number_validation:
+            value += 0.0  # do not accept strings here
-                value = float(value)  # for legacy code
-            else:
-                value += 0.0  # do not accept strings here
         except Exception:
-            raise BadValueError('Can not convert %r to float' % value) from None
+            try:
+                value = float(value)
+            except Exception:
+                raise BadValueError('Can not convert %r to float' % value) from None
+            if not generalConfig.lazy_number_validation:
+                raise DiscouragedConversion('automatic string to float conversion no longer supported') from None
+
         # map +/-infty to +/-max possible number
         value = clamp(-sys.float_info.max, value, sys.float_info.max)
 
@@ -236,6 +234,18 @@ class FloatRange(DataType):
             return ' '.join([self.fmtstr % value, unit])
         return self.fmtstr % value
 
+    def problematic_range(self, target_type):
+        """check problematic range
+
+        returns True when self.min or self.max is given, not 0 and equal to the same limit on target_type.
+        """
+        value_info = self.get_info()
+        target_info = target_type.get_info()
+        minval = value_info.get('min')  # None when -infinite
+        maxval = value_info.get('max')  # None when +infinite
+        return ((minval and minval == target_info.get('min')) or
+                (maxval and maxval == target_info.get('max')))
+
     def compatible(self, other):
         if not isinstance(other, (FloatRange, ScaledInteger)):
             raise BadValueError('incompatible datatypes')
@@ -269,13 +279,16 @@ class IntRange(DataType):
 
     def __call__(self, value):
         try:
-            if generalConfig.lazy_number_validation:
+            fvalue = value + 0.0  # do not accept strings here
-                fvalue = float(value)  # for legacy code
-            else:
-                fvalue = value + 0.0  # do not accept strings here
             value = int(value)
         except Exception:
-            raise BadValueError('Can not convert %r to int' % value) from None
+            try:
+                fvalue = float(value)
+                value = int(value)
+            except Exception:
+                raise BadValueError('Can not convert %r to int' % value) from None
+            if not generalConfig.lazy_number_validation:
+                raise DiscouragedConversion('automatic string to float conversion no longer supported') from None
         if not self.min <= value <= self.max or round(fvalue) != fvalue:
             raise BadValueError('%r should be an int between %d and %d' %
                                 (value, self.min, self.max))
@@ -305,13 +318,15 @@ class IntRange(DataType):
         return '%d' % value
 
     def compatible(self, other):
-        if isinstance(other, IntRange):
+        if isinstance(other, (IntRange, FloatRange, ScaledInteger)):
             other(self.min)
             other(self.max)
             return
-        # this will accept some EnumType, BoolType
+        if isinstance(other, (EnumType, BoolType)):
-        for i in range(self.min, self.max + 1):
+            # the following loop will not cycle more than the number of Enum elements
-            other(i)
+            for i in range(self.min, self.max + 1):
+                other(i)
+        raise BadValueError('incompatible datatypes')
 
 
 class ScaledInteger(DataType):
@@ -376,12 +391,14 @@ class ScaledInteger(DataType):
 
     def __call__(self, value):
         try:
-            if generalConfig.lazy_number_validation:
+            value += 0.0  # do not accept strings here
-                value = float(value)  # for legacy code
-            else:
-                value += 0.0  # do not accept strings here
         except Exception:
-            raise BadValueError('Can not convert %r to float' % value) from None
+            try:
+                value = float(value)
+            except Exception:
+                raise BadValueError('Can not convert %r to float' % value) from None
+            if not generalConfig.lazy_number_validation:
+                raise DiscouragedConversion('automatic string to float conversion no longer supported') from None
         prec = max(self.scale, abs(value * self.relative_resolution),
                    self.absolute_resolution)
         if self.min - prec <= value <= self.max + prec:
@@ -1128,37 +1145,26 @@ def floatargs(kwds):
 DATATYPES = dict(
     bool    = lambda **kwds:
         BoolType(),
-
     int     = lambda min, max, **kwds:
         IntRange(minval=min, maxval=max),
-
     scaled  = lambda scale, min, max, **kwds:
         ScaledInteger(scale=scale, minval=min*scale, maxval=max*scale, **floatargs(kwds)),
-
     double  = lambda min=None, max=None, **kwds:
         FloatRange(minval=min, maxval=max, **floatargs(kwds)),
-
     blob    = lambda maxbytes, minbytes=0, **kwds:
         BLOBType(minbytes=minbytes, maxbytes=maxbytes),
-
     string  = lambda minchars=0, maxchars=None, isUTF8=False, **kwds:
         StringType(minchars=minchars, maxchars=maxchars, isUTF8=isUTF8),
-
     array   = lambda maxlen, members, minlen=0, pname='', **kwds:
         ArrayOf(get_datatype(members, pname), minlen=minlen, maxlen=maxlen),
-
     tuple   = lambda members, pname='', **kwds:
         TupleOf(*tuple((get_datatype(t, pname) for t in members))),
-
     enum    = lambda members, pname='', **kwds:
         EnumType(pname, members=members),
-
     struct  = lambda members, optional=None, pname='', **kwds:
         StructOf(optional, **dict((n, get_datatype(t, pname)) for n, t in list(members.items()))),
-
     command = lambda argument=None, result=None, pname='', **kwds:
         CommandType(get_datatype(argument, pname), get_datatype(result)),
-
     limit   = lambda members, pname='', **kwds:
         LimitsType(get_datatype(members, pname)),
 )

secop/io.py

@@ -49,7 +49,7 @@ class HasIodev(Module):
 
     def __init__(self, name, logger, opts, srv):
         iodev = opts.get('iodev')
-        Module.__init__(self, name, logger, opts, srv)
+        super().__init__(name, logger, opts, srv)
         if self.uri:
             opts = {'uri': self.uri, 'description': 'communication device for %s' % name,
                     'export': False}

secop/modules.py

@@ -29,10 +29,10 @@ from collections import OrderedDict
 from functools import wraps
 
 from secop.datatypes import ArrayOf, BoolType, EnumType, FloatRange, \
-    IntRange, StatusType, StringType, TextType, TupleOf
+    IntRange, StatusType, StringType, TextType, TupleOf, DiscouragedConversion
 from secop.errors import BadValueError, ConfigError, \
     ProgrammingError, SECoPError, SilentError, secop_error
-from secop.lib import formatException, mkthread, UniqueObject
+from secop.lib import formatException, mkthread, UniqueObject, generalConfig
 from secop.lib.enum import Enum
 from secop.params import Accessible, Command, Parameter
 from secop.poller import BasicPoller, Poller
@@ -40,6 +40,8 @@ from secop.properties import HasProperties, Property
 from secop.logging import RemoteLogHandler, HasComlog
 
 
+generalConfig.defaults['disable_value_range_check'] = False  # check for problematic value range by default
+
 Done = UniqueObject('already set')
 """a special return value for a read/write function
 
@@ -475,9 +477,13 @@ class Module(HasAccessibles):
         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 isinstance(e, DiscouragedConversion):
+                if DiscouragedConversion.log_message:
+                    self.log.error(str(e))
+                    self.log.error('you may disable this behaviour by running the server with --relaxed')
+                    DiscouragedConversion.log_message = False
             if not err:  # do not overwrite given error
                 err = e
         if err:
@@ -682,10 +688,30 @@ class Readable(Module):
 
 class Writable(Readable):
     """basic writable module"""
-
+    disable_value_range_check = Property('disable value range check', BoolType(), default=False)
     target = Parameter('target value of the module',
                        default=0, readonly=False, datatype=FloatRange(unit='$'))
 
+    def __init__(self, name, logger, cfgdict, srv):
+        super().__init__(name, logger, cfgdict, srv)
+        value_dt = self.parameters['value'].datatype
+        target_dt = self.parameters['target'].datatype
+        try:
+            # this handles also the cases where the limits on the value are more
+            # restrictive than on the target
+            target_dt.compatible(value_dt)
+        except Exception:
+            if type(value_dt) == type(target_dt):
+                raise ConfigError('the target range extends beyond the value range') from None
+            raise ProgrammingError('the datatypes of target and value are not compatible') from None
+        if isinstance(value_dt, FloatRange):
+            if (not self.disable_value_range_check and not generalConfig.disable_value_range_check
+                    and value_dt.problematic_range(target_dt)):
+                self.log.error('the value range must be bigger than the target range!')
+                self.log.error('you may disable this error message by running the server with --relaxed')
+                self.log.error('or by setting the disable_value_range_check property of the module to True')
+                raise ConfigError('the value range must be bigger than the target range')
+
 
 class Drivable(Writable):
     """basic drivable module"""

test/test_datatypes.py

@@ -28,7 +28,8 @@ import pytest
 from secop.datatypes import ArrayOf, BLOBType, BoolType, \
     CommandType, ConfigError, DataType, Enum, EnumType, FloatRange, \
     IntRange, ProgrammingError, ScaledInteger, StatusType, \
-    StringType, StructOf, TextType, TupleOf, get_datatype
+    StringType, StructOf, TextType, TupleOf, get_datatype, \
+    DiscouragedConversion
 from secop.lib import generalConfig
 
 
@@ -677,5 +678,5 @@ def test_lazy_validation(dt):
     generalConfig.defaults['lazy_number_validation'] = True
     dt('0')
     generalConfig.defaults['lazy_number_validation'] = False
-    with pytest.raises(ValueError):
+    with pytest.raises(DiscouragedConversion):
         dt('0')

test/test_modules.py

@@ -24,13 +24,14 @@
 
 import pytest
 
-from secop.datatypes import BoolType, FloatRange, StringType, IntRange
+from secop.datatypes import BoolType, FloatRange, StringType, IntRange, ScaledInteger
 from secop.errors import ProgrammingError, ConfigError
 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
+from secop.lib import generalConfig
 
 
 class DispatcherStub:
@@ -54,7 +55,7 @@ class DispatcherStub:
 class LoggerStub:
     def debug(self, fmt, *args):
         print(fmt % args)
-    info = warning = exception = debug
+    info = warning = exception = error = debug
     handlers = []
 
 
@@ -88,6 +89,7 @@ def test_ModuleMagic():
         a1 = Parameter('a1', datatype=BoolType(), default=False)
         a2 = Parameter('a2', datatype=BoolType(), default=True)
         value = Parameter(datatype=StringType(), default='first')
+        target = Parameter(datatype=StringType(), default='')
 
         @Command(argument=BoolType(), result=BoolType())
         def cmd2(self, arg):
@@ -136,6 +138,7 @@ def test_ModuleMagic():
             return arg
 
         value = Parameter(datatype=FloatRange(unit='deg'))
+        target = Parameter(datatype=FloatRange(), default=0)
         a1 = Parameter(datatype=FloatRange(unit='$/s'), readonly=False)
         b2 = Parameter('<b2>', datatype=BoolType(), default=True,
                        poll=True, readonly=False, initwrite=True)
@@ -174,7 +177,7 @@ def test_ModuleMagic():
 
     # check for inital updates working properly
     o1 = Newclass1('o1', logger, {'.description':''}, srv)
-    expectedBeforeStart = {'target': 0.0, 'status': (Drivable.Status.IDLE, ''),
+    expectedBeforeStart = {'target': '', 'status': (Drivable.Status.IDLE, ''),
             'param1': False, 'param2': 1.0, 'a1': 0.0, 'a2': True, 'pollinterval': 5.0,
             'value': 'first'}
     assert updates.pop('o1') == expectedBeforeStart
@@ -191,6 +194,7 @@ def test_ModuleMagic():
     o2 = Newclass2('o2', logger, {'.description':'', 'a1': 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
     o2.earlyInit()
     event = MultiEvent()
@@ -214,7 +218,7 @@ def test_ModuleMagic():
     # check '$' in unit works properly
     assert o2.parameters['a1'].datatype.unit == 'mm/s'
     cfg = Newclass2.configurables
-    assert set(cfg.keys()) == {'export', 'group', 'description',
+    assert set(cfg.keys()) == {'export', 'group', 'description', 'disable_value_range_check',
         'meaning', 'visibility', 'implementation', 'interface_classes', 'target', 'stop',
         'status', 'param1', 'param2', 'cmd', 'a2', 'pollinterval', 'b2', 'cmd2', 'value',
         'a1'}
@@ -575,3 +579,65 @@ def test_no_read_write():
     assert obj.write_param('egg') == 'egg'
     assert obj.param == 'egg'
     assert updates == {'obj': {'param': 'egg'}}
+
+
+def test_incompatible_value_target():
+    class Mod1(Drivable):
+        value = Parameter('', FloatRange(0, 10), default=0)
+        target = Parameter('', FloatRange(0, 11), default=0)
+
+    class Mod2(Drivable):
+        value = Parameter('', FloatRange(), default=0)
+        target = Parameter('', StringType(), default='')
+
+    class Mod3(Drivable):
+        value = Parameter('', FloatRange(), default=0)
+        target = Parameter('', ScaledInteger(1, 0, 10), default=0)
+
+    srv = ServerStub({})
+
+    with pytest.raises(ConfigError):
+        obj = Mod1('obj', logger, {'description': ''}, srv)  # pylint: disable=unused-variable
+
+    with pytest.raises(ProgrammingError):
+        obj = Mod2('obj', logger, {'description': ''}, srv)
+
+    obj = Mod3('obj', logger, {'description': ''}, srv)
+
+
+def test_problematic_value_range():
+    class Mod(Drivable):
+        value = Parameter('', FloatRange(0, 10), default=0)
+        target = Parameter('', FloatRange(0, 10), default=0)
+
+    srv = ServerStub({})
+
+    obj = Mod('obj', logger, {'description': '', 'value.max': 10.1}, srv)  # pylint: disable=unused-variable
+
+    with pytest.raises(ConfigError):
+        obj = Mod('obj', logger, {'description': ''}, srv)
+
+    class Mod2(Drivable):
+        value = Parameter('', FloatRange(), default=0)
+        target = Parameter('', FloatRange(), default=0)
+
+    obj = Mod2('obj', logger, {'description': ''}, srv)
+    obj = Mod2('obj', logger, {'description': '', 'target.min': 0, 'target.max': 10}, srv)
+
+    with pytest.raises(ConfigError):
+        obj = Mod('obj', logger, {
+            'value.min': 0, 'value.max': 10,
+            'target.min': 0, 'target.max': 10, 'description': ''}, srv)
+
+    obj = Mod('obj', logger, {'disable_value_range_check': True,
+        'value.min': 0, 'value.max': 10,
+        'target.min': 0, 'target.max': 10, 'description': ''}, srv)
+
+    generalConfig.defaults['disable_value_range_check'] = True
+
+    class Mod4(Drivable):
+        value = Parameter('', FloatRange(0, 10), default=0)
+        target = Parameter('', FloatRange(0, 10), default=0)
+    obj = Mod4('obj', logger, {
+        'value.min': 0, 'value.max': 10,
+        'target.min': 0, 'target.max': 10, 'description': ''}, srv)