Merge branch 'main' into 782_read_aperture_scatterguard_better

src/dodal/devices/focusing_mirror.py

@@ -76,7 +76,7 @@ async def set(self, value, *args, **kwargs):
             LOGGER.debug(f"{setpoint_v.name} already at {value} - skipping set")
             return
 
-        LOGGER.debug(f"setting {setpoint_v.name} to {value}")
+        LOGGER.debug(f"Setting {setpoint_v.name} to {value}")
 
         # Register an observer up front to ensure we don't miss events after we
         # perform the set
@@ -85,16 +85,14 @@ async def set(self, value, *args, **kwargs):
         )
         # discard the current value (OK) so we can await a subsequent change
         await anext(demand_accepted_iterator)
-        await setpoint_v.set(value)
+        set_status = setpoint_v.set(value, wait=False)
 
         # The set should always change to SLEW regardless of whether we are
         # already at the set point, then change back to OK/FAIL depending on
         # success
         accepted_value = await anext(demand_accepted_iterator)
         assert accepted_value == MirrorVoltageDemand.SLEW
-        LOGGER.debug(
-            f"Demand not accepted for {setpoint_v.name}, waiting for acceptance..."
-        )
+        LOGGER.debug(f"Waiting for {setpoint_v.name} to set")
         while MirrorVoltageDemand.SLEW == (
             accepted_value := await anext(demand_accepted_iterator)
         ):
@@ -104,6 +102,7 @@ async def set(self, value, *args, **kwargs):
             raise AssertionError(
                 f"Voltage slew failed for {setpoint_v.name}, new state={accepted_value}"
             )
+        await set_status
 
 
 class VFMMirrorVoltages(StandardReadable):

src/dodal/devices/webcam.py

@@ -1,12 +1,24 @@
+from collections.abc import ByteString
+from io import BytesIO
 from pathlib import Path
 
 import aiofiles
 from aiohttp import ClientSession
 from bluesky.protocols import Triggerable
 from ophyd_async.core import AsyncStatus, HintedSignal, StandardReadable, soft_signal_rw
+from PIL import Image
 
 from dodal.log import LOGGER
 
+PLACEHOLDER_IMAGE_SIZE = (1024, 768)
+IMAGE_FORMAT = "png"
+
+
+def create_placeholder_image() -> ByteString:
+    image = Image.new("RGB", PLACEHOLDER_IMAGE_SIZE)
+    image.save(buffer := BytesIO(), format=IMAGE_FORMAT)
+    return buffer.getbuffer()
+
 
 class Webcam(StandardReadable, Triggerable):
     def __init__(self, name, prefix, url):
@@ -18,19 +30,33 @@ def __init__(self, name, prefix, url):
         self.add_readables([self.last_saved_path], wrapper=HintedSignal)
         super().__init__(name=name)
 
-    async def _write_image(self, file_path: str):
+    async def _write_image(self, file_path: str, image: ByteString):
+        async with aiofiles.open(file_path, "wb") as file:
+            await file.write(image)
+
+    async def _get_and_write_image(self, file_path: str):
         async with ClientSession() as session:
             async with session.get(self.url) as response:
-                response.raise_for_status()
-                LOGGER.info(f"Saving webcam image from {self.url} to {file_path}")
-                async with aiofiles.open(file_path, "wb") as file:
-                    await file.write(await response.read())
+                if not response.ok:
+                    LOGGER.warning(
+                        f"Webcam responded with {response.status}: {response.reason}. Attempting to read anyway."
+                    )
+                try:
+                    data = await response.read()
+                    LOGGER.info(f"Saving webcam image from {self.url} to {file_path}")
+                except Exception as e:
+                    LOGGER.warning(
+                        f"Failed to read data from {self.url} ({e}). Using placeholder image."
+                    )
+                    data = create_placeholder_image()
+
+                await self._write_image(file_path, data)
 
     @AsyncStatus.wrap
     async def trigger(self) -> None:
         filename = await self.filename.get_value()
         directory = await self.directory.get_value()
 
-        file_path = Path(f"{directory}/{filename}.png").as_posix()
-        await self._write_image(file_path)
+        file_path = Path(f"{directory}/{filename}.{IMAGE_FORMAT}").as_posix()
+        await self._get_and_write_image(file_path)
         await self.last_saved_path.set(file_path)

tests/devices/unit_tests/test_focusing_mirror.py

@@ -4,13 +4,13 @@
 # see https://docs.python.org/3.10/library/asyncio-exceptions.html
 # https://github.com/python/cpython/issues?q=is%3Aissue+timeouterror++alias+
 from asyncio import TimeoutError
-from unittest.mock import DEFAULT, patch
+from unittest.mock import ANY, DEFAULT, patch
 
 import pytest
 from bluesky import plan_stubs as bps
 from bluesky.run_engine import RunEngine
 from bluesky.utils import FailedStatus
-from ophyd_async.core import get_mock_put, set_mock_value
+from ophyd_async.core import callback_on_mock_put, get_mock_put, set_mock_value
 
 from dodal.devices.focusing_mirror import (
     FocusingMirrorWithStripes,
@@ -84,9 +84,9 @@ def not_ok_then_other_value(*args, **kwargs):
         asyncio.create_task(set_demand_accepted_after_delay())
         return DEFAULT
 
-    get_mock_put(
-        vfm_mirror_voltages.voltage_channels[0]._setpoint_v
-    ).side_effect = not_ok_then_other_value
+    callback_on_mock_put(
+        vfm_mirror_voltages.voltage_channels[0]._setpoint_v, not_ok_then_other_value
+    )
     set_mock_value(
         vfm_mirror_voltages.voltage_channels[0]._demand_accepted,
         MirrorVoltageDemand.OK,
@@ -136,7 +136,7 @@ def plan():
 
     RE(plan())
 
-    mock_put.assert_called_with(100, wait=True, timeout=10.0)
+    mock_put.assert_called_with(100, wait=ANY, timeout=ANY)
 
 
 def test_mirror_set_voltage_sets_and_waits_happy_path_spin_while_waiting_for_slew(
@@ -166,7 +166,7 @@ def plan():
 
     RE(plan())
 
-    mock_put.assert_called_with(100, wait=True, timeout=10.0)
+    mock_put.assert_called_with(100, wait=ANY, timeout=ANY)
 
 
 def test_mirror_set_voltage_set_rejected_when_not_ok(
@@ -191,9 +191,7 @@ def test_mirror_set_voltage_sets_and_waits_set_fail(
     def failed(*args, **kwargs):
         raise AssertionError("Test Failure")
 
-    get_mock_put(
-        vfm_mirror_voltages_with_set.voltage_channels[0]._setpoint_v
-    ).side_effect = failed
+    vfm_mirror_voltages_with_set.voltage_channels[0]._setpoint_v.set = failed
 
     def plan():
         with pytest.raises(FailedStatus) as e:

tests/devices/unit_tests/test_webcam.py

@@ -1,10 +1,12 @@
+from io import BytesIO
 from unittest.mock import AsyncMock, MagicMock, patch
 
 import pytest
 from bluesky.run_engine import RunEngine
+from PIL import Image
 
 from dodal.beamlines import i03
-from dodal.devices.webcam import Webcam
+from dodal.devices.webcam import Webcam, create_placeholder_image
 
 
 @pytest.fixture
@@ -37,9 +39,6 @@ async def test_given_filename_and_directory_when_trigger_and_read_then_returns_e
     webcam: Webcam,
 ):
     mock_get.return_value.__aenter__.return_value = AsyncMock()
-    mock_get.return_value.__aenter__.return_value = (mock_response := AsyncMock())
-    # raise_for_status should be MagicMock() not AsyncMock()
-    mock_response.raise_for_status = MagicMock()
     await webcam.filename.set(filename)
     await webcam.directory.set(directory)
     await webcam.trigger()
@@ -53,8 +52,6 @@ async def test_given_data_returned_from_url_when_trigger_then_data_written(
     mock_get: MagicMock, mock_aiofiles, webcam: Webcam
 ):
     mock_get.return_value.__aenter__.return_value = (mock_response := AsyncMock())
-    # raise_for_status should be MagicMock() not AsyncMock()
-    mock_response.raise_for_status = MagicMock()
     mock_response.read.return_value = (test_web_data := "TEST")
     mock_open = mock_aiofiles.open
     mock_open.return_value.__aenter__.return_value = (mock_file := AsyncMock())
@@ -65,20 +62,43 @@ async def test_given_data_returned_from_url_when_trigger_then_data_written(
     mock_file.write.assert_called_once_with(test_web_data)
 
 
-@patch("dodal.devices.webcam.aiofiles", autospec=True)
 @patch("dodal.devices.webcam.ClientSession.get", autospec=True)
-async def test_given_response_throws_exception_when_trigger_then_exception_rasied(
-    mock_get: MagicMock, mock_aiofiles, webcam: Webcam
+async def test_given_response_has_bad_status_but_response_read_still_returns_then_still_write_data(
+    mock_get: MagicMock, webcam: Webcam
 ):
-    class MyException(Exception):
-        pass
+    mock_get.return_value.__aenter__.return_value = (mock_response := AsyncMock())
+    mock_response.ok = MagicMock(return_value=False)
+    mock_response.read.return_value = (test_web_data := b"TEST")
+
+    webcam._write_image = (mock_write := AsyncMock())
+
+    await webcam.filename.set("file")
+    await webcam.directory.set("/tmp")
+    await webcam.trigger()
 
-    def _raise():
-        raise MyException()
+    mock_write.assert_called_once_with("/tmp/file.png", test_web_data)
 
+
+@patch("dodal.devices.webcam.create_placeholder_image", autospec=True)
+@patch("dodal.devices.webcam.ClientSession.get", autospec=True)
+async def test_given_response_read_fails_then_placeholder_image_written(
+    mock_get: MagicMock, mock_placeholder_image: MagicMock, webcam: Webcam
+):
     mock_get.return_value.__aenter__.return_value = (mock_response := AsyncMock())
-    mock_response.raise_for_status = _raise
+    mock_response.read = AsyncMock(side_effect=Exception())
+    mock_placeholder_image.return_value = (test_placeholder_data := b"TEST")
+
+    webcam._write_image = (mock_write := AsyncMock())
+
     await webcam.filename.set("file")
     await webcam.directory.set("/tmp")
-    with pytest.raises(MyException):
-        await webcam.trigger()
+    await webcam.trigger()
+
+    mock_write.assert_called_once_with("/tmp/file.png", test_placeholder_data)
+
+
+def test_create_place_holder_image_gives_expected_bytes():
+    image_bytes = create_placeholder_image()
+    placeholder_image = Image.open(BytesIO(image_bytes))
+    assert placeholder_image.width == 1024
+    assert placeholder_image.height == 768