Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #941 +/- ##
==========================================
+ Coverage 97.38% 97.42% +0.03%
==========================================
Files 143 143
Lines 6087 6087
==========================================
+ Hits 5928 5930 +2
+ Misses 159 157 -2 ☔ View full report in Codecov by Sentry. |
4a373d5 to
820f4d8
Compare
…_for() instead of timeout()
|
CI initially failed due to a flakey unit test unrelated to this PR, it passed after re-running. See #943 |
There was a problem hiding this comment.
Thanks, this looks great so far!
I've left a few comments, but the main one is something I incorrectly suggested to you, which was to not bother running set in the background by using asyncio.create_task. The original code ran set asynchronously, so we should try to keep that.
By the way, if you're curious, this was the recently added policy for reviews in dodal: https://diamondlightsource.github.io/dodal/main/explanations/reviews.html
olliesilvester
left a comment
There was a problem hiding this comment.
Looks mostly good now, just a couple more things. When you're happy with this, we can create a short bluesky plan to test this device on the beamline
…sing asyncio.timeout for exception
There was a problem hiding this comment.
This seemed to work well during testing. A few minor features were requested by David on I04:
- Add the signals for the read-back values for the number of lenses and the vertical beam size,
BL04I-MO-FSWT-01:NUM_FILTERSandBL04I-MO-FSWT-01:VER - After the set is done, log the readbacks for the above
olliesilvester
left a comment
There was a problem hiding this comment.
Thanks! Looks neater than the old device 🥳
| is_set_filters_done = True | ||
|
|
||
| # The value hasn't changed so assume the device is already set up correctly | ||
| async for value in observe_value(self.predicted_vertical_num_lenses): |
There was a problem hiding this comment.
Must: I think there's a potential infinite loop here. See #995
| ): | ||
| # We can only put an integer number of lenses in the beam but the | ||
| # calculation in the IOC returns the theoretical float number of lenses | ||
| nonlocal is_set_filters_done |
There was a problem hiding this comment.
Could: I always think nonlocal is a bit of a code smell. For example, in this case we could just return the value but I'm also not 100% sure of the purpose of is_set_filters_done
Fixes #748
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}