pcdshub / pcdsdaq

Utilities for using the DAQ's pydaq, pycdb, and pyami libraries in conjunction with Bluesky
https://pcdshub.github.io/pcdsdaq
Other
0 stars 9 forks source link

API: DAQ as a readable device #27

Closed ZLLentz closed 6 years ago

ZLLentz commented 6 years ago

Description

Apologies in advance for breaking the API.

Motivation and Context

daq.configure(events=120)
RE(scan([daq], 0, 10, 11))

Is better than

RE(scan([], 0, 10, 11, per_step=calib_at_step(events=120)))

And this structure means we can use bluesky subscriptions to implement the scan PVs later, whereas the repeated flyer approach doesn't increment the sequence number the same way.

I opted to remove the calib_cycle stuff so that the module was easier to understand, and so that when we do implement subscriptions that rely on event documents we don't have people accidentally using calib_cycle and wondering why the scan PVs don't update.

I also opted to remove the mode configuration parameter because it was superfluous, complicated, and useless. You were extremely likely to get stuck in the wrong mode. Now we just support trigger_and_read to read when you want to, daq_wrapper to read for the whole plan, and stage/unstage to manage the starting and stopping of daq runs.

How Has This Been Tested?

Adjusted the tests. (Full coverage now! Wow!) I would like to spend quality time in a hutch with this for "feel" tests before thinking about merging.

Where Has This Been Documented?

Lots of docs commits in this repo

codecov-io commented 6 years ago

Codecov Report

Merging #27 into master will increase coverage by 0.76%. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #27      +/-   ##
========================================
+ Coverage   99.23%   100%   +0.76%     
========================================
  Files           2      2              
  Lines         393    356      -37     
========================================
- Hits          390    356      -34     
+ Misses          3      0       -3
Impacted Files Coverage Δ
pcdsdaq/daq.py 100% <100%> (+0.55%) :arrow_up:
pcdsdaq/preprocessors.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0a1ae12...412cb51. Read the comment docs.

teddyrendahl commented 6 years ago
  1. Are we guaranteed that unstage happens after end_run? If not we might leave the DAQ hanging (unsubscribe before ending run)

  2. Do we want to deprecating the mode. Could we do something similar where have a "calib" mode that watches for create and save?

ZLLentz commented 6 years ago
  1. This happens in all the built-ins, but it is not guaranteed. Perhaps unstage should also do end_run to prevent the desync.
  2. I'd much rather re-implement this as a preprocessor that inserts kickoff at create and complete at save instead of the existing msg_hook gore
teddyrendahl commented 6 years ago
  1. 👍
  2. Kind of disagree. One ring to rule them all! Even if it is a little nasty. I don't like having to put two wrappers on for the DAQ.
ZLLentz commented 6 years ago

I'm not bringing the message hook gore back under any conditions, but if you think having a mode=calib-like behavior is valuable (e.g. people will use it) then we can re-implement as a preprocessor. When would we use this?

teddyrendahl commented 6 years ago

I'm sorry. I think I might have missed something by having the docs split up. If I want a flyer I use daq_wrapper this runs the entire scan in a single run. If I want calib cycles I configure for "n" events and pass it as a regular detector.

If this is correct, I vote for a rename of daq_wrapper to indicate the flying nature.

ZLLentz commented 6 years ago

This is reasonable, daq_wrapper was a catch-all for when we were going to require it on every daq plan. Any suggestions? daq_during_wrapper?

teddyrendahl commented 6 years ago

Sure, obvious link to the fly_during_wrapper

ZLLentz commented 6 years ago

Some final thoughts here:

teddyrendahl commented 6 years ago

Separate PR. Test with pcdsdaq first to see if events is accurate at all.