Make the Setting class private to enable refactoring of Settings (issue #125)

Managing different types of Settings may be achieved more flexibly
and simply through the use of subclasses of Setting and the use of a
factory pattern or helper method for their creation.  So make the
`Setting` class and the `Device.settings` attribute private so that
the `Device` methods become the only sactioned method to get to them.

* microscope/cameras/atmcd.py: use the `*_setting` methods instead of
  manipulating `self.settings` directly.
* microscope/devices.py: mark `Setting` class and `Device.settings`
  attribute as private.
* microscope/testsuite/test_setting.py: Setting class is now _Setting.
* NEWS: make note of backwards incompatible change.

Author: mickp

NEWS

@@ -4,6 +4,11 @@ python-microscope releases.
 Version 0.4.0 (upcoming)
 ------------------------
 
+* Selected most important, backwards incompatible, changes:
+
+  * The `Setting` class is now private.  The only supported method to
+    add settings to a `Device` is via its `add_setting` method.
+
 * New devices supported:
 
   * Coherent Obis laser

microscope/cameras/atmcd.py

@@ -1132,7 +1132,7 @@ def __str__(self):
 
 from threading import Lock
 from microscope import devices
-from microscope.devices import keep_acquiring, Setting, Binning, ROI
+from microscope.devices import keep_acquiring, Binning, ROI
 import time
 
 # A lock on the DLL used to ensure DLL calls act on the correct device.
@@ -1253,21 +1253,21 @@ def initialize(self):
         # Mode
         name = 'readout mode'
         if self._readout_modes:
-            self.settings[name] = Setting(name, 'enum',
-                                          None,
-                                          self._set_readout_mode,
-                                          lambda: [str(mode) for mode in self._readout_modes])
-            self.settings[name].set(0)
+            self.add_setting(name, 'enum',
+                             None,
+                             self._set_readout_mode,
+                             lambda: [str(mode) for mode in self._readout_modes])
+            self.set_setting(name, 0)
         # TriggerMode
         name = 'TriggerMode'
-        self.settings[name] = Setting(name, 'enum',
-                                      None,
-                                      self._bind(SetTriggerMode),
-                                      TriggerMode)
+        self.add_setting(name, 'enum',
+                         None,
+                         self._bind(SetTriggerMode),
+                         TriggerMode)
         if self._caps.ulTriggerModes & AC_TRIGGERMODE_EXTERNAL:
-            self.settings[name].set(TriggerMode.EXTERNAL)
+            self.set_setting(name, TriggerMode.EXTERNAL)
         elif self._caps.ulTriggerModes & AC_TRIGGERMODE_CONTINUOUS:
-            self.settings[name].set(TriggerMode.SOFTWARE)
+            self.set_setting(name, TriggerMode.SOFTWARE)
         # Gain - device will use either EMGain or MCPGain
         name = 'gain'
         getter, setter, vrange = None, None, None
@@ -1282,9 +1282,7 @@ def initialize(self):
             setter = self._bind(SetMCPGain)
             vrange = self._bind(GetMCPGainRange)
         if getter or setter:
-            self.settings[name] = Setting(name, 'int',
-                                          getter, setter, vrange,
-                                          setter is None)
+            self.add_setting(name, 'int', getter, setter, vrange, setter is None)
         # Temperature
         name = 'TemperatureSetPoint'
         getter, setter, vrange = None, None, None
@@ -1293,77 +1291,73 @@ def initialize(self):
         if self._caps.ulGetFunctions & AC_GETFUNCTION_TEMPERATURERANGE:
             vrange = self._bind(GetTemperatureRange)
         if setter:
-            self.settings[name] = Setting(name, 'int',
-                                          None, setter, vrange,
-                                          setter is None)
+            self.add_setting(name, 'int', None, setter, vrange, setter is None)
         # Set a conservative default temperature set-point.
-        self.settings[name].set(-20)
+        self.set_setting(name, -20)
         # Fan control
         name = 'Temperature'
-        self.settings[name] = Setting(name, 'int',
-                                      self.get_sensor_temperature,
-                                      None, (None, None), True)
+        self.add_setting(name, 'int', self.get_sensor_temperature, None, (None, None), True)
         name = 'Fan mode'
-        self.settings[name] = Setting(name, 'enum',
-                                      None, # Can't query fan mode
-                                      self._bind(SetFanMode),
-                                      {0:'full', 1:'low', 2:'off'}
-                                      )
+        self.add_setting(name, 'enum',
+                         None, # Can't query fan mode
+                         self._bind(SetFanMode),
+                         {0:'full', 1:'low', 2:'off'}
+                         )
         # Cooler control
         name = 'Cooler Enabled'
-        self.settings[name] = Setting(name, 'bool',
-                                      None,
-                                      self._set_cooler_state,
-                                      None)
-        self.settings[name].set(True)
+        self.add_setting(name, 'bool',
+                         None,
+                         self._set_cooler_state,
+                         None)
+        self.set_setting(name, True)
         # Binning
         name = 'Binning'
-        self.settings[name] = Setting(name, 'tuple',
-                                      self.get_binning,
-                                      self.set_binning,
-                                      None)
+        self.add_setting(name, 'tuple',
+                         self.get_binning,
+                         self.set_binning,
+                         None)
         # Roi
         name = 'Roi'
-        self.settings[name] = Setting(name, 'tuple',
-                                      self.get_roi,
-                                      lambda roi: self.set_roi(*roi),
-                                      None)
+        self.add_setting(name, 'tuple',
+                         self.get_roi,
+                         lambda roi: self.set_roi(*roi),
+                         None)
         # BaselineClamp
         name = 'BaselineClamp'
         if self._caps.ulSetFunctions & AC_SETFUNCTION_BASELINECLAMP:
-            self.settings[name] = Setting(name, 'bool',
-                                          None,
-                                          self._bind(SetBaselineClamp))
-            self.settings[name].set(False)
+            self.add_setting(name, 'bool',
+                             None,
+                             self._bind(SetBaselineClamp))
+            self.set_setting(name, False)
         # BaselineOffset
         name = 'BaselineOffset'
         if self._caps.ulSetFunctions & AC_SETFUNCTION_BASELINEOFFSET:
-            self.settings[name] = Setting(name, 'int',
-                                          None,
-                                          self._bind(SetBaselineOffset),
-                                          (-1000, 1000))
-            self.settings[name].set(0)
+            self.add_setting(name, 'int',
+                             None,
+                             self._bind(SetBaselineOffset),
+                             (-1000, 1000))
+            self.set_setting(name, 0)
         # EMAdvanced
         name = 'EMAdvanced'
         if self._caps.ulSetFunctions & AC_SETFUNCTION_EMADVANCED:
-            self.settings[name] = Setting(name, 'bool',
-                                          None,
-                                          self._bind(SetEMAdvanced))
-            self.settings[name].set(False)
+            self.add_setting(name, 'bool',
+                             None,
+                             self._bind(SetEMAdvanced))
+            self.set_setting(name, False)
         # GateMode
         name = 'GateMode'
         if self._caps.ulSetFunctions & AC_SETFUNCTION_GATEMODE:
             vrange = range(0, [5,6][self._caps.ulCameraType & AC_CAMERATYPE_ISTAR])
-            self.setings[name] = Setting(name, 'int',
-                                         None,
-                                         self._bind(SetGateMode),
-                                         vrange)
+            self.add_setting(name, 'int',
+                             None,
+                             self._bind(SetGateMode),
+                             vrange)
         # HighCapacity
         name = 'HighCapacity'
         if self._caps.ulSetFunctions & AC_SETFUNCTION_HIGHCAPACITY:
-            self.settings[name] = Setting(name, 'bool',
-                                          None,
-                                          self._bind(SetHighCapacity))
+            self.add_setting(name, 'bool',
+                             None,
+                             self._bind(SetHighCapacity))
 
     def _fetch_data(self):
         """Poll for data and return it, with minimal processing.
@@ -1429,7 +1423,7 @@ def _on_enable(self):
             SetReadMode(ReadMode.IMAGE)
             x, y = GetDetector()
             self._set_image()
-            if not IsTriggerModeAvailable(self.settings['TriggerMode'].get()):
+            if not IsTriggerModeAvailable(self.get_setting('TriggerMode')):
                 raise AtmcdException("Trigger mode is not valid.")
             StartAcquisition()
         return True
@@ -1506,7 +1500,7 @@ def get_sensor_temperature(self):
 
     def get_trigger_type(self):
         """Return the microscope.devices trigger type."""
-        trig = self.settings['TriggerMode'].get()
+        trig = self.get_setting('TriggerMode')
         if trig == TriggerMode.BULB:
             return devices.TRIGGER_DURATION
         elif trig == TriggerMode.SOFTWARE:

microscope/devices.py

@@ -75,7 +75,10 @@
 _call_if_callable = lambda f: f() if callable(f) else f
 
 
-class Setting():
+class _Setting():
+    # TODO: refactor into subclasses to avoid if isinstance .. elif .. else.
+    # Settings classes should be private: devices should use a factory method
+    # rather than instantiate settings directly; most already use add_setting for this.
     def __init__(self, name, dtype, get_func, set_func=None, values=None, readonly=False):
         """Create a setting.
 
@@ -202,7 +205,7 @@ class Device(metaclass=abc.ABCMeta):
     def __init__(self, index=None):
         self.enabled = None
         # A list of settings. (Can't serialize OrderedDict, so use {}.)
-        self.settings = OrderedDict()
+        self._settings = OrderedDict()
         self._index = index
 
     def __del__(self):
@@ -265,8 +268,6 @@ def make_safe(self):
     def add_setting(self, name, dtype, get_func, set_func, values, readonly=False):
         """Add a setting definition.
 
-        Can also use self.settings[name] = Setting(name, dtype,...)
-
         :param name: the setting's name
         :param dtype: a data type from ('int', 'float', 'bool', 'enum', 'str')
         :param get_func: a function to get the current value
@@ -290,45 +291,46 @@ class with getter, setter, etc., and adding Setting instances as
             raise Exception("Invalid values type for %s '%s': expected function or %s" %
                             (dtype, name, DTYPES[dtype][1:]))
         else:
-            self.settings[name] = Setting(name, dtype, get_func, set_func, values, readonly)
+            self._settings[name] = _Setting(name, dtype, get_func, set_func,
+                                            values, readonly)
 
     def get_setting(self, name):
         """Return the current value of a setting."""
         try:
-            return self.settings[name].get()
+            return self._settings[name].get()
         except Exception as err:
             _logger.error("in get_setting(%s):" % (name), exc_info=err)
             raise
 
     def get_all_settings(self):
         """Return ordered settings as a list of dicts."""
         try:
-            return {k: v.get() for k, v in self.settings.items()}
+            return {k: v.get() for k, v in self._settings.items()}
         except Exception as err:
             _logger.error("in get_all_settings:", exc_info=err)
             raise
 
     def set_setting(self, name, value):
         """Set a setting."""
         try:
-            self.settings[name].set(value)
+            self._settings[name].set(value)
         except Exception as err:
             _logger.error("in set_setting(%s):" % (name), exc_info=err)
             raise
 
     def describe_setting(self, name):
         """Return ordered setting descriptions as a list of dicts."""
-        return self.settings[name].describe()
+        return self._settings[name].describe()
 
     def describe_settings(self):
         """Return ordered setting descriptions as a list of dicts."""
-        return [(k, v.describe()) for (k, v) in self.settings.items()]
+        return [(k, v.describe()) for (k, v) in self._settings.items()]
 
     def update_settings(self, incoming, init=False):
         """Update settings based on dict of settings and values."""
         if init:
             # Assume nothing about state: set everything.
-            my_keys = set(self.settings.keys())
+            my_keys = set(self._settings.keys())
             their_keys = set(incoming.keys())
             update_keys = my_keys & their_keys
             if update_keys != my_keys:
@@ -338,24 +340,24 @@ def update_settings(self, incoming, init=False):
                 raise Exception(msg)
         else:
             # Only update changed values.
-            my_keys = set(self.settings.keys())
+            my_keys = set(self._settings.keys())
             their_keys = set(incoming.keys())
             update_keys = set(key for key in my_keys & their_keys
                               if self.get_setting(key) != incoming[key])
         results = {}
         # Update values.
         for key in update_keys:
-            if key not in my_keys or not self.settings[key].set:
+            if key not in my_keys or not self._settings[key].set:
                 # Setting not recognised or no set function implemented
                 results[key] = NotImplemented
                 update_keys.remove(key)
                 continue
-            if _call_if_callable(self.settings[key].readonly):
+            if _call_if_callable(self._settings[key].readonly):
                 continue
-            self.settings[key].set(incoming[key])
+            self._settings[key].set(incoming[key])
         # Read back values in second loop.
         for key in update_keys:
-            results[key] = self.settings[key].get()
+            results[key] = self._settings[key].get()
         return results
 
 

microscope/testsuite/test_settings.py

@@ -49,8 +49,8 @@ def create_enum_setting(default, with_getter=True, with_setter=True):
     thing = ThingWithSomething(EnumSetting(default))
     getter = thing.get_val if with_getter else None
     setter = thing.set_val if with_setter else None
-    setting = microscope.devices.Setting('foobar', 'enum', get_func=getter,
-                                         set_func=setter, values=EnumSetting)
+    setting = microscope.devices._Setting('foobar', 'enum', get_func=getter,
+                                          set_func=setter, values=EnumSetting)
     return setting, thing