bluesky / yaqc-bluesky

A bluesky interface to the yaq instrument control framework.
https://yaq.fyi/
BSD 3-Clause "New" or "Revised" License
8 stars 4 forks source link

Loading looping triggered sensors hangs #83

Closed ksunden closed 2 years ago

ksunden commented 2 years ago

https://github.com/bluesky/yaqc-bluesky/blob/master/yaqc_bluesky/_is_sensor.py#L10

Offending line, could possibly update "wait until still" to read measurment id instead of busy for triggered sensors.

non-triggered sensors just have a null trigger message which is immediately marked as done

Triggered sensors call measure in trigger (see _has_measure_trigger.py) then wait_until_still which runs in a thread (see _base.py, last method)

This in turn waits until it goes not busy, which it never returns from

it's needed to get a measurement at init time (at least for our hardware... we should maybe make it a requirement for yaq that everything be populated correctly, at which point we could remove that....  So essentially this comes down to "I understand what is happening, but not sure what the path forward is yet"

untzag commented 2 years ago

How about this:

while True:
    names = self.yaq_client.get_channel_names()
    if names:
        break
    time.sleep(0.01)
ksunden commented 2 years ago

I think that trigger for has-measure-trigger maybe just looks at measurement-id

The thing is that some daemons (ni daq in particular) requires a measurement to have correct channel names.... it should be fixed on the daemon end perhaps, but is a larger fix there than this... (and not sure if other daemons have similar behavior)

untzag commented 2 years ago

My assumption is that channel names is an empty list until the first successful measurement. If channel names is not empty we don't have to wait at all.

ksunden commented 2 years ago

most sensors it is not empty, ni specifically starts with the channel names it can tell by config, but ignores those that are provided by shots processing

ksunden commented 2 years ago

that likely applies to gage as well

untzag commented 2 years ago

OK, I'm wrong. My first thought is that's out of specification, but clearly it's something we do.

Given this, the robust solution is to ask the daemon for metadata each time describe is called. If we cannot reliably cache this, let's not try.

untzag commented 2 years ago

We could use the new functionality introduced by our friends at Diamond light source---the run engine now respects async describe.

ksunden commented 2 years ago

I would support writing it into the spec, It actually appears that it does not affect gage, so pretty sure its only ni that needs addressing

untzag commented 2 years ago

Is this what you are thinking fora pragmatic spec?

MAY return empty metadata] at startup until first measurement, MUST not change contents of populated metadata once first measurement completes and metadata populates.

Would apply to

Would also apply to mapping metadata?

ksunden commented 2 years ago

Mapping can explicitly change, I'm not too fussed about if the keys change as we have mapping id to invalidate caches.

I would support channel names shapes and units being required to be fully correct at init time so if a measurement is required then init must sort that out... I could see some minor room for wiggle room that like the measurement is started in init but for some reason (e.g. the daq not having a trigger) does not complete in a reasonable time, that is undefined behavior and I'm not too interested being too hard on that.

I think perhaps we can sort out NI such that we can get that info from shots processing without real data.

untzag commented 2 years ago

I think we'll need to have a high bandwidth discussion @ksunden

untzag commented 2 years ago

@ksunden and I discussed this and decided that empty channel_names and similar are out of spec. yaqd-ni will need to be updated and yep language will be clairified

untzag commented 2 years ago

leaving this issue open for the time being, as this behavior will be deleted once all dameons are in-spec