From 0319d83df5025069ff10d5667ac64bd9a3dec5f8 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Thu, 24 Oct 2024 16:44:12 +0100 Subject: [PATCH 1/6] EM27SensorsBase: Wait for response before signalling device is open Closes #415. --- finesse/hardware/plugins/sensors/decades.py | 1 + .../hardware/plugins/sensors/em27_sensors.py | 17 +++++++++++++++-- .../hardware/plugins/sensors/sensors_base.py | 5 ----- .../plugins/sensors/test_sensors_base.py | 3 --- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/finesse/hardware/plugins/sensors/decades.py b/finesse/hardware/plugins/sensors/decades.py index de1d5320..8e33ca3c 100644 --- a/finesse/hardware/plugins/sensors/decades.py +++ b/finesse/hardware/plugins/sensors/decades.py @@ -207,3 +207,4 @@ def _on_params_received(self, reply: QNetworkReply, params: Set[str]) -> None: # Now we have enough information to start parsing sensor readings self.start_polling() + self.request_readings() diff --git a/finesse/hardware/plugins/sensors/em27_sensors.py b/finesse/hardware/plugins/sensors/em27_sensors.py index ae5c2a63..1e9f6163 100644 --- a/finesse/hardware/plugins/sensors/em27_sensors.py +++ b/finesse/hardware/plugins/sensors/em27_sensors.py @@ -51,7 +51,11 @@ class EM27Error(Exception): """Indicates than an error occurred while parsing the webpage.""" -class EM27SensorsBase(SensorsBase, class_type=DeviceClassType.IGNORE): +class EM27SensorsBase( + SensorsBase, + class_type=DeviceClassType.IGNORE, + async_open=True, +): """An interface for monitoring EM27 properties.""" def __init__(self, url: str, poll_interval: float = float("nan")) -> None: @@ -63,8 +67,11 @@ def __init__(self, url: str, poll_interval: float = float("nan")) -> None: """ self._url: str = url self._requester = HTTPRequester() + self._connected = False + + super().__init__(poll_interval, start_polling=False) - super().__init__(poll_interval) + self.request_readings() def request_readings(self) -> None: """Request the EM27 property data from the web server. @@ -89,6 +96,12 @@ def _on_reply_received(self, reply: QNetworkReply) -> None: data: bytes = reply.readAll().data() content = data.decode() readings = get_em27_sensor_data(content) + + if not self._connected: + self._connected = True + self.signal_is_opened() + self.start_polling() + self.send_readings_message(readings) diff --git a/finesse/hardware/plugins/sensors/sensors_base.py b/finesse/hardware/plugins/sensors/sensors_base.py index ac069ea7..4256060d 100644 --- a/finesse/hardware/plugins/sensors/sensors_base.py +++ b/finesse/hardware/plugins/sensors/sensors_base.py @@ -47,11 +47,6 @@ def start_polling(self) -> None: if not isnan(self._poll_interval): self._poll_timer.start(int(self._poll_interval * 1000)) - # Poll device once on open. - # TODO: Run this synchronously so we can check that things work before the - # device.opened message is sent - self.request_readings() - @abstractmethod def request_readings(self) -> None: """Request new sensor readings from the device.""" diff --git a/tests/hardware/plugins/sensors/test_sensors_base.py b/tests/hardware/plugins/sensors/test_sensors_base.py index 3f6b9e94..0f872992 100644 --- a/tests/hardware/plugins/sensors/test_sensors_base.py +++ b/tests/hardware/plugins/sensors/test_sensors_base.py @@ -46,7 +46,6 @@ def test_start_polling_oneshot(timer_mock: Mock) -> None: device = _MockSensorsDevice(start_polling=False) device.start_polling() - device.request_readings_mock.assert_called_once_with() timer = cast(Mock, device._poll_timer) timer.start.assert_not_called() @@ -57,7 +56,6 @@ def test_start_polling_repeated(timer_mock: Mock) -> None: device = _MockSensorsDevice(1.0, start_polling=False) device.start_polling() - device.request_readings_mock.assert_called_once_with() timer = cast(Mock, device._poll_timer) timer.start.assert_called_once_with(1000) @@ -68,7 +66,6 @@ def test_init_no_timer(timer_mock: Mock) -> None: device = _MockSensorsDevice() timer = cast(Mock, device._poll_timer) timer.start.assert_not_called() - device.request_readings_mock.assert_called_once_with() def test_send_readings_message(device: SensorsBase) -> None: From f38bd5b763c70cce13cc1cf66575ae153c00c7a4 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Thu, 24 Oct 2024 16:57:42 +0100 Subject: [PATCH 2/6] Decades: Fix: Call super().__init__() before requesting params Technically this is a race condition as it stands. --- finesse/hardware/plugins/sensors/decades.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/finesse/hardware/plugins/sensors/decades.py b/finesse/hardware/plugins/sensors/decades.py index 8e33ca3c..50842587 100644 --- a/finesse/hardware/plugins/sensors/decades.py +++ b/finesse/hardware/plugins/sensors/decades.py @@ -119,14 +119,13 @@ def __init__( self._params: list[DecadesParameter] """Parameters returned by the server.""" + super().__init__(poll_interval, start_polling=False) + # Obtain full parameter list in order to parse received data self.obtain_parameter_list( frozenset(params.split(",")) if params else frozenset() ) - # We only want to start polling after we have loaded the parameter list - super().__init__(poll_interval, start_polling=False) - def obtain_parameter_list(self, params: Set[str]) -> None: """Request the parameter list from the DECADES server and wait for response.""" self._requester.make_request( From 3a2cc7b2889cf03a8865de9ef6f9f54939e6583d Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Thu, 24 Oct 2024 16:59:56 +0100 Subject: [PATCH 3/6] Decades: Wait until params are received before signalling device is open Closes #666. --- finesse/hardware/plugins/sensors/decades.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/finesse/hardware/plugins/sensors/decades.py b/finesse/hardware/plugins/sensors/decades.py index 50842587..9e154838 100644 --- a/finesse/hardware/plugins/sensors/decades.py +++ b/finesse/hardware/plugins/sensors/decades.py @@ -97,6 +97,7 @@ class Decades( "documentation." ), }, + async_open=True, ): """A class for monitoring a DECADES sensor server.""" @@ -204,6 +205,9 @@ def _on_params_received(self, reply: QNetworkReply, params: Set[str]) -> None: else: self._params = list(_get_selected_params(all_params_info, params)) + # Tell the frontend that the device is ready + self.signal_is_opened() + # Now we have enough information to start parsing sensor readings self.start_polling() self.request_readings() From d7251d298b57e21980a92cbda909592eeba265f1 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Thu, 24 Oct 2024 17:06:19 +0100 Subject: [PATCH 4/6] SensorsBase: Remove unneeded start_polling argument All the callers of SensorsBase.__init__() now call it with start_polling=False, so we can just remove the option. --- finesse/hardware/plugins/sensors/decades.py | 2 +- .../hardware/plugins/sensors/em27_sensors.py | 2 +- .../hardware/plugins/sensors/sensors_base.py | 8 +----- .../plugins/sensors/test_sensors_base.py | 25 +++++++------------ 4 files changed, 12 insertions(+), 25 deletions(-) diff --git a/finesse/hardware/plugins/sensors/decades.py b/finesse/hardware/plugins/sensors/decades.py index 9e154838..3dcb6e79 100644 --- a/finesse/hardware/plugins/sensors/decades.py +++ b/finesse/hardware/plugins/sensors/decades.py @@ -120,7 +120,7 @@ def __init__( self._params: list[DecadesParameter] """Parameters returned by the server.""" - super().__init__(poll_interval, start_polling=False) + super().__init__(poll_interval) # Obtain full parameter list in order to parse received data self.obtain_parameter_list( diff --git a/finesse/hardware/plugins/sensors/em27_sensors.py b/finesse/hardware/plugins/sensors/em27_sensors.py index 1e9f6163..6a5d290c 100644 --- a/finesse/hardware/plugins/sensors/em27_sensors.py +++ b/finesse/hardware/plugins/sensors/em27_sensors.py @@ -69,7 +69,7 @@ def __init__(self, url: str, poll_interval: float = float("nan")) -> None: self._requester = HTTPRequester() self._connected = False - super().__init__(poll_interval, start_polling=False) + super().__init__(poll_interval) self.request_readings() diff --git a/finesse/hardware/plugins/sensors/sensors_base.py b/finesse/hardware/plugins/sensors/sensors_base.py index 4256060d..103a6572 100644 --- a/finesse/hardware/plugins/sensors/sensors_base.py +++ b/finesse/hardware/plugins/sensors/sensors_base.py @@ -23,15 +23,12 @@ class SensorsBase( calling send_readings_message() with new sensor values (at some point). """ - def __init__( - self, poll_interval: float = float("nan"), start_polling: bool = True - ) -> None: + def __init__(self, poll_interval: float = float("nan")) -> None: """Create a new SensorsBase. Args: poll_interval: How often to poll the sensor device (seconds). If set to nan, the device will only be polled once on device open - start_polling: Whether to start polling the device immediately """ super().__init__() @@ -39,9 +36,6 @@ def __init__( self._poll_timer.timeout.connect(self.request_readings) self._poll_interval = poll_interval - if start_polling: - self.start_polling() - def start_polling(self) -> None: """Begin polling the device.""" if not isnan(self._poll_interval): diff --git a/tests/hardware/plugins/sensors/test_sensors_base.py b/tests/hardware/plugins/sensors/test_sensors_base.py index 0f872992..04362c5c 100644 --- a/tests/hardware/plugins/sensors/test_sensors_base.py +++ b/tests/hardware/plugins/sensors/test_sensors_base.py @@ -16,34 +16,27 @@ def device(timer_mock: Mock) -> SensorsBase: class _MockSensorsDevice(SensorsBase, description="Mock sensors device"): - def __init__(self, poll_interval: float = float("nan"), start_polling=True): + def __init__(self, poll_interval: float = float("nan")): self.request_readings_mock = MagicMock() - super().__init__(poll_interval, start_polling) + super().__init__(poll_interval) def request_readings(self) -> None: self.request_readings_mock() -@pytest.mark.parametrize("start_polling", (False, True)) @patch("finesse.hardware.plugins.sensors.sensors_base.QTimer") -def test_init(timer_mock: Mock, start_polling: bool) -> None: +def test_init(timer_mock: Mock) -> None: """Test for the constructor.""" - with patch.object(_MockSensorsDevice, "start_polling") as start_mock: - device = _MockSensorsDevice(1.0, start_polling) - assert device._poll_interval == 1.0 - timer = cast(Mock, device._poll_timer) - timer.timeout.connect.assert_called_once_with(device.request_readings) - - if start_polling: - start_mock.assert_called_once_with() - else: - start_mock.assert_not_called() + device = _MockSensorsDevice(1.0) + assert device._poll_interval == 1.0 + timer = cast(Mock, device._poll_timer) + timer.timeout.connect.assert_called_once_with(device.request_readings) @patch("finesse.hardware.plugins.sensors.sensors_base.QTimer") def test_start_polling_oneshot(timer_mock: Mock) -> None: """Test the start_polling() method when polling is only done once.""" - device = _MockSensorsDevice(start_polling=False) + device = _MockSensorsDevice() device.start_polling() timer = cast(Mock, device._poll_timer) @@ -53,7 +46,7 @@ def test_start_polling_oneshot(timer_mock: Mock) -> None: @patch("finesse.hardware.plugins.sensors.sensors_base.QTimer") def test_start_polling_repeated(timer_mock: Mock) -> None: """Test the start_polling() method when polling is only done repeatedly.""" - device = _MockSensorsDevice(1.0, start_polling=False) + device = _MockSensorsDevice(1.0) device.start_polling() timer = cast(Mock, device._poll_timer) From ca4426087c0538a1347c2de7d537b3b33b7a937d Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Thu, 24 Oct 2024 17:17:49 +0100 Subject: [PATCH 5/6] OPUSInterface: Wait for response from EM27 before signalling device is open Closes #667. --- finesse/hardware/plugins/spectrometer/opus_interface.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/finesse/hardware/plugins/spectrometer/opus_interface.py b/finesse/hardware/plugins/spectrometer/opus_interface.py index 34a64d1f..ffe645d5 100644 --- a/finesse/hardware/plugins/spectrometer/opus_interface.py +++ b/finesse/hardware/plugins/spectrometer/opus_interface.py @@ -71,6 +71,7 @@ class OPUSInterface( "This is rate limited to around one request every two seconds by OPUS." ), }, + async_open=True, ): """Interface for communicating with the OPUS program. @@ -122,6 +123,10 @@ def _on_reply_received(self, reply: QNetworkReply) -> None: # If the status has changed, notify listeners if new_status != self._status: + # On first update, we need to signal that the device is now open + if self._status == SpectrometerStatus.UNDEFINED: + self.signal_is_opened() + self._status = new_status self.send_status_message(new_status) From 9e5d3adb392e1084a83bf58aa3fcaf7c160cc90b Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Wed, 11 Dec 2024 09:39:58 +0000 Subject: [PATCH 6/6] OpusInterface: Set initial status to `None` Previously we were using `SpectrometerStatus.UNDEFINED` as the initial value, but this is a bad idea because: 1. It's a bit ugly 2. The server could conceivably return `UNDEFINED` as a status, which would break our code -- probably best not to rely on this not happening --- finesse/hardware/plugins/spectrometer/opus_interface.py | 4 ++-- tests/hardware/plugins/spectrometer/test_opus_interface.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/finesse/hardware/plugins/spectrometer/opus_interface.py b/finesse/hardware/plugins/spectrometer/opus_interface.py index ffe645d5..c9834f46 100644 --- a/finesse/hardware/plugins/spectrometer/opus_interface.py +++ b/finesse/hardware/plugins/spectrometer/opus_interface.py @@ -96,7 +96,7 @@ def __init__( self._url = f"http://{host}:{port}/opusrs" """URL to make requests.""" - self._status = SpectrometerStatus.UNDEFINED + self._status: SpectrometerStatus | None = None """The last known status of the spectrometer.""" self._status_timer = QTimer() self._status_timer.timeout.connect(self._request_status) @@ -124,7 +124,7 @@ def _on_reply_received(self, reply: QNetworkReply) -> None: # If the status has changed, notify listeners if new_status != self._status: # On first update, we need to signal that the device is now open - if self._status == SpectrometerStatus.UNDEFINED: + if self._status is None: self.signal_is_opened() self._status = new_status diff --git a/tests/hardware/plugins/spectrometer/test_opus_interface.py b/tests/hardware/plugins/spectrometer/test_opus_interface.py index 7e712924..0ffcb05b 100644 --- a/tests/hardware/plugins/spectrometer/test_opus_interface.py +++ b/tests/hardware/plugins/spectrometer/test_opus_interface.py @@ -36,7 +36,7 @@ def test_init(timer_mock: Mock, subscribe_mock: Mock) -> None: assert opus._url == f"http://{DEFAULT_OPUS_HOST}:{DEFAULT_OPUS_PORT}/opusrs" status_mock.assert_called_once_with() - assert opus._status == SpectrometerStatus.UNDEFINED + assert opus._status is None timer.setSingleShot.assert_called_once_with(True) timer.setInterval.assert_called_once_with(1000) timer.timeout.connect.assert_called_once_with(opus._request_status)