473 create a device for xpress3 areadetector

[Relm-Arrowny]

Fixes DiamondLightSource/hyperion#473

• Replaced xspress3_mini(ophyd) with xspress3(ophyd_async) and added support for more than one channel. Extra channels are stored as a list. xspress3mini.channels[<channel number>]

• Removed function arm and unify it with stage. Should be able to replace
  yield from bps.abs_set(xspress3mini.do_arm, 1, wait=True)
  with
  yield from bps.stage(xspress3mini, wait=True)
  in plans.

• Also removed signal with epics ip ending Erase as it is not connected or used for anything.

Instructions to reviewer on how to test:

1. Run unit test
2. Confirm the function stage is logically sound to replace express_mini.arm

Checks for reviewer

• Would the PR title make sense to a scientist on a set of release notes
• If a new device has been added does it follow the standards
• If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly

[DominicOram]

This is a good start, some comments in code. Additionally can you:

• Remove the old xspress3_mini
• Remove ophyd_async and oa from the names, I think if there's only one device it's not needed
• Change i03.py to use this version and add it to i20_1.py
• Run the system tests for i03/i20_1 and confirm the PVs all connect still

How would you like to use this device? All we do in hyperion is arm the device and directly read a couple of the signals into the plan. Presumably you want something more complicated with getting the data to disk etc? In which case we should think about what you want read() to do. We do have a requirement at some point to also read data out in a better way, supposedly the device can write HDF files? We'd be interested in doing that and embedding the data in our nexus files.

[stan-dot]

just expand this one abbreviation pls

[DominicOram]

This is really good work but a couple of comments in code

[DominicOram]

Must: This test isn't doing what you think it is. if you remove set_mock_value(mock_xspress3mini.acquire, AcquireState.DONE) it still passes fine. I think you need to write some tests that confirm:

• That stage is setting everything you think it is
• That if acquire RBV does not go to DONE it fails
• That RE(bps.stage(...)) is happy

[DominicOram]

@callumforrester - From playing with this a bit, am I right that we need Xspress3 to inherit from Stageable to be able to call bps.stage(..) on it? If so, some comments:

• It's not obvious from a unit test (without using the RE) that this is needed, in which case we should recommend that all devices are covered with RE tests
• If we do inherit from Stageable it also looks like we need to provide an unstage. In this case I'm not sure what that would be, it's a bit frustrating to have to provide a noop, but maybe that's deliberate?

[callumforrester]

Stageable is a protocol, meaning you don't actually need to inherit from it, you can also type annotate uses of it and then it will be checked via duck typing. If you inherit from it you're just making it clearer in the code: this must be Stageable.

Both bullet points are good, I would quite like all devices to have RE tests, it would have caught several issues we've seen on the beamline. I'd also happily support an issue in bluesky to separate out the Stageable protocol. Something like:

class Stageable(Protocol):
    def stage(self): ...

class StageableUnstageable(Protocol):
    def stage(self): ...
    def unstage(self): ...

[DominicOram]

Couple more comments

[DominicOram]

Should: I don't think it makes sense to wait on both of these stage basically means get the device ready to take data so I expect that we want to wait for ACQUIRE (it is now taking data) rather than DONE (it has finished taking data).

[Relm-Arrowny]

I think I got confused with the return await_value(self.acquire, 0).

acquire = Component(EpicsSignalWithRBV, "Acquire")

def arm(self) -> Status:
        LOGGER.info("Arming Xspress3Mini detector...")
        self.trigger_mode_mini.put(TriggerMode.BURST.value)
        arm_status = await_value_in_list(self.detector_state, self.detector_busy_states)
        self.erase.put(EraseState.ERASE.value)
        arm_status &= self.acquire.set(AcquireState.ACQUIRE.value)
        arm_status.wait(self.ARM_STATUS_WAIT)
        return await_value(self.acquire, 0)

would this be more correct with unstage?

   @AsyncStatus.wrap
   async def stage(self) -> None:
       LOGGER.info("Arming Xspress3 detector...")
       await self.trigger_mode.set(TriggerMode.BURST)
       await wait_for_value(
           self.detector_state,
           lambda v: v in self.detector_busy_states,
           timeout=self.timeout,
       )
       await self.acquire.set(AcquireState.ACQUIRE)
       await wait_for_value(
           self.acquire_rbv, AcquireRBVState.ACQUIRE, timeout=self.timeout
       )
   @AsyncStatus.wrap
   async def unstage(self) -> None:
       LOGGER.info("unstaging Xspress3 detector...")
       await self.acquire.set(AcquireState.DONE)
       await wait_for_value(
           self.acquire_rbv, AcquireRBVState.DONE, timeout=self.timeout
       )

Or just does nothing?

    @AsyncStatus.wrap
    async def unstage(self) -> None:
        LOGGER.info("unstaging Xspress3 detector...")

I think RE automatically call unstage if stage was used, so if we use stage we will need to have unstage. The most annoying part is RE seem to just skip and make it looks like test is passing if there is no unstage.....

[DominicOram]

I think the waiting for the done in unstage makes more logically sense to me as something we would expect on the device. So let's leave it at that and make sure it works as expected as part of https://github.com/DiamondLightSource/hyperion/issues/1433

[DominicOram]

Great, thank you! I think I'm happy with this now. I put in some time for us to go and test on the beamline next month. Could you just fix the unit tests then we can merge. Also, can you make sure to merge DiamondLightSource/hyperion#1434 at the same time too?

[DominicOram]

Great, thanks!