We have a collection of ophyd devices in Artemis that should live here, move them in and confirm tests still pass

Our mypy enforces not having implicit optionals (i.e. x: float = None) and instead needs something like x: Union[None,float] = None, x: Optional[float] = None or x: float | None = None.

I like the last format best, but it is available in >=3.10 only, and we use python 3.10 features elsewhere (in artemis). Do we need to support 3.8 and 3.9 for other applications, or, since everyone is using venvs anyway, can we just say it's 3.10+?

If dict exists in the root of a module, eg

a = dict

make_all_devices crashes with ValueError: no signature found for builtin type <class 'dict'>

  File "/dls/athena/blueapi/src/blueapi/core/context.py", line 112, in with_device_module
    self.with_dodal_module(module)
  File "/dls/athena/blueapi/src/blueapi/core/context.py", line 117, in with_dodal_module
    for device in make_all_devices(module).values():
                  ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/dls/athena/blueapi/env/lib/python3.11/site-packages/dodal/utils.py", line 117, in make_all_devices
    factories = collect_factories(module)
                ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/dls/athena/blueapi/env/lib/python3.11/site-packages/dodal/utils.py", line 157, in collect_factories
    if callable(var) and _is_device_factory(var) and not _is_device_skipped(var):
                         ^^^^^^^^^^^^^^^^^^^^^^^
  File "/dls/athena/blueapi/env/lib/python3.11/site-packages/dodal/utils.py", line 169, in _is_device_factory
    return_type = signature(func).return_annotation
                  ^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/inspect.py", line 3279, in signature
    return Signature.from_callable(obj, follow_wrapped=follow_wrapped,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/inspect.py", line 3027, in from_callable
    return _signature_from_callable(obj, sigcls=cls,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/inspect.py", line 2587, in _signature_from_callable
    raise ValueError(
ValueError: no signature found for builtin type 

because it checks if precondition: where precondition is a callable

Currently SnapshotWithGrid inherits from MJPG, both of these are expected to be used in an OAV device. This is quite clean as one provides the snapshots with the grid and one without. However, because of the way Ophyd v.1 creates devices it means that it's not easy to choose at runtime which should be used without:

• Having the OAV contain both devices
• Having two OAV objects, one with grids and one without

We should decide if there is a better way of doing this (do it in v2?) or which of the two approaches above we prefer

Attenuation optimisation using total counts is added in #97 , but this algorithm is normally ran using the dead time optimisation.

In order to replace Zebra 1 on I03 with a PandA, there will need to be an Ophyd device to support it.

After #43 arming the Eiger is being done in parallel, we should also add the ability to disarm in parallel. Note that this probably won't save us much time in the xray centring case as we're waiting on results anyway but it may be useful in the future

See DiamondLightSource/hyperion#588

Reinstate the following lines turning warnings into errors in the pyproject.toml file.

# https://iscinumpy.gitlab.io/post/bound-version-constraints/#watch-for-warnings
filterwarnings = "error"

It has been temporarily removed in #17 in order to proceed with tests.

Ophyd is currently pinned to a specific git commit as we need bluesky/ophyd#1139 for some tests. Once this is merged and in a release we should unpin ophyd again

To avoid multiple people trying to write to the beamline at the same time we should have a baton. It probably makes sense for this baton to be in dodal so that anyone using it obeys the baton

Acceptance Criteria

• There is a discussion about how to handle this in dodal
• There is some documentation on what the solution looks like

If i03.aperture_scatterguard is called multiple times, sometimes with the AperturePositions and sometimes without you can end up with an instance that has no set positions and so is unusable

See error at https://github.com/DiamondLightSource/dodal/actions/runs/5454662045/jobs/9925840037

We should add CI now that https://github.com/DiamondLightSource/python3-pip-skeleton-cli/releases/tag/3.1.0 is ready

For some devices there may be metadata that is required alongside the device for later storage. e.g. the material the device is made of. It probably makes sense for dodal to have this data for applications to access but the way it's stored etc. is TBD

Acceptance Criteria

• A meeting is had to discuss the best way of storing this data
• The decision, and reasoning is documented in dodal with an example

As discussed in DiamondLightSource/hyperion#129 it would be useful to have timing metrics for how long actions take.

(As I understand it): dodal in Artemis/Hyperion lazily instantiates devices when plans that use the devices are required, so as to allow those plans to run regardless of the state of other devices on the beamline, and in this case if a device fails to instantiate the exception should be thrown.

make_all_devices is called by tests and when dodal is imported into Blueapi, which wants to know about all devices and register them to its context eagerly: in an analogous way, Blueapi would like to successfully start but not allow any devices that failed to be instantiated to be used as arguments for plans.

Tangentially related to but potentially opposed to desired behaviour of #3 (Presumably will call make_all_devices, but would be happy allowing an exception to be fatal in this case?)

Currently we import the devices from dodal and then create them locally rather than using the i03.py file in dodal. We should switch to using this.

Acceptance Criteria

• All device creation in Artemis is done through the functions in i03.py
• We should be using s03.py in the tests

As a developer it would be good to know ASAP if there have been changes to i03 that cause my ophyd devices to no longer be able to connect. As such it would be good to have a system test to check this. The test will only be able to be run on DLS network and getting it run in CI will be hard but just having it would be a good start.

It is likely that code will be being migrated from other repos to this one, it would be useful to have instructions on how to do this in this repo.

This should have the same functionality as the code already written for this device in GDA

Using the ophyd simulated devices is nice but we want some basic logic in them e.g. if we arm the zebra then armed gets set. It would be useful if we could get a device with this logic when we do i03.zebra(fake_with_ophyd=True)

The zebra device in dodal is currently missing some PVs which will be used in attenuation optimisation for the fluorescence detector

Once bluesky/ophyd-epics-devices#11 is merged we should be using that on i03

Given that staging is now asynchronous if it's still in progress when we unstage we should wait for it to finish (most likely this will only happen in an error state). We should also be more robust when unstaging, ensuring that if it fails at earlier points the detector still gets disarmed. There should also be tests on this

Existing optical edge detection algorithm is in an adPython areadetector plugin (it is a relatively simple script that calls into OpenCV). adPython is becoming unsupported at diamond long term.

Following discussion in MX office, pull this algorithm into the dodal layer to avoid introducing adPython onto i23.

Note: refers to pin tip centring not loop centring.

@DominicOram and @noemifrisina

Relates to #6, #3 and DiamondLightSource/hyperion#200

It seems, to avoid verbosity, we want module level devices so we can say

from i03 import dcm

and immediately use dcm as oppose to having a make_dcm() function that only instantiates the object when called.

However, we may want the device to do slightly different things, depending on context, when we import it, for example:

• Use a different PV prefix, for example, if it's a simulated beamline vs. real (#6) or if there are macros in the IOCs themselves that change the prefix (e.g. for the soft motors available in Diamond).
• Test that the devices connect on import (in Ophyd v1, this is done by calling wait_for_connection()) so that it doesn't unexpectedly cause an error when first used
• Explicitly not test that devices connect on import, for example because of long startup times, or because only a subset of devices are active on the beamline at any one time
• Run custom checks on the devices (e.g. detector.hdf5.warmup()) or not, or sometimes, as above

If we take parameterization as a possible solution for these use cases, we should think about how to achieve it. We want to maximize granularity and flexibility while minimizing boilerplate. The two are likely to work in mutual contravention so we would need to find a good compromise. Also, depending on exactly how much flexibility we feel we need, we should keep in mind the possibility that the make_dcm() function above may actually be the least worst option.

This will be used along with the device added in #71 to run attenuation optimisation for the Xspress3MiniDetector within Dodal. The PV's required for this are the same as what are used in GDA's PhaseISixteenFilterAttenuators

As a developer/scientist I would like to make to make sure that the devices for the simulated beamline are kept the same as those for the real one, with as much ease as possible. Currently to add a new device to e.g. i03/s03 I need to add the following to i03.py:

BEAMLINE_PREFIX = "BL03I"

def device():
     return MyDevice(prefix=f"{BEAMLINE_PREFIX}-BLAH")

Then the following to s03.py:

BEAMLINE_PREFIX = "BL03S"

def device():
     return MyDevice(prefix=f"{BEAMLINE_PREFIX}-BLAH")

Which is an exact replica, except the BEAMLINE_PREFIX. Ideally I should just need to add to i03.py and s03.py will update itself.

I have seen this error pop up occasionally in our CI testing:

=========================== short test summary info ============================
FAILED src/artemis/devices/unit_tests/test_eiger.py::test_given_stale_parameters_goes_high_before_callbacks_then_stale_parameters_waited_on - assert not True
 +  where True = <bound method Thread.is_alive of <Thread(Thread-459 (wait_on_staging), started daemon 140251838105152)>>()
 +    where <bound method Thread.is_alive of <Thread(Thread-459 (wait_on_staging), started daemon 140251838105152)>> = <Thread(Thread-459 (wait_on_staging), started daemon 140251838105152)>.is_alive
===== 1 failed, 196 passed, 1 skipped, 27 deselected, 4 warnings in 12.14s =====

e.g. https://github.com/DiamondLightSource/python-artemis/actions/runs/3998478557/attempts/1

await_value() in dodal/devices/status.py returns an unsuccessful status when the subscribable PV never gets changed. If the PV is already at the correct value but doesn't get modified, the function returns an unsuccessful status.

It would be useful for Python courses run at DLS to have a realistic Ophyd/Bluesky tutorial to try. There are already a number of useful tutorials:

• A non-interactive tutorial at https://nsls-ii.github.io/bluesky/tutorial.html
• An interactive tutorial at https://mybinder.org/v2/gh/bluesky/tutorials/main (but it's a bit messy)

Add I23's goniometer into dodal.

Currently the ApertureScatterguard says it's not allowed to move when it is sufficiently close

Currently the ApertureScatterguard and the Eiger devices need to be passed a set of parameters to be valid devices. This is done by doing something like the following:

def eiger(params: Optional[Parameters] = None):
    eiger = EigerDetector(...)
    if params is not None:
        eiger.set_params(params)
    return eiger

This causes issues as it's easy to make an invalid Eiger that has no parameters. Instead it would be nicer to do one of the following:

1. Require parameters to be set when instantiating the device - this then make it very hard to easily create all devices on the beamline
2. Have some method of getting a good default set of parameters - this opens up cans of worms around proper storage of configurations. e.g. do we have a configuration service that dodal will interrogate when trying to initialise a device?

I tried to add ad test for the p38 beamline definition using the make_all_device function as in test_i24.py, but was plagued by hangover of shared state from previous tests which was different from run to run on Github (due to the (correct) random order of test running) causing the tests to fail in different versions of Python on subsequent runs of the same actions. This seems to be associated with the BL and ACTIVE_DEVICES objects from the beamline_utilities module. Basically the set beamline can persist from one beamline test module to the next as can davices stored in the ACTIVE_DEVICES dict, resulting in tests that compare the length of created device to this dict failing randomly.

My efforts to try and get round this are currently pushed to the p38fix branch

to support jungfrau work and ssx

With #43 the stage of the eiger no longer exists. We should reinstate it so that:

• If the Eiger has already been armed it does nothing
• If the async_stage has started it waits on it to complete
• If the async_stage has not started it starts it and waits on it to complete

This may be the cause of DiamondLightSource/hyperion#686, we should confirm if this is fixed after we do this

A lot of the movements in DetectorMotion are binary setpoints, using 0/1 to mean open/closed. We should refactor these to be enums to be more descriptive

• In general, PV prefixes belong where devices are instantiated, in the beamline files
• Sometimes this is not possible because multiple prefixes are needed (#73)

Loading in the OAV params is currently very messy, I think it could be simplified using chainmap

An issue we have in GDA is that it's not always clear what code is in use by what beamlines and so whether code can be removed or how changing it may affect other beamlines.

Acceptance criteria

• By default people who use dodal get usage statistics of their use

Note

Should probably use the same system as #10

We should make it as easy as possible for scientists to get their code logging to file/graylog. As such there should be helper functions to initialise this and documentation on how to use it.

As part of moving plans from dls-bluesky-core, the scan plan (an equivalent to the GDA mscan command: mscan constructs a ScanPointGenerator, scan is passed a constructed ScanSpec: each just a description of an N-dimensional sequence of points) should be re-examined.

@callumforrester is fairly sure that axes_to_move was a Pydantic, APISchema or Mypy specific requirement (for type checking?): if it can be removed and the string value of an axis in a Spec converted to its underlying device, it would make typing a Scan with a Spec a little less verbose.

Alternatively, a default factory could be provided for the argument with {x: x for x in Spec.axes}, and the argument made optional.

For posterity, here is the source code of scan:

import operator
from functools import reduce
from typing import Any, List, Mapping, Optional

import bluesky.plans as bp
from bluesky.protocols import Movable, Readable
from cycler import Cycler, cycler
from scanspec.specs import Spec

"""
Plans related to the use of the `ScanSpec https://github.com/dls-controls/scanspec`
library for constructing arbitrarily complex N-dimensional trajectories, similar to
Diamond's "mapping scans" using ScanPointGenerator.
"""


def scan(
    detectors: List[Readable],
    axes_to_move: Mapping[str, Movable],
    spec: Spec[str],
    metadata: Optional[Mapping[str, Any]] = None,
) -> MsgGenerator:
    """
    Scan wrapping `bp.scan_nd`

    Args:
        detectors: List of readable devices, will take a reading at
                                    each point
        axes_to_move: All axes involved in this scan, names and
            objects
        spec: ScanSpec modelling the path of the scan
        metadata: Key-value metadata to include
                                                          in exported data, defaults to
                                                          None.

    Returns:
        MsgGenerator: Plan

    Yields:
        Iterator[MsgGenerator]: Bluesky messages
    """

    _md = {
        "plan_args": {
            "detectors": list(map(repr, detectors)),
            "axes_to_move": {k: repr(v) for k, v in axes_to_move.items()},
            "spec": repr(spec),
        },
        "plan_name": "scan",
        "shape": spec.shape(),
        **(metadata or {}),
    }

    cycler = _scanspec_to_cycler(spec, axes_to_move)
    yield from bp.scan_nd(detectors, cycler, md=_md)


def _scanspec_to_cycler(spec: Spec[str], axes: Mapping[str, Movable]) -> Cycler:
    """
    Convert a scanspec to a cycler for compatibility with legacy Bluesky plans such as
    `bp.scan_nd`. Use the midpoints of the scanspec since cyclers are normally used
    for software triggered scans.

    Args:
        spec: A scanspec
        axes: Names and axes to move

    Returns:
        Cycler: A new cycler
    """

    midpoints = spec.frames().midpoints
    midpoints = {axes[name]: points for name, points in midpoints.items()}

    # Need to "add" the cyclers for all the axes together. The code below is
    # effectively: cycler(motor1, [...]) + cycler(motor2, [...]) + ...
    return reduce(operator.add, map(lambda args: cycler(*args), midpoints.items()))

In order to for example:

• load parameters at instantiation time
• allow passing in extra prefixes for devices which need more than one (eg OAV, CTAB...)

In 99% of cases (on i03) we want to apply the flatfield to the OAV when we set the zoom level to 1x. Given this is so common we should do it device side and create a combined OAV with zoom device.

This isn't actually used anywhere, instead we use the num_triggers and num_images_per_trigger variables for this.

Currently we do not have the zoom level selection in S03, we should add it in