-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(DiamondLightSource/hyperion#1464) Change topup gating to prevent gating with long exposures #674
base: main
Are you sure you want to change the base?
(DiamondLightSource/hyperion#1464) Change topup gating to prevent gating with long exposures #674
Conversation
…ing with long exposures
5b6b6a0
to
15b4379
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #674 +/- ##
==========================================
+ Coverage 94.17% 94.23% +0.06%
==========================================
Files 108 108
Lines 4306 4319 +13
==========================================
+ Hits 4055 4070 +15
+ Misses 251 249 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks. Some optional comments to help with working out why this the behaviour in the future. Otherwise looks good.
@@ -23,15 +39,28 @@ def _gating_permitted(machine_mode: SynchrotronMode): | |||
return False | |||
|
|||
|
|||
def _delay_to_avoid_topup(total_run_time, time_to_topup): | |||
def _delay_to_avoid_topup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could: We could have a docstring on this function explaining why we need the threshold and why sometimes wait and sometimes don't (basically just summarising this issue).
# For planned exposures less than this value, wait for topup to finish instead of | ||
# collecting throughout topup. | ||
THRESHOLD_EXPOSURE_S = "dodal_topup_threshold_exposure_s" | ||
# Additional safety margin to wait after the end of topup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: A little bit more detail on why we need the safety margin would be good.
Fixes DiamondLightSource/hyperion#1464
After conversation with beamline staff, rather than implement the same solution as exists currently in GDA, the following behaviour is implemented:
beamlineParameters
:dodal_topup_threshold_exposure_s
(default 120) controls the exposure time at which hyperion gates topupsdodal_topup_end_delay_s
(default 1) controls an additional adjustable delay after topup end to ensure no accidental overlap with topup due to timing inaccurancies and can be adjusted to allow further time for settling if necessary.To determine whether a topup is performed:
COUNTDOWN
PV != -1 and Synchrotron mode isUser
orSpecial
then the gating steps are followed as below, otherwise we proceed with the scan as normal.ENDCOUNTDN
PV), if it is more than this then the scan may be delayed, otherwise the scan is not delayed.dodal_topup_threshold_exposure_s
and if it is less than this the scan is delayed, otherwise it is considered a long duration scan which can be performed during topup and is not delayed.ENDCOUNTDN - COUNTDOWN + dodal_topup_end_delay_s
seconds rounded up to the nearest second.Note:
It is expected that the location of these parameters is temporary until at some later stage they are moved into the config server.
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}