Make it easier to use area detectors with a factory

[prjemian]

• close area detector factory function #984

[prjemian]

The initial commit is for a fixed version of ophyd area detector support. It is templated on the support provided for the APS XPCS instrument.

[prjemian]

Still need to add/blend contribution by @cooleyv that requests the ADCore version from the existing PV and use case from @canismarko

The challenge with asking the ADCore version is that the IOC must be running enough to respond.

[prjemian]

Will be a fun challenge blending these and then building unit tests.

[prjemian]

The factory method looks too complex. Can it be simpler? (advice and here)

[prjemian]

There are very few detectors in use at APS that still use ADCore releases older than 3.4. Additional support for versions of the various plugins is not justified. They can be addressed individually if the new factory design is not adequate.

[prjemian]

Needs some tests for ophyd.sim.instantiate_fake_device(), as suggested by @canismarko.

[prjemian]

Instead of:

apstools/apstools/devices/area_detector_factory.py

Lines 147 to 148 in abd00fe

	REMOVE_DEFAULT_KEY = object()
	"""Flag to remove a plugin configuration key."""

to remove a default, such as:

apstools/apstools/devices/area_detector_factory.py

Lines 53 to 60 in abd00fe

	det = ad_creator(
	"MyEiger", "ad:", "det",
	[
	{"cam": {"class": EigerDetectorCam}},
	"image",
	{"hdf1": {"write_path_template": REMOVE_DEFAULT_KEY}},
	],
	)

allow the caller to re-define a key with defaults kwarg which will replace the PLUGIN_DEFAULTS dictionary:

apstools/apstools/devices/area_detector_factory.py

Line 82 in abd00fe

PLUGIN_DEFAULTS = { # some of the common plugins

[prjemian]

@cooleyv, @keenanlang, @canismarko, @gfabbris, @strempfer, @MarkRivers -- The new area detector factory support is ready for review.

[prjemian]

@sureshnaps : Your suggestion has led to simplifications such as.

    from ophyd.areadetector import SimDetectorCam
    from apstools.devices import ad_creator

    det = ad_creator(
        "ad:", name="det", class_name="MySimDetector",
        [
            {"cam": {"class": SimDetectorCam}},
            "image",  # all the defaults are acceptable
            "pva",
            {"hdf1": {"write_path_template": "/", "read_path_template": "/mnt/ioc/tmp"}},
            "codec1",
            "proc1",
            "roi1",
            "stat1",
        ],
    )

[prjemian]

In the PLUGIN_DEFAULTS, the origin (ophyd v. apstools) of the default class for each plugin should be more obvious. Use absolute paths for the classes from ophyd.

[MDecarabas]

Looks good, can you make an issue to remove the local version out of 8-ID and transition to the APS tools version. We can make the changes at the same time we re-implement specwriter

[canismarko]

This strikes me as a positive change.

The difference between the plugins and plugin_defaults arguments to the AD factory is tricky for me to understand. I think this is because of the complicated way area detectors are built in ophyd rather than a design issue for the factory, so probably this needs to be well documented.

[canismarko]

Updating my reivew based on Pete's point that these actually need to be keyword arguments in the examples.

[canismarko]

This makes it impossible to pass an empty dict ({}) to this function as plugin_defaults, since bool({}) evaluates to False and then PLUGIN_DEFAULTS would get used.

Maybe not a big deal since I can't imagine many people would want this. But I think it could be avoided by using PLUGIN_DEFAULTS as the default value for the plugin_defaults, which makes the intention more clear when looking at the call signature.

[prjemian]

Can do.

[prjemian]

Same for plugins=None. Changing it to plugins=["cam"].

[prjemian]

Ah... there's the reason to keep the default kwarg value of None -- in the unit tests, None is used.

ERROR apstools/devices/tests/test_area_detector_support.py::test_AD_EpicsFileNameMixin[hdf1-AD_HDF5] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_AD_EpicsFileNameMixin[jpeg1-AD_JPEG] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_AD_EpicsFileNameMixin[tiff1-AD_TIFF] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_stage[hdf1-AD_EpicsFileNameHDF5Plugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_stage[jpeg1-AD_EpicsFileNameJPEGPlugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_stage[tiff1-AD_EpicsFileNameTIFFPlugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_bp_count_custom_name[hdf1-AD_EpicsFileNameHDF5Plugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_bp_count_custom_name[jpeg1-AD_EpicsFileNameJPEGPlugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_bp_count_custom_name[tiff1-AD_EpicsFileNameTIFFPlugin] - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_full_file_name_local - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_no_file_path - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_file_numbering - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_capture_error_with_single_mode - TypeError: Must be a dict.  Received plugin_defaults=None
ERROR apstools/devices/tests/test_area_detector_support.py::test_single_mode - TypeError: type.__new__() argument 2 must be tuple, not None

Code like this is needed:

    if plugin_defaults is None:
        plugin_defaults = PLUGIN_DEFAULTS

[canismarko]

Okay, sounds good.

[prjemian]

I can't imagine many people would want plugin_defaults={}

Actually, this is a realistic case to remove all defaults and force the caller to define all keywords of each plugin. Probably only useful for unit testing, not real world?

[prjemian]

@canismarko Will these changes work for you?

[canismarko]

Yes, changes look good to me. Thanks!

[prjemian]

I also added some documentation (look for EXAMPLE 1: and others).