Allow wait_for_value to take existing generator

[DominicOram]

For a couple of devices in dodal we have hit potential race conditions where we:

1. Do x
2. This causes x_rbv to go 0 -> 1 -> 0
3. wait_for_value(x_rbv, 1) - this fails as the PV has already done the transition

A solution to this would be for wait_for_value to accept an AsyncGenerator[T] and so you could do:

monitor = observe_value(x_rbv)
x.set(1)
wait_for_value(x_rbv, 1)

This could potentially be extended to something like:

with wait_for_value_after(x_rbv, 1):
    x.set(1)

[coretl]

Alternatively we change

ophyd-async/src/ophyd_async/core/signal.py

Line 560 in 0269697

async def set_and_wait_for_value(

to support this behaviour, so then we would:

async def set_and_wait_for_value(set_signal: SignalRW, set_value, read_signal: Optional[SignalR] = None, read_value = None):
    if read_signal is None:
        read_signal = set_signal
    if read_value is None:
        read_value = set_value
    values_gen = observe_value(read_signal)
    # Do we discard the initial value here?
    status = set_signal.set(set_value)
    async for value in values_gen:
        if value == read_value:
            return status

Timeout handling needs adding, but would this work for your usecase?

[DominicOram]

Yes, I like this. I will do it like that. I think no need to discard initial_value the loop will do that anyway.

[coretl]

I missed something though, the signature is async def set_and_wait_for_value(...) -> Status. This is meant for the use case of a busy record:

# Do a caput -c to Acquire PV, but don't wait for it to complete, then return when Acquire_RBV == 1
finished_status = await set_and_wait_for_value(self.acquire, True)
...
# Wait for the original caput -c to return
await finished_status

Which means a double await for your use case:

await (await set_and_wait_for_value(x, 1, x_rbv))

Now, given the difficulty I've had in explaining that code to people, I'm tempted to change the usage for areaDetector to:

finished_status = self.acquire.set(True)
await wait_for_value(self.acquire, True, timeout=1)
...
await finished_status

That would leave the name free to be:

async def set_and_wait_for_value(set_signal: SignalRW, set_value, read_signal: Optional[SignalR] = None, read_value = None):
    if read_signal is None:
        read_signal = set_signal
    if read_value is None:
        read_value = set_value
    values_gen = observe_value(read_signal)
    # Do we discard the initial value here?
    status = set_signal.set(set_value)
    async for value in values_gen:
        if value == read_value:
            break
    await status

and your use case would shrink to

await set_and_wait_for_value(x, 1, x_rbv)

What do you reckon?

[DominicOram]

Yes, that works for me. Although I have found a typing issue whilst trying to implement this. In the case where all params are supplied the following makes sense as you need not wait on the same type as you set (in fact they are different types in my usecase):

async def set_and_wait_for_value(
    set_signal: SignalRW[T],
    set_value: T,
    read_signal: Optional[SignalR[S]] = None,
    read_value: Optional[S] = None,
...

However, this then obviously causes issues when on the optional case in trying to do:

    if read_signal is None:
        read_signal = set_signal # pyright complains
    if read_value is None:
        read_value = set_value # pyright complains

I can't find a nice answer to this, do you have any thoughts?

[DominicOram]

Ok, from https://diamondlightsource.slack.com/archives/CKW8E0V4H/p1721118847475889 I think we need to split into two functions.

[coretl]

Ok, please can you propose the signatures of those functions? Bonus points if neither of them use the name set_and_wait_for_value so we can deprecate that rather than change its behaviour...

Also FYI we will be adding tolerance to wait_for_value in #447, so at the risk of snowballing this maybe we should consider the set of utility functions in this area we should support based on the use cases in hyperion

[DominicOram]

Initial suggestion for name in the PR.

Now, given the difficulty I've had in explaining that code to people, I'm tempted to change the usage for areaDetector to:

Upon reflection, can I suggest that this is a new issue? It's a wider change, that could be made independently from the race condition discussed here.

[coretl]

Yes, raised #461 to cover the wider change