From 059236885c4b086bdec7512dd6de140243b1180f Mon Sep 17 00:00:00 2001 From: Mick Phillips Date: Wed, 27 Nov 2019 16:42:59 +0000 Subject: [PATCH 1/9] Catch exceptions during shutdown. Leaving this unhandled could prevent other devices in the same process (e.g. on a controller) being shut down. --- microscope/devices.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/microscope/devices.py b/microscope/devices.py index 8ee62256..d8ceae55 100644 --- a/microscope/devices.py +++ b/microscope/devices.py @@ -256,7 +256,10 @@ def initialize(self): def shutdown(self): """Shutdown the device for a prolonged period of inactivity.""" - self.disable() + try: + self.disable() + except Exception as e: + _logger.debug("Exeption in disable() during shutdown: %s" % e) _logger.info("Shutting down ... ... ...") self._on_shutdown() _logger.info("... ... ... ... shut down completed.") From 0058fd92255da2fcd34e2ea70b6c6925ab7f551e Mon Sep 17 00:00:00 2001 From: Mick Phillips Date: Wed, 27 Nov 2019 17:04:59 +0000 Subject: [PATCH 2/9] Don't leave caller waiting for data if camera not enabled. --- microscope/devices.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/microscope/devices.py b/microscope/devices.py index d8ceae55..333b75cd 100644 --- a/microscope/devices.py +++ b/microscope/devices.py @@ -614,6 +614,8 @@ def grab_next_data(self, soft_trigger=True): :param soft_trigger: calls soft_trigger if True, waits for hardware trigger if False. """ + if not self.enabled: + raise Exception("Camera not enabled.") self._new_data_condition.acquire() # Push self onto client stack. self.set_client(self) From 5e0f57ea0e241d14293d9a04eff4fe3522714872 Mon Sep 17 00:00:00 2001 From: Mick Phillips Date: Thu, 19 Dec 2019 11:55:02 +0000 Subject: [PATCH 3/9] Added error tolerance to get_all_settings. get_all_settings reports the values of problem settings as None. Previously, we considered adding a new entry, __errors__, to the returned dict, but this is adding an item that is not a setting to a dict where everything else is a setting, and would require clients to handle this key as a special case. --- microscope/devices.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/microscope/devices.py b/microscope/devices.py index 333b75cd..e05772a9 100644 --- a/microscope/devices.py +++ b/microscope/devices.py @@ -307,11 +307,15 @@ def get_setting(self, name): 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()} - except Exception as err: - _logger.error("in get_all_settings:", exc_info=err) - raise + # Fetching some settings may fail depending on device state. + # Report these values as 'None' and continue fetching other settings. + def catch(f): + try: + return f() + except Exception as err: + _logger.error("getting %s: %s" % (f.__self__.name, err)) + return None + return {k: catch(v.get) for k, v in self._settings.items()} def set_setting(self, name, value): """Set a setting.""" From 71850b33619aaefb7286dfad3b19603056b3746b Mon Sep 17 00:00:00 2001 From: Mick Phillips Date: Thu, 5 Dec 2019 17:10:29 +0000 Subject: [PATCH 4/9] Fix logging typo. --- microscope/devices.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/microscope/devices.py b/microscope/devices.py index e05772a9..820bdeaa 100644 --- a/microscope/devices.py +++ b/microscope/devices.py @@ -259,7 +259,7 @@ def shutdown(self): try: self.disable() except Exception as e: - _logger.debug("Exeption in disable() during shutdown: %s" % e) + _logger.debug("Exception in disable() during shutdown: %s" % e) _logger.info("Shutting down ... ... ...") self._on_shutdown() _logger.info("... ... ... ... shut down completed.") From 1fef4b3a42d20cf2a1ec49f7bd4d9afbcf47e4e6 Mon Sep 17 00:00:00 2001 From: Mick Phillips Date: Wed, 11 Dec 2019 10:30:29 +0000 Subject: [PATCH 5/9] Don't start collection thread until hardware is enabled. --- microscope/devices.py | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/microscope/devices.py b/microscope/devices.py index 820bdeaa..aebd1aee 100644 --- a/microscope/devices.py +++ b/microscope/devices.py @@ -438,18 +438,6 @@ def enable(self): Implement device-specific code in _on_enable . """ _logger.debug("Enabling ...") - if self._using_callback: - if self._fetch_thread: - self._fetch_thread_run = False - else: - if not self._fetch_thread or not self._fetch_thread.is_alive(): - self._fetch_thread = Thread(target=self._fetch_loop) - self._fetch_thread.daemon = True - self._fetch_thread.start() - if not self._dispatch_thread or not self._dispatch_thread.is_alive(): - self._dispatch_thread = Thread(target=self._dispatch_loop) - self._dispatch_thread.daemon = True - self._dispatch_thread.start() # Call device-specific code. try: result = self._on_enable() @@ -461,7 +449,20 @@ def enable(self): self.enabled = False else: self.enabled = True - _logger.debug("... enabled.") + # Set up data fetching + if self._using_callback: + if self._fetch_thread: + self._fetch_thread_run = False + else: + if not self._fetch_thread or not self._fetch_thread.is_alive(): + self._fetch_thread = Thread(target=self._fetch_loop) + self._fetch_thread.daemon = True + self._fetch_thread.start() + if not self._dispatch_thread or not self._dispatch_thread.is_alive(): + self._dispatch_thread = Thread(target=self._dispatch_loop) + self._dispatch_thread.daemon = True + self._dispatch_thread.start() + _logger.debug("... enabled.") return self.enabled From 5ff43e0c6ca4b4f2fdeeabf43470a5dbc4713b88 Mon Sep 17 00:00:00 2001 From: Mick Phillips Date: Wed, 11 Dec 2019 10:31:25 +0000 Subject: [PATCH 6/9] TestCamera throws exceptions if it should be initialized but isn't. --- microscope/testsuite/devices.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/microscope/testsuite/devices.py b/microscope/testsuite/devices.py index 7aa395c2..0ac4fd91 100644 --- a/microscope/testsuite/devices.py +++ b/microscope/testsuite/devices.py @@ -36,6 +36,15 @@ _logger = logging.getLogger(__name__) +from functools import wraps +def must_be_initialized(f): + @wraps(f) + def wrapper(self, *args, **kwargs): + if hasattr(self, '_initialized') and self._initialized: + return f(self, *args, **kwargs) + else: + raise Exception("Device not initialized.") + return wrapper class CamEnum(IntEnum): A = 1 @@ -215,11 +224,13 @@ def _purge_buffers(self): """Purge buffers on both camera and PC.""" _logger.info("Purging buffers.") + @must_be_initialized def _create_buffers(self): """Create buffers and store values needed to remove padding later.""" self._purge_buffers() _logger.info("Creating buffers.") + @must_be_initialized def _fetch_data(self): if self._acquiring and self._triggered > 0: if random.randint(0, 100) < self._error_percent: @@ -249,6 +260,7 @@ def initialize(self): """ _logger.info('Initializing.') time.sleep(0.5) + self._initialized = True def make_safe(self): if self._acquiring: @@ -257,6 +269,7 @@ def make_safe(self): def _on_disable(self): self.abort() + @must_be_initialized def _on_enable(self): _logger.info("Preparing for acquisition.") if self._acquiring: @@ -282,6 +295,7 @@ def _get_sensor_shape(self): def get_trigger_type(self): return devices.TRIGGER_SOFT + @must_be_initialized def soft_trigger(self): _logger.info('Trigger received; self._acquiring is %s.', self._acquiring) From f11d893604c3486f544036a0a5dd05a1e847b304 Mon Sep 17 00:00:00 2001 From: Mick Phillips Date: Mon, 16 Dec 2019 17:03:45 +0000 Subject: [PATCH 7/9] Only evaluate necessary transforms. Dict evaulation is not lazy: previously, this code evaluated all allowable transforms before picking the appropriate one. This is now fixed by breaking it down into the rotation transfrom, then passing the result to one function from a dict of flip transform functions. --- microscope/devices.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/microscope/devices.py b/microscope/devices.py index aebd1aee..25b67639 100644 --- a/microscope/devices.py +++ b/microscope/devices.py @@ -685,12 +685,15 @@ def _process_data(self, data): rot = self._transform[2] # Choose appropriate transform based on (flips, rot). - return {(0, 0): numpy.rot90(data, rot), - (0, 1): numpy.flipud(numpy.rot90(data, rot)), - (1, 0): numpy.fliplr(numpy.rot90(data, rot)), - (1, 1): numpy.fliplr(numpy.flipud(numpy.rot90(data, rot))) - }[flips] - + # Do rotation + data = numpy.rot90(data, rot) + # Flip + data = {(0, 0): lambda d: d, + (0, 1): numpy.flipud, + (1, 0): numpy.fliplr, + (1, 1): lambda d: numpy.fliplr(numpy.flipud(d)) + }[flips](data) + return super()._process_data(data) def set_readout_mode(self, description): """Set the readout mode and _readout_transform.""" From 78366c87241e7a8e76735da1613a95dcd221c223 Mon Sep 17 00:00:00 2001 From: Mick Phillips Date: Mon, 16 Dec 2019 17:06:07 +0000 Subject: [PATCH 8/9] Changed logging level for problems disabling devices on shutdown. --- microscope/devices.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/microscope/devices.py b/microscope/devices.py index 25b67639..73e3ce54 100644 --- a/microscope/devices.py +++ b/microscope/devices.py @@ -259,7 +259,7 @@ def shutdown(self): try: self.disable() except Exception as e: - _logger.debug("Exception in disable() during shutdown: %s" % e) + _logger.warning("Exception in disable() during shutdown: %s" % e) _logger.info("Shutting down ... ... ...") self._on_shutdown() _logger.info("... ... ... ... shut down completed.") From c2fc01233b9e543b1fde59538f2b7c255bbf67b3 Mon Sep 17 00:00:00 2001 From: Mick Phillips Date: Mon, 13 Jan 2020 11:38:21 +0000 Subject: [PATCH 9/9] Linting devices.py. --- microscope/devices.py | 126 ++++++++++++++++++++++-------------------- 1 file changed, 66 insertions(+), 60 deletions(-) diff --git a/microscope/devices.py b/microscope/devices.py index 73e3ce54..7d70e123 100644 --- a/microscope/devices.py +++ b/microscope/devices.py @@ -1,21 +1,21 @@ #!/usr/bin/env python # -*- coding: utf-8 -*- -## Copyright (C) 2017 David Pinto -## Copyright (C) 2016 Mick Phillips -## -## Microscope 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 3 of the License, or -## (at your option) any later version. -## -## Microscope 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 Microscope. If not, see . +# Copyright (C) 2017-2020 David Pinto +# Copyright (C) 2016-2020 Mick Phillips +# +# Microscope 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 3 of the License, or +# (at your option) any later version. +# +# Microscope 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 Microscope. If not, see . """Classes for control of microscope components. @@ -55,9 +55,9 @@ (TRIGGER_AFTER, TRIGGER_BEFORE, TRIGGER_DURATION, TRIGGER_SOFT) = range(4) # Mapping of setting data types to descriptors allowed-value description types. -# For python 2 and 3 compatibility, we convert the type into a descriptor string. -# This avoids problems with, say a python 2 client recognising a python 3 -# as a python 2 . +# For python 2 and 3 compatibility, we convert the type into a descriptor +# string. This avoids problems with, say a python 2 client recognising a +# python 3 as a python 2 . DTYPES = {'int': ('int', tuple), 'float': ('float', tuple), 'bool': ('bool', type(None)), @@ -70,16 +70,19 @@ str: ('str', int), tuple: ('tuple', type(None))} -# A utility function to call callables or return value of non-callables. -# noinspection PyPep8 -_call_if_callable = lambda f: f() if callable(f) else f + +def call_if_callable(f): + """Call callables, or return value of non-callables.""" + return f() if callable(f) else f 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): + # 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. :param name: the setting's name @@ -102,7 +105,8 @@ def __init__(self, name, dtype, get_func, set_func=None, values=None, readonly=F if dtype not in DTYPES: raise Exception('Unsupported dtype.') elif not (isinstance(values, DTYPES[dtype][1:]) or callable(values)): - raise Exception("Invalid values type for %s '%s': expected function or %s" % + raise Exception("Invalid values type for %s '%s':" + "expected function or %s" % (dtype, name, DTYPES[dtype][1:])) self.dtype = DTYPES[dtype][0] self._get = get_func @@ -211,11 +215,9 @@ def __init__(self, index=None): def __del__(self): self.shutdown() - def get_is_enabled(self): return self.enabled - def _on_disable(self): """Do any device-specific work on disable. @@ -268,7 +270,8 @@ def make_safe(self): """Put the device into a safe state.""" pass - def add_setting(self, name, dtype, get_func, set_func, values, readonly=False): + def add_setting(self, name, dtype, get_func, set_func, values, + readonly=False): """Add a setting definition. :param name: the setting's name @@ -291,7 +294,8 @@ class with getter, setter, etc., and adding Setting instances as if dtype not in DTYPES: raise Exception('Unsupported dtype.') elif not (isinstance(values, DTYPES[dtype][1:]) or callable(values)): - raise Exception("Invalid values type for %s '%s': expected function or %s" % + 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, @@ -465,7 +469,6 @@ def enable(self): _logger.debug("... enabled.") return self.enabled - def disable(self): """Disable the data capture device. @@ -501,13 +504,14 @@ def _send_data(self, client, data, timestamp): # this function name as an argument to set_client, but # not sure how to subsequently resolve this over Pyro. client.receiveData(data, timestamp) - except (Pyro4.errors.ConnectionClosedError, Pyro4.errors.CommunicationError): + except (Pyro4.errors.ConnectionClosedError, + Pyro4.errors.CommunicationError): # Client not listening _logger.info("Removing %s from client stack: disconnected.", client._pyroUri) self._clientStack = list(filter(client.__ne__, self._clientStack)) self._liveClients = self._liveClients.difference([client]) - except: + except Exception: raise def _dispatch_loop(self): @@ -525,12 +529,13 @@ def _dispatch_loop(self): err = e else: try: - self._send_data(client, self._process_data(data), timestamp) + self._send_data(client, self._process_data(data), + timestamp) except Exception as e: err = e if err: - # Raising an exception will kill the dispatch loop. We need another - # way to notify the client that there was a problem. + # Raising an exception will kill the dispatch loop. We need + # another way to notify the client that there was a problem. _logger.error("in _dispatch_loop:", exc_info=err) self._dispatch_buffer.task_done() @@ -543,13 +548,13 @@ def _fetch_loop(self): data = self._fetch_data() except Exception as e: _logger.error("in _fetch_loop:", exc_info=e) - # Raising an exception will kill the fetch loop. We need another - # way to notify the client that there was a problem. + # Raising an exception will kill the fetch loop. We need + # another way to notify the client that there was a problem. timestamp = time.time() self._put(e, timestamp) data = None if data is not None: - # ***TODO*** Add support for timestamp from hardware. + # TODO Add support for timestamp from hardware. timestamp = time.time() self._put(data, timestamp) else: @@ -602,7 +607,6 @@ def set_client(self, new_client): else: _logger.info("Current client is %s.", str(self._client)) - @keep_acquiring def update_settings(self, settings, init=False): """Update settings, toggling acquisition if necessary.""" @@ -679,6 +683,7 @@ def __init__(self, **kwargs): self.get_roi, self.set_roi, None) + def _process_data(self, data): """Apply self._transform to data.""" flips = (self._transform[0], self._transform[1]) @@ -708,7 +713,8 @@ def set_transform(self, transform): if isinstance(transform, str): transform = literal_eval(transform) self._client_transform = transform - lr, ud, rot = (self._readout_transform[i] ^ transform[i] for i in range(3)) + lr, ud, rot = (self._readout_transform[i] ^ transform[i] + for i in range(3)) if self._readout_transform[2] and self._client_transform[2]: lr = not lr ud = not ud @@ -758,7 +764,7 @@ def _get_binning(self): pass def get_binning(self): - """Return a tuple of (horizontal, vertical), corrected for transform.""" + """Return a tuple of (horizontal, vertical) corrected for transform.""" binning = self._get_binning() if self._transform[2]: # 90 degree rotation @@ -808,9 +814,9 @@ def set_roi(self, roi): maxw, maxh = self.get_sensor_shape() binning = self.get_binning() left, top, width, height = roi - if not width: # 0 or None + if not width: # 0 or None width = maxw // binning.h - if not height: # 0 o rNone + if not height: # 0 o rNone height = maxh // binning.v if self._transform[2]: roi = ROI(left, top, height, width) @@ -843,6 +849,7 @@ class TriggerType(Enum): FALLING_EDGE = 2 PULSE = 3 + class TriggerMode(Enum): ONCE = 1 BULB = 2 @@ -866,6 +873,7 @@ class TriggerTargetMixIn(metaclass=abc.ABCMeta): @property def trigger_mode(self) -> TriggerMode: return self._trigger_mode + @property def trigger_type(self) -> TriggerType: return self._trigger_type @@ -889,11 +897,11 @@ class SerialDeviceMixIn(metaclass=abc.ABCMeta): """ def __init__(self, **kwargs): super().__init__(**kwargs) - ## TODO: We should probably construct the connection here but - ## the Serial constructor takes a lot of arguments, and - ## it becomes tricky to separate those from arguments to - ## the constructor of other parent classes. - self.connection = None # serial.Serial (to be constructed by child) + # TODO: We should probably construct the connection here but + # the Serial constructor takes a lot of arguments, and + # it becomes tricky to separate those from arguments to + # the constructor of other parent classes. + self.connection = None # serial.Serial (to be constructed by child) self._comms_lock = threading.RLock() def _readline(self): @@ -962,8 +970,8 @@ def __init__(self, **kwargs) -> None: """ super().__init__(**kwargs) - self._patterns = None # type: typing.Optional[numpy.ndarray] - self._pattern_idx = -1 # type: int + self._patterns = None # type: typing.Optional[numpy.ndarray] + self._pattern_idx = -1 # type: int @property @abc.abstractmethod @@ -1008,7 +1016,7 @@ def queue_patterns(self, patterns: numpy.ndarray) -> None: """ self._validate_patterns(patterns) self._patterns = patterns - self._pattern_idx = -1 # none is applied yet + self._pattern_idx = -1 # none is applied yet def next_pattern(self) -> None: """Apply the next pattern in the queue. @@ -1018,7 +1026,7 @@ def next_pattern(self) -> None: if self._patterns is None: raise Exception("no pattern queued to apply") self._pattern_idx += 1 - self.apply_pattern(self._patterns[self._pattern_idx,:]) + self.apply_pattern(self._patterns[self._pattern_idx, :]) def initialize(self) -> None: pass @@ -1087,16 +1095,16 @@ def set_power_mw(self, mw): class FilterWheelBase(Device, metaclass=abc.ABCMeta): - def __init__(self, filters: typing.Union[typing.Mapping[int, str], typing.Iterable] = [], - positions: int = 0, **kwargs) -> None: + def __init__(self, filters: typing.Union[typing.Mapping[int, str], + typing.Iterable] = [], positions: int = 0, **kwargs) -> None: super().__init__(**kwargs) if isinstance(filters, dict): self._filters = filters else: - self._filters = {i:f for (i, f) in enumerate(filters)} + self._filters = {i: f for (i, f) in enumerate(filters)} self._inv_filters = {val: key for key, val in self._filters.items()} if not hasattr(self, '_positions'): - self._positions = positions # type: int + self._positions = positions # type: int # The position as an integer. # Deprecated: clients should call get_position and set_position; # still exposed as a setting until cockpit uses set_position. @@ -1104,12 +1112,11 @@ def __init__(self, filters: typing.Union[typing.Mapping[int, str], typing.Iterab 'int', self.get_position, self.set_position, - lambda: (0, self.get_num_positions()) ) - + lambda: (0, self.get_num_positions())) def get_num_positions(self) -> int: """Returns the number of wheel positions.""" - return(max( self._positions, len(self._filters))) + return(max(self._positions, len(self._filters))) @abc.abstractmethod def get_position(self) -> int: @@ -1122,7 +1129,7 @@ def set_position(self, position: int) -> None: pass def get_filters(self) -> typing.List[typing.Tuple[int, str]]: - return [(k,v) for k,v in self._filters.items()] + return [(k, v) for k, v in self._filters.items()] class ControllerDevice(Device, metaclass=abc.ABCMeta): @@ -1143,7 +1150,6 @@ class ControllerDevice(Device, metaclass=abc.ABCMeta): their shutdown and initialisation. """ - def initialize(self) -> None: super().initialize() for d in self.devices.values():