From 165a7b77946d60072e719f78faa2a2195cc0887a Mon Sep 17 00:00:00 2001 From: Daniel Cummins Date: Wed, 11 Dec 2024 11:56:52 +0000 Subject: [PATCH 1/8] Remove 'return None' from angle property --- .../stepper_motor/stepper_motor_base.py | 21 +++++-------------- .../stepper_motor/test_stepper_motor_base.py | 2 +- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/finesse/hardware/plugins/stepper_motor/stepper_motor_base.py b/finesse/hardware/plugins/stepper_motor/stepper_motor_base.py index d8982770..2a72b6d3 100644 --- a/finesse/hardware/plugins/stepper_motor/stepper_motor_base.py +++ b/finesse/hardware/plugins/stepper_motor/stepper_motor_base.py @@ -41,12 +41,8 @@ def steps_per_rotation(self) -> int: @property @abstractmethod - def step(self) -> int | None: - """The current state of the device's step counter. - - As this can only be requested when the motor is stationary, if the motor is - moving then None will be returned. - """ + def step(self) -> int: + """The current state of the device's step counter.""" @step.setter @abstractmethod @@ -67,20 +63,13 @@ def is_moving(self) -> bool: """Whether the motor is currently moving.""" @property - def angle(self) -> float | None: + def angle(self) -> float: """The current angle of the motor in degrees. - As this can only be requested when the motor is stationary, if the motor is - moving then None will be returned. - Returns: - The current angle or None if the stepper motor is moving + The current angle """ - step = self.step - if step is None: - return None - - return step * 360.0 / self.steps_per_rotation + return self.step * 360.0 / self.steps_per_rotation def move_to(self, target: float | str) -> None: """Move the motor to a specified rotation and send message when complete. diff --git a/tests/hardware/plugins/stepper_motor/test_stepper_motor_base.py b/tests/hardware/plugins/stepper_motor/test_stepper_motor_base.py index c113eeb9..60b685bd 100644 --- a/tests/hardware/plugins/stepper_motor/test_stepper_motor_base.py +++ b/tests/hardware/plugins/stepper_motor/test_stepper_motor_base.py @@ -22,7 +22,7 @@ def is_moving(self) -> bool: return False @property - def step(self) -> int | None: + def step(self) -> int: return self._step @step.setter From ffaf60dbb6011240a7e0ce773d7dd0bd982ebf60 Mon Sep 17 00:00:00 2001 From: Daniel Cummins Date: Wed, 11 Dec 2024 13:07:26 +0000 Subject: [PATCH 2/8] Request mirror position with 'IP' rather than 'SP' --- .../plugins/stepper_motor/st10_controller.py | 12 +++++------ .../stepper_motor/test_st10_controller.py | 20 +++++++------------ 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/finesse/hardware/plugins/stepper_motor/st10_controller.py b/finesse/hardware/plugins/stepper_motor/st10_controller.py index 79aa228e..f90527a2 100644 --- a/finesse/hardware/plugins/stepper_motor/st10_controller.py +++ b/finesse/hardware/plugins/stepper_motor/st10_controller.py @@ -335,21 +335,19 @@ def is_moving(self) -> bool: return self.status_code & 0x0010 == 0x0010 @property - def step(self) -> int | None: + def step(self) -> int: """The current state of the device's step counter. - As this can only be requested when the motor is stationary, if the motor is - moving then None will be returned. + This makes use of the "IP" command, which estimates the immediate position of + the motor. If the motor is moving, this is an estimated (calculated trajectory) + position. If the motor is stationary, this is the actual position. Raises: SerialException: Error communicating with device SerialTimeoutException: Timed out waiting for response from device ST10ControllerError: Malformed message received from device """ - if self.is_moving: - return None - - step = self._request_value("SP") + step = self._request_value("IP") try: return int(step) except ValueError: diff --git a/tests/hardware/plugins/stepper_motor/test_st10_controller.py b/tests/hardware/plugins/stepper_motor/test_st10_controller.py index cf8dac9d..1c3dd9c4 100644 --- a/tests/hardware/plugins/stepper_motor/test_st10_controller.py +++ b/tests/hardware/plugins/stepper_motor/test_st10_controller.py @@ -365,31 +365,25 @@ def test_is_moving( "finesse.hardware.plugins.stepper_motor.st10_controller.ST10Controller.is_moving", new_callable=PropertyMock, ) -def test_get_step_not_moving( +def test_get_step( is_moving_mock: PropertyMock, step: int, response: str, raises: Any, dev: ST10Controller, ) -> None: - """Test getting the step property when the motor is stationary.""" + """Test getting the step property.""" + # When the motor is stationary: is_moving_mock.return_value = False with read_mock(dev, response): with raises: assert dev.step == step - -@patch( - "finesse.hardware.plugins.stepper_motor.st10_controller.ST10Controller.is_moving", - new_callable=PropertyMock, -) -def test_get_step_moving( - is_moving_mock: PropertyMock, - dev: ST10Controller, -) -> None: - """Test getting the step property when the motor is moving.""" + # When the motor is moving: is_moving_mock.return_value = True - assert dev.step is None + with read_mock(dev, response): + with raises: + assert dev.step == step @pytest.mark.parametrize("step", range(0, 40, 7)) From ec79eef52712744a6c68809727dc9c631cfd31ea Mon Sep 17 00:00:00 2001 From: Daniel Cummins Date: Wed, 11 Dec 2024 18:16:13 +0000 Subject: [PATCH 3/8] Fix test with 'SP' -> 'IP' --- .../hardware/plugins/stepper_motor/test_st10_controller.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/hardware/plugins/stepper_motor/test_st10_controller.py b/tests/hardware/plugins/stepper_motor/test_st10_controller.py index 1c3dd9c4..de914576 100644 --- a/tests/hardware/plugins/stepper_motor/test_st10_controller.py +++ b/tests/hardware/plugins/stepper_motor/test_st10_controller.py @@ -240,7 +240,7 @@ def test_write_check(dev: ST10Controller) -> None: if response.startswith(f"{name}=") else pytest.raises(ST10ControllerError), ) - for name in ["hello", "IS", "SP"] + for name in ["hello", "IS", "IP"] for value in ["", "value", "123"] for response in [f"{name}={value}", value, "%", "*", "?4"] ], @@ -357,8 +357,8 @@ def test_is_moving( @pytest.mark.parametrize( "step,response,raises", chain( - [(step, f"SP={step}", does_not_raise()) for step in range(0, 250, 50)], - [(4, "SP=hello", pytest.raises(ST10ControllerError))], + [(step, f"IP={step}", does_not_raise()) for step in range(0, 250, 50)], + [(4, "IP=hello", pytest.raises(ST10ControllerError))], ), ) @patch( From 78f0424758d90f9bd9587ba2ac0a3f3b8601e2ed Mon Sep 17 00:00:00 2001 From: Daniel Cummins Date: Fri, 13 Dec 2024 08:34:23 +0000 Subject: [PATCH 4/8] Removed unused is_moving_mock --- .../plugins/stepper_motor/test_st10_controller.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/tests/hardware/plugins/stepper_motor/test_st10_controller.py b/tests/hardware/plugins/stepper_motor/test_st10_controller.py index de914576..f382bf0e 100644 --- a/tests/hardware/plugins/stepper_motor/test_st10_controller.py +++ b/tests/hardware/plugins/stepper_motor/test_st10_controller.py @@ -361,26 +361,13 @@ def test_is_moving( [(4, "IP=hello", pytest.raises(ST10ControllerError))], ), ) -@patch( - "finesse.hardware.plugins.stepper_motor.st10_controller.ST10Controller.is_moving", - new_callable=PropertyMock, -) def test_get_step( - is_moving_mock: PropertyMock, step: int, response: str, raises: Any, dev: ST10Controller, ) -> None: """Test getting the step property.""" - # When the motor is stationary: - is_moving_mock.return_value = False - with read_mock(dev, response): - with raises: - assert dev.step == step - - # When the motor is moving: - is_moving_mock.return_value = True with read_mock(dev, response): with raises: assert dev.step == step From 9dade8f2b5dce2defa002315e2d1a16e6e633a8a Mon Sep 17 00:00:00 2001 From: Daniel Cummins Date: Fri, 13 Dec 2024 08:47:36 +0000 Subject: [PATCH 5/8] Removed writing IsMoving to csv data file --- finesse/hardware/data_file_writer.py | 20 +++++++------------- tests/hardware/test_data_file_writer.py | 3 +-- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/finesse/hardware/data_file_writer.py b/finesse/hardware/data_file_writer.py index 2d7adac7..bb2d4ae8 100644 --- a/finesse/hardware/data_file_writer.py +++ b/finesse/hardware/data_file_writer.py @@ -56,7 +56,6 @@ def _create_writer(path: Path) -> Writer: *(f"Temp{i+1}" for i in range(config.NUM_TEMPERATURE_MONITOR_CHANNELS)), "TimeAsSeconds", "Angle", - "IsMoving", "TemperatureControllerPower", ) ) @@ -64,28 +63,24 @@ def _create_writer(path: Path) -> Writer: return writer -def _get_stepper_motor_angle() -> tuple[float, bool]: +def _get_stepper_motor_angle() -> float: """Get the current angle of the stepper motor. - This function returns a float indicating the angle in degrees and a boolean - indicating whether the motor is currently moving. If an error occurs or the motor is - moving, the angle returned will be nan. + This function returns a float indicating the angle in degrees. If an error occurs, + the angle returned will be nan. """ stepper = get_stepper_motor_instance() # Stepper motor not connected if not stepper: - return (float("nan"), False) + return float("nan") try: angle = stepper.angle - if angle is None: - return (float("nan"), True) - else: - return (angle, False) + return angle except Exception as error: stepper.send_error_message(error) - return (float("nan"), False) + return float("nan") def _get_hot_bb_power() -> float: @@ -170,7 +165,7 @@ def write(self, time: datetime, temperatures: list[Decimal]) -> None: midnight = datetime(time.year, time.month, time.day) secs_since_midnight = floor((time - midnight).total_seconds()) - angle, is_moving = _get_stepper_motor_angle() + angle = _get_stepper_motor_angle() self._writer.writerow( ( time.strftime("%Y%m%d"), @@ -178,7 +173,6 @@ def write(self, time: datetime, temperatures: list[Decimal]) -> None: *(round(t, config.TEMPERATURE_PRECISION) for t in temperatures), secs_since_midnight, angle, - int(is_moving), _get_hot_bb_power(), ) ) diff --git a/tests/hardware/test_data_file_writer.py b/tests/hardware/test_data_file_writer.py index bee7a5e5..0587da91 100644 --- a/tests/hardware/test_data_file_writer.py +++ b/tests/hardware/test_data_file_writer.py @@ -57,7 +57,6 @@ def test_open( "Temp2", "TimeAsSeconds", "Angle", - "IsMoving", "TemperatureControllerPower", ) ) @@ -138,7 +137,7 @@ def test_write( writer._writer = MagicMock() writer.write(time, data) writer._writer.writerow.assert_called_once_with( - ("20230414", "00:01:00", *data, 60, 90.0, False, 10) + ("20230414", "00:01:00", *data, 60, 90.0, 10) ) sendmsg_mock.assert_called_once_with("data_file.writing") From d3095fc5c2b86213babadfecadaed91d93860092 Mon Sep 17 00:00:00 2001 From: Daniel Cummins Date: Fri, 13 Dec 2024 09:35:54 +0000 Subject: [PATCH 6/8] Revert "Removed writing IsMoving to csv data file" This reverts commit 9dade8f2b5dce2defa002315e2d1a16e6e633a8a. --- finesse/hardware/data_file_writer.py | 20 +++++++++++++------- tests/hardware/test_data_file_writer.py | 3 ++- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/finesse/hardware/data_file_writer.py b/finesse/hardware/data_file_writer.py index bb2d4ae8..2d7adac7 100644 --- a/finesse/hardware/data_file_writer.py +++ b/finesse/hardware/data_file_writer.py @@ -56,6 +56,7 @@ def _create_writer(path: Path) -> Writer: *(f"Temp{i+1}" for i in range(config.NUM_TEMPERATURE_MONITOR_CHANNELS)), "TimeAsSeconds", "Angle", + "IsMoving", "TemperatureControllerPower", ) ) @@ -63,24 +64,28 @@ def _create_writer(path: Path) -> Writer: return writer -def _get_stepper_motor_angle() -> float: +def _get_stepper_motor_angle() -> tuple[float, bool]: """Get the current angle of the stepper motor. - This function returns a float indicating the angle in degrees. If an error occurs, - the angle returned will be nan. + This function returns a float indicating the angle in degrees and a boolean + indicating whether the motor is currently moving. If an error occurs or the motor is + moving, the angle returned will be nan. """ stepper = get_stepper_motor_instance() # Stepper motor not connected if not stepper: - return float("nan") + return (float("nan"), False) try: angle = stepper.angle - return angle + if angle is None: + return (float("nan"), True) + else: + return (angle, False) except Exception as error: stepper.send_error_message(error) - return float("nan") + return (float("nan"), False) def _get_hot_bb_power() -> float: @@ -165,7 +170,7 @@ def write(self, time: datetime, temperatures: list[Decimal]) -> None: midnight = datetime(time.year, time.month, time.day) secs_since_midnight = floor((time - midnight).total_seconds()) - angle = _get_stepper_motor_angle() + angle, is_moving = _get_stepper_motor_angle() self._writer.writerow( ( time.strftime("%Y%m%d"), @@ -173,6 +178,7 @@ def write(self, time: datetime, temperatures: list[Decimal]) -> None: *(round(t, config.TEMPERATURE_PRECISION) for t in temperatures), secs_since_midnight, angle, + int(is_moving), _get_hot_bb_power(), ) ) diff --git a/tests/hardware/test_data_file_writer.py b/tests/hardware/test_data_file_writer.py index 0587da91..bee7a5e5 100644 --- a/tests/hardware/test_data_file_writer.py +++ b/tests/hardware/test_data_file_writer.py @@ -57,6 +57,7 @@ def test_open( "Temp2", "TimeAsSeconds", "Angle", + "IsMoving", "TemperatureControllerPower", ) ) @@ -137,7 +138,7 @@ def test_write( writer._writer = MagicMock() writer.write(time, data) writer._writer.writerow.assert_called_once_with( - ("20230414", "00:01:00", *data, 60, 90.0, 10) + ("20230414", "00:01:00", *data, 60, 90.0, False, 10) ) sendmsg_mock.assert_called_once_with("data_file.writing") From 8e1be124239973446de8d6cb9d4bd67166354d26 Mon Sep 17 00:00:00 2001 From: Daniel Cummins Date: Fri, 13 Dec 2024 11:54:14 +0000 Subject: [PATCH 7/8] Explicitly check if stepper motor is moving --- finesse/hardware/data_file_writer.py | 10 ++++----- tests/hardware/test_data_file_writer.py | 29 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/finesse/hardware/data_file_writer.py b/finesse/hardware/data_file_writer.py index 2d7adac7..d0315ad0 100644 --- a/finesse/hardware/data_file_writer.py +++ b/finesse/hardware/data_file_writer.py @@ -68,8 +68,8 @@ def _get_stepper_motor_angle() -> tuple[float, bool]: """Get the current angle of the stepper motor. This function returns a float indicating the angle in degrees and a boolean - indicating whether the motor is currently moving. If an error occurs or the motor is - moving, the angle returned will be nan. + indicating whether the motor is currently moving. If an error occurs, the angle + returned will be nan. """ stepper = get_stepper_motor_instance() @@ -79,10 +79,8 @@ def _get_stepper_motor_angle() -> tuple[float, bool]: try: angle = stepper.angle - if angle is None: - return (float("nan"), True) - else: - return (angle, False) + is_moving = stepper.is_moving + return (angle, is_moving) except Exception as error: stepper.send_error_message(error) return (float("nan"), False) diff --git a/tests/hardware/test_data_file_writer.py b/tests/hardware/test_data_file_writer.py index bee7a5e5..646b7e1d 100644 --- a/tests/hardware/test_data_file_writer.py +++ b/tests/hardware/test_data_file_writer.py @@ -129,6 +129,7 @@ def test_write( """Test the write() method.""" get_stepper_mock.return_value = stepper = MagicMock() stepper.angle = 90.0 + stepper.is_moving = False get_tc_mock.return_value = hot_bb = MagicMock() hot_bb.power = 10 @@ -144,6 +145,33 @@ def test_write( sendmsg_mock.assert_called_once_with("data_file.writing") +@patch("finesse.hardware.data_file_writer.get_temperature_controller_instance") +@patch("finesse.hardware.data_file_writer.get_stepper_motor_instance") +def test_write_moving( + get_stepper_mock: Mock, + get_tc_mock: Mock, + writer: DataFileWriter, + sendmsg_mock: Mock, +) -> None: + """Test the write() method.""" + get_stepper_mock.return_value = stepper = MagicMock() + stepper.angle = 95.0 + stepper.is_moving = True + get_tc_mock.return_value = hot_bb = MagicMock() + hot_bb.power = 10 + + time = datetime(2023, 4, 14, 0, 1, 0) # one minute past midnight + data = [Decimal(i) for i in range(3)] + + writer._writer = MagicMock() + writer.write(time, data) + writer._writer.writerow.assert_called_once_with( + ("20230414", "00:01:00", *data, 60, 95.0, True, 10) + ) + + sendmsg_mock.assert_called_once_with("data_file.writing") + + @patch("finesse.hardware.data_file_writer.get_temperature_controller_instance") @patch("finesse.hardware.data_file_writer.get_stepper_motor_instance") def test_write_error( @@ -155,6 +183,7 @@ def test_write_error( """Test the write() method when an error occurs.""" get_stepper_mock.return_value = stepper = MagicMock() stepper.angle = 90.0 + stepper.angle = False get_tc_mock.return_value = hot_bb = MagicMock() hot_bb.power = 10 From 9ec257ac4ff60377d737fde8195ff28ac5d21c06 Mon Sep 17 00:00:00 2001 From: Daniel Cummins Date: Fri, 13 Dec 2024 11:55:36 +0000 Subject: [PATCH 8/8] Update docstring --- tests/hardware/test_data_file_writer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/hardware/test_data_file_writer.py b/tests/hardware/test_data_file_writer.py index 646b7e1d..c4fad9da 100644 --- a/tests/hardware/test_data_file_writer.py +++ b/tests/hardware/test_data_file_writer.py @@ -153,7 +153,7 @@ def test_write_moving( writer: DataFileWriter, sendmsg_mock: Mock, ) -> None: - """Test the write() method.""" + """Test the write() method when the stepper motor is moving.""" get_stepper_mock.return_value = stepper = MagicMock() stepper.angle = 95.0 stepper.is_moving = True