ADTriggerStatus (v1) notifies watchers after completion hook

[callumforrester]

Like all statuses, you can put a complete callback on ADTriggerStatus. You can also put a watch callback on it which is called when it's progress changes. When the status completes, it calls the complete hook and then calls the watch hook one last time. I am unsure if this is expected behaviour. It makes sense to me that the complete hook should be called last.

I'm happy to make a PR to fix this, just want to check first.

Expected Behavior

When applying the following two hooks to status

from ophyd.areadetector import ADTriggerStatus
from ophyd.status import Status

def setup_hooks(status: ADTriggerStatus) -> None:
    def on_event(*args, **kwargs) -> None:
        print("Status Updated")

    def on_complete(status: Status) -> None:
        print("Status Completed")

    status.watch(on_event)
    status.add_callback(on_complete)

det = SomeAreaDetector()
try:
    det.stage()
    status = det.trigger()
    setup_hooks(status)
    status.wait()
finally:
    det.unstage()

It makes sense to me that it should output updates followed by a complete e.g.

Status Updated
Status Updated
Status Updated
Status Completed

Current Behavior

The above actually outputs:

Status Updated
Status Updated
Status Completed
Status Updated

Possible Solution

Assuming this isn't the desired behaviour, I'm unsure if the status should simply not make the final call to watchers or do it before complete.

The reason I suspect it may be expected behaviour is that the watch hook does the following:

def _notify_watchers(self, value, *args, **kwargs):
        if self.done:
            self.device.cam.array_counter.clear_sub(self._notify_watchers)
        ...
        for watcher in self._watchers:
            watcher(...)

Suggesting that the author intended a final pass over the notification code after unsubscribing. If not the solution is to simply add an else clause.

Steps to Reproduce (for bugs)

See code block above

[tacaswell]

My memory is that this was intentional.

The idea is that if the updates are being throttled / gated somehow there is not guarantee that the last organically generated status update will correspond to "done". Thus once we know that the acquisition (or motion) is done (which is decided someplace else), there is one last message passed to the watchers to let them know everything is finished.

I am not sure if we want ensure that all the watchers (which is a UI thing) is notified that the motion is done before the Status object done callbacks (which are used in the RE as part of the acquisition logic) fire. I would also have to trace through again to remind myself if the watchers go out sync or async....

[callumforrester]

"order not guaranteed because they are for different things" is a reasonable stance IMO.

But I was playing with remote operation of the RunEngine and ran into this while doing status bookeeping, so possibly an issue you and @danielballan will run into with the minimal server.