Refactor triggering in area detector to simplify it

[danielballan]

The implementation of area detector triggering is one of the oldest parts of ophyd. It was (re-)developed when we had the least experience with this newly-conceived hardware abstraction. It allows much more flexibility that we actually have needed in practice. Knowing that, we can massively simplify it.

Current implementation

WARNING: Thar be :dragon: s!

When an area detector is triggered, we dynamically add items to the dictionary that will be returned by describe() and read(). In practice, we add one item, typically named something like "fccd_image" or "eiger1m_image". We also provide a timestamp at this point. The important line in this snippet is self.dispatch(...).

https://github.com/bluesky/ophyd/blob/22089ca1795dd41b214c3589fb25af84f644356d/ophyd/areadetector/trigger_mixins.py#L129-L138

We also allow one trigger() to add multiple items to describe() and read(), with unique names of course. This was designed to support a range of readings in one Event with different gains, or with light and dark frames. In practice, we long ago found better, simpler ways address these use cases. AFAIK this was only ever used at NSLS-II CSX, and it has not been used there for several years.

https://github.com/bluesky/ophyd/blob/22089ca1795dd41b214c3589fb25af84f644356d/ophyd/areadetector/trigger_mixins.py#L221-L263

When self.dispatch(...) is called, all currently-enabled file plugins are told to generate a datum and add it to an internal cache that will be merged with the return of the items returned by read(). That's right, we support the possibility that the detector is running with multiple file plugins, like HDF5 and TIFF. This has never been needed in practice.

https://github.com/bluesky/ophyd/blob/bc9770f19c16bda79df12049242db5c2583690df/ophyd/areadetector/detectors.py#L49-L70

https://github.com/bluesky/ophyd/blob/22089ca1795dd41b214c3589fb25af84f644356d/ophyd/areadetector/filestore_mixins.py#L349-L396

Notice the lock self._locked_key_list that ensures that describe() and read() always return the same keys within a given stage/unstage cycle. It would be better if they returned the same keys always. This is the only place is ophyd where a Device dynamically adds keys to the dictionaries returned by describe() and read() or where they contain values for which there is no corresponding Component. It would be great to remove this exception from the codebase.

Proposed implementation

Rather than dynamically adding keys to the describe() and read() dict, add soft-signal (ophyd.Signal) Component to the Device when can be used to stash the datum_id that goes with the current reading.

Trial implementations, ordered from initial attempt to the latest (maybe most polished) attempt:

• https://github.com/aps-8id-dys/ipython-8idiuser/blob/master/profile_bluesky/startup/instrument/devices/rigaku_ufxc.py
• https://github.com/NSLS-II-FXI/profile_collection/blob/430a79862b215ea4b4e3bf1990644182e3a0b298/startup/10-area-detector.py#L25-L36
• https://github.com/NSLS-II-BMM/profile_collection/blob/b1b4b2d4ca2a087f649012c91c1535c2024bec00/startup/48-camera-device.py#L46-L114

In these examples, the triggering behavior is hard-coded in the detector, but when moved upstream it may be best to keep it in the mixin.

[tacaswell]

https://github.com/bluesky/tutorials/blob/master/bluesky-tutorial-utils/bluesky_tutorial_utils/_newton.py is another example of the newer design.

[mrakitin]

https://github.com/bluesky/tutorials/blob/master/bluesky-tutorial-utils/bluesky_tutorial_utils/_newton.py is another example of the newer design.

Nice example :) I somehow missed that nice example. What was the context of creating it?

[danielballan]

That was made for the Users' Meeting in May.

[danielballan]

The main question in my mind for this simplification is, "If we drop MultiTriggerMixin from the codebase, can we move the triggering logic and Resource/Datum logic more deeply into the core classes, such that all the objects in ophyd.area_detector.trigger_mixins and ophyd.area_detector.filestore_mixins become empty or nearly-empty no-ops, just present for backward-compatibility?" I am not sure what the consequences would be of going all the way that, but that's the general direction I want to push in. Making those objects simpler or even obsolete would make #889 easier as well.

[thomascobb]

One thing that I found when writing Malcolm is that the separation of interface (and HDF plugin has N Pvs of these type with these names) and logic (when I trigger a scan, give the HDF plugin some static metadata and start it) into different objects has been necessary. At the moment there is mostly only one set of logic with each interface, but we're just starting to need others. I've also favoured composition over inheritance, so OneFilePerRunHDF object has an HDFPluginDevice, rather than inherits from it. This becomes especially important when dealing with multi-purpose devices like PandABoxes. I haven't dug enough into ophyd's areaDetector implementation to see if you do the same yet (so this might be completely off topic), but I'll be looking at that soon.

[danielballan]

We're big fans of "composition is generally better than inheritance" here. We bake the sequencing for how to setup ("stage" in bluesky/ophyd jargon) a detector for acquisition into the class, and it's been a pain point. Separating that into a separate, composable object is a very interesting idea.

[thomascobb]

ok, I'll continue exploring that separation while I try out some flyscanning ideas, hopefully I'll report back with a toy implementation in the next couple of weeks

[thomascobb]

Discussed this in detail with @tacaswell today, our discussion points are here: https://bluefly.readthedocs.io/en/latest/discussion.html

The best example is that of MotorDevice, it is a SettableDevice that has a MotorRecord child which has Signals, as opposed to EpicsMotor which has Signals itself. This separates out the logic from the PV interface, and means that another logic class can use the same MotorRecord to do fly scanning without the step scan logic possibly getting in the way.

The bonus of this approach is that you get to write logic that spans multiple Devices in a transparent way (i.e. HDF + Proc + Stats + Driver in one bit of logic), which means it is more readable down the line. The downside is that you repeat yourself more, although you can reduce most of the boilerplate with judicious extraction of library functions...