bluesky / ophyd-epics-devices

https://bluesky.github.io/ophyd-epics-devices
Apache License 2.0
0 stars 0 forks source link

Find a way to remove bps.sleep(0.1) from the plan #27

Closed coretl closed 1 year ago

DominicOram commented 1 year ago

In MX we have similar issues around waiting plan side, which is very messy (https://github.com/DiamondLightSource/python-artemis/issues/426). Some examples:

The detector Z and shutter has been moved by GDA, we want to make sure it's finished:

def wait_for_det_to_finish_moving(detector: DetectorMotion, timeout=120):
    LOGGER.info("Waiting for detector to finish moving")
    SLEEP_PER_CHECK = 0.1
    times_to_check = int(timeout / SLEEP_PER_CHECK)
    for _ in range(times_to_check):
        detector_state = yield from bps.rd(detector.shutter)
        detector_z_dmov = yield from bps.rd(detector.z.motor_done_move)
        LOGGER.info(f"Shutter state is {'open' if detector_state==1 else 'closed'}")
        LOGGER.info(f"Detector z DMOV is {detector_z_dmov}")
        if detector_state == 1 and detector_z_dmov == 1:
            return
        yield from bps.sleep(SLEEP_PER_CHECK)
    raise TimeoutError("Detector not finished moving")

We want to set a bunch of PVs and wait on a separate PV to tell us the device is configured. We currently do:

def wait_for_fgs_valid(fgs_motors: FastGridScan, timeout=0.5):
    artemis.log.LOGGER.info("Waiting for valid fgs_params")
    SLEEP_PER_CHECK = 0.1
    times_to_check = int(timeout / SLEEP_PER_CHECK)
    for _ in range(times_to_check):
        scan_invalid = yield from bps.rd(fgs_motors.scan_invalid)
        pos_counter = yield from bps.rd(fgs_motors.position_counter)
        artemis.log.LOGGER.debug(
            f"Scan invalid: {scan_invalid} and position counter: {pos_counter}"
        )
        if not scan_invalid and pos_counter == 0:
            return
        yield from bps.sleep(SLEEP_PER_CHECK)
    raise WarningException("Scan invalid - pin too long/short/bent and out of range")

I think there is probably a cleaner way to do this in the device for 2) and you could argue that 1) is only a problem due to GDA/Bluesky migration and will go away on it's own

tacaswell commented 1 year ago

https://github.com/bluesky/bluesky/pull/1596 is one path that may help.

In the first case eventually it should be the shutter and motor's job to report when they are done so in the plan you should do:

    yield from bps.mv(detector.shutter, "open", detector.z_motor, target)

Part of the assumption of blueky is that when it is running it controls the beamline so having a hybrid GDA/bluesky system is likely to have some rough edges.

coretl commented 1 year ago

Possibly the ophyd.v2.core.wait_for_value async method could be converted to return a Status, then we could pass this to bps.wait_for

coretl commented 1 year ago

Closed by https://github.com/bluesky/bluesky/pull/1598

prjemian commented 1 year ago

I do not see where bps.sleep() was removed in https://github.com/bluesky/bluesky/pull/1598.

coretl commented 1 year ago

the bps.sleep is in the test plan in this repo, rather than in bluesky. It enables the following: https://github.com/bluesky/ophyd-epics-devices/blob/c5efe8b5ee9adca2ba6dc0be9bb2844070c4cceb/tests/test_area_detector.py#L166-L174

To be written as per https://github.com/bluesky/bluesky/issues/1597:

for det in dets:
    yield from bps.complete(det, wait=False, group="complete")
done = False
while not done:
    try:
        yield from bps.wait(group="complete", timeout=FLUSH_PERIOD)
    except TimeoutError:
        pass
    else:
        done = True
    for det in dets:
        yield from bps.collect(det, stream=True, return_payload=False)
prjemian commented 1 year ago

Thanks for describing that!