Get moveonclick to work (#194)

* temporary moveonclick workaround and zoomcalibrator calculation

* Actually pass the oav device to the function

* Temporary fix for test

* Tidy up

* Do not start the gui in a RunEngine but pass it around

* Tidy up a bit

* Try to fix test

* Fix test

* Tidy up again

* Add a zoomcalibrator calculation test for commonly used zoom levels

* Address comment

* Remove unused function

---------

Co-authored-by: Robin Owen <robin.owen@diamond.ac.uk>

src/mx_bluesky/beamlines/i24/serial/fixed_target/i24ssx_moveonclick.py

@@ -4,6 +4,7 @@
 """
 
 import logging
+from collections.abc import Sequence
 
 import bluesky.plan_stubs as bps
 import cv2 as cv
@@ -20,10 +21,6 @@
 
 logger = logging.getLogger("I24ssx.moveonclick")
 
-# Set scale.
-# TODO See https://github.com/DiamondLightSource/mx_bluesky/issues/44
-zoomcalibrator = 6  # 8 seems to work well for zoom 2
-
 
 def _get_beam_centre(oav: OAV):
     """Extract the beam centre x/y positions from the display.configuration file.
@@ -34,20 +31,40 @@ def _get_beam_centre(oav: OAV):
     return oav.parameters.beam_centre_i, oav.parameters.beam_centre_j
 
 
+def _calculate_zoom_calibrator(oav: OAV):
+    """Set the scale for the zoom calibrator for the pmac moves."""
+    currentzoom = yield from bps.rd(oav.zoom_controller.percentage)
+    zoomcalibrator = 1.547 - (0.03 * currentzoom) + (0.0001634 * currentzoom**2)
+    return zoomcalibrator
+
+
+def _move_on_mouse_click_plan(
+    oav: OAV, pmac: PMAC, beam_centre: Sequence[int], clicked_position: Sequence[int]
+):
+    """A plan that calculates the zoom calibrator and moves to the clicked \
+        position coordinates.
+    """
+    zoomcalibrator = yield from _calculate_zoom_calibrator(oav)
+    beamX, beamY = beam_centre
+    x, y = clicked_position
+    xmove = -1 * (beamX - x) * zoomcalibrator
+    ymove = -1 * (beamY - y) * zoomcalibrator
+    logger.info(f"Moving X and Y {xmove} {ymove}")
+    xmovepmacstring = "#1J:" + str(xmove)
+    ymovepmacstring = "#2J:" + str(ymove)
+    yield from bps.abs_set(pmac.pmac_string, xmovepmacstring, wait=True)
+    yield from bps.abs_set(pmac.pmac_string, ymovepmacstring, wait=True)
+
+
 # Register clicks and move chip stages
 def onMouse(event, x, y, flags, param):
     if event == cv.EVENT_LBUTTONUP:
-        pmac = param[0]
-        oav = param[1]
+        RE = param[0]
+        pmac = param[1]
+        oav = param[2]
         beamX, beamY = _get_beam_centre(oav)
         logger.info(f"Clicked X and Y {x} {y}")
-        xmove = -1 * (beamX - x) * zoomcalibrator
-        ymove = -1 * (beamY - y) * zoomcalibrator
-        logger.info(f"Moving X and Y {xmove} {ymove}")
-        xmovepmacstring = "#1J:" + str(xmove)
-        ymovepmacstring = "#2J:" + str(ymove)
-        yield from bps.abs_set(pmac.pmac_string, xmovepmacstring, wait=True)
-        yield from bps.abs_set(pmac.pmac_string, ymovepmacstring, wait=True)
+        RE(_move_on_mouse_click_plan(oav, pmac, (beamX, beamY), (x, y)))
 
 
 def update_ui(oav, frame):
@@ -58,7 +75,6 @@ def update_ui(oav, frame):
     cv.ellipse(
         frame, (beamX, beamY), (12, 8), 0.0, 0.0, 360, (0, 255, 255), thickness=2
     )
-    # putText(frame,'text',bottomLeftCornerOfText, font, fontScale, fontColor, thickness, lineType)
     cv.putText(
         frame,
         "Key bindings",
@@ -132,13 +148,13 @@ def update_ui(oav, frame):
     cv.imshow("OAV1view", frame)
 
 
-def start_viewer(oav: OAV, pmac: PMAC, oav1: str = OAV1_CAM):
+def start_viewer(oav: OAV, pmac: PMAC, RE: RunEngine, oav1: str = OAV1_CAM):
     # Create a video caputure from OAV1
     cap = cv.VideoCapture(oav1)
 
     # Create window named OAV1view and set onmouse to this
     cv.namedWindow("OAV1view")
-    cv.setMouseCallback("OAV1view", onMouse, param=[pmac, oav])  # type: ignore
+    cv.setMouseCallback("OAV1view", onMouse, param=[RE, pmac, oav])  # type: ignore
 
     logger.info("Showing camera feed. Press escape to close")
     # Read captured video and store them in success and frame
@@ -152,38 +168,40 @@ def start_viewer(oav: OAV, pmac: PMAC, oav1: str = OAV1_CAM):
 
         k = cv.waitKey(1)
         if k == 113:  # Q
-            yield from manager.moveto(Fiducials.zero, pmac)
+            RE(manager.moveto(Fiducials.zero, pmac))
         if k == 119:  # W
-            yield from manager.moveto(Fiducials.fid1, pmac)
+            RE(manager.moveto(Fiducials.fid1, pmac))
         if k == 101:  # E
-            yield from manager.moveto(Fiducials.fid2, pmac)
+            RE(manager.moveto(Fiducials.fid2, pmac))
         if k == 97:  # A
-            yield from bps.trigger(pmac.home, wait=True)
+            RE(bps.trigger(pmac.home, wait=True))
             print("Current position set as origin")
         if k == 115:  # S
-            yield from manager.fiducial(1)
+            RE(manager.fiducial(1))
         if k == 100:  # D
-            yield from manager.fiducial(2)
+            RE(manager.fiducial(2))
         if k == 99:  # C
-            yield from manager.cs_maker(pmac)
+            RE(manager.cs_maker(pmac))
         if k == 98:  # B
-            yield from manager.block_check()  # doesn't work well for blockcheck as image doesn't update
+            RE(
+                manager.block_check()
+            )  # doesn't work well for blockcheck as image doesn't update
         if k == 104:  # H
-            yield from bps.abs_set(pmac.pmac_string, "#2J:-10", wait=True)
+            RE(bps.abs_set(pmac.pmac_string, "#2J:-10", wait=True))
         if k == 110:  # N
-            yield from bps.abs_set(pmac.pmac_string, "#2J:10", wait=True)
+            RE(bps.abs_set(pmac.pmac_string, "#2J:10", wait=True))
         if k == 109:  # M
-            yield from bps.abs_set(pmac.pmac_string, "#1J:-10", wait=True)
+            RE(bps.abs_set(pmac.pmac_string, "#1J:-10", wait=True))
         if k == 98:  # B
-            yield from bps.abs_set(pmac.pmac_string, "#1J:10", wait=True)
+            RE(bps.abs_set(pmac.pmac_string, "#1J:10", wait=True))
         if k == 105:  # I
-            yield from bps.abs_set(pmac.pmac_string, "#3J:-150", wait=True)
+            RE(bps.abs_set(pmac.pmac_string, "#3J:-150", wait=True))
         if k == 111:  # O
-            yield from bps.abs_set(pmac.pmac_string, "#3J:150", wait=True)
+            RE(bps.abs_set(pmac.pmac_string, "#3J:150", wait=True))
         if k == 117:  # U
-            yield from bps.abs_set(pmac.pmac_string, "#3J:-1000", wait=True)
+            RE(bps.abs_set(pmac.pmac_string, "#3J:-1000", wait=True))
         if k == 112:  # P
-            yield from bps.abs_set(pmac.pmac_string, "#3J:1000", wait=True)
+            RE(bps.abs_set(pmac.pmac_string, "#3J:1000", wait=True))
         if k == 0x1B:  # esc
             cv.destroyWindow("OAV1view")
             print("Pressed escape. Closing window")
@@ -198,4 +216,4 @@ def start_viewer(oav: OAV, pmac: PMAC, oav1: str = OAV1_CAM):
     # Get devices out of dodal
     oav: OAV = i24.oav()
     pmac: PMAC = i24.pmac()
-    RE(start_viewer(oav, pmac))
+    start_viewer(oav, pmac, RE)

tests/unit_tests/beamlines/i24/serial/fixed_target/test_moveonclick.py

@@ -1,52 +1,65 @@
 from unittest.mock import ANY, MagicMock, call, patch
 
+import bluesky.plan_stubs as bps
 import cv2 as cv
 import pytest
 from dodal.devices.i24.pmac import PMAC
 from dodal.devices.oav.oav_detector import OAV
 from ophyd_async.core import get_mock_put
 
 from mx_bluesky.beamlines.i24.serial.fixed_target.i24ssx_moveonclick import (
+    _calculate_zoom_calibrator,
     onMouse,
     update_ui,
-    zoomcalibrator,
 )
 
+ZOOMCALIBRATOR = 6
+
+
+def fake_generator(value):
+    yield from bps.null()
+    return value
+
 
 @pytest.mark.parametrize(
     "beam_position, expected_xmove, expected_ymove",
     [
         (
             (15, 10),
-            "#1J:-" + str(15 * zoomcalibrator),
-            "#2J:-" + str(10 * zoomcalibrator),
+            "#1J:-" + str(15 * ZOOMCALIBRATOR),
+            "#2J:-" + str(10 * ZOOMCALIBRATOR),
         ),
         (
             (475, 309),
-            "#1J:-" + str(475 * zoomcalibrator),
-            "#2J:-" + str(309 * zoomcalibrator),
+            "#1J:-" + str(475 * ZOOMCALIBRATOR),
+            "#2J:-" + str(309 * ZOOMCALIBRATOR),
         ),
         (
             (638, 392),
-            "#1J:-" + str(638 * zoomcalibrator),
-            "#2J:-" + str(392 * zoomcalibrator),
+            "#1J:-" + str(638 * ZOOMCALIBRATOR),
+            "#2J:-" + str(392 * ZOOMCALIBRATOR),
         ),
     ],
 )
 @patch(
     "mx_bluesky.beamlines.i24.serial.fixed_target.i24ssx_moveonclick._get_beam_centre"
 )
+@patch(
+    "mx_bluesky.beamlines.i24.serial.fixed_target.i24ssx_moveonclick._calculate_zoom_calibrator"
+)
 def test_onMouse_gets_beam_position_and_sends_correct_str(
+    fake_zoom_calibrator: MagicMock,
     fake_get_beam_pos: MagicMock,
     beam_position: tuple,
     expected_xmove: str,
     expected_ymove: str,
     pmac: PMAC,
     RE,
 ):
+    fake_zoom_calibrator.side_effect = [fake_generator(ZOOMCALIBRATOR)]
     fake_get_beam_pos.side_effect = [beam_position]
     fake_oav: OAV = MagicMock(spec=OAV)
-    RE(onMouse(cv.EVENT_LBUTTONUP, 0, 0, "", param=[pmac, fake_oav]))
+    onMouse(cv.EVENT_LBUTTONUP, 0, 0, "", param=[RE, pmac, fake_oav])
     mock_pmac_str = get_mock_put(pmac.pmac_string)
     mock_pmac_str.assert_has_calls(
         [
@@ -56,6 +69,20 @@ def test_onMouse_gets_beam_position_and_sends_correct_str(
     )
 
 
+@pytest.mark.parametrize(
+    "zoom_percentage, expected_calibrator", [(1, 1.517), (20, 1.012), (50, 0.455)]
+)
+@patch("mx_bluesky.beamlines.i24.serial.fixed_target.i24ssx_moveonclick.bps.rd")
+def test_calculate_zoom_calibrator(
+    fake_read: MagicMock, zoom_percentage: int, expected_calibrator: float, RE
+):
+    fake_read.side_effect = [fake_generator(zoom_percentage)]
+    fake_oav: OAV = MagicMock(spec=OAV)
+    res = RE(_calculate_zoom_calibrator(fake_oav)).plan_result  # type: ignore
+
+    assert res == pytest.approx(expected_calibrator, abs=1e-3)
+
+
 @patch("mx_bluesky.beamlines.i24.serial.fixed_target.i24ssx_moveonclick.cv")
 @patch(
     "mx_bluesky.beamlines.i24.serial.fixed_target.i24ssx_moveonclick._get_beam_centre"