-
Notifications
You must be signed in to change notification settings - Fork 2
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
Set up i24 serial to run on procserv #577
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #577 +/- ##
==========================================
+ Coverage 78.39% 78.64% +0.25%
==========================================
Files 96 96
Lines 6850 6842 -8
==========================================
+ Hits 5370 5381 +11
+ Misses 1480 1461 -19
|
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.
Looks good, I've pressed approve but some of my comments may need addressing before merging, have a read and see if any of them are urgent enough
@@ -85,7 +85,8 @@ def calculate_collection_timeout(parameters: FixedTargetParameters) -> float: | |||
Returns: | |||
The estimated collection time, in s. | |||
""" | |||
buffer = 30 | |||
buffer = PMAC_MOVE_TIME * parameters.total_num_images + 2 | |||
# buffer = 30 |
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.
Did you mean to leave this comment in? It might be explain why we need a buffer for the timeout
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.
Ah, no, good spot
logger = logging.getLogger("I24ssx.fixed_target") | ||
|
||
PMAC_MOVE_TIME = 0.008 # Move time between positions on chip ~ 7-8 ms |
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.
Will be good to have constants like this moved into more organised locations, but perhaps better to wait for me to finish my work on this
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.
Good point. In this case I was also half considering having this constant in dodal
, but I don't have a strong opinion on this. Will make a note in the code to do this
@@ -474,24 +475,8 @@ def start_i24( | |||
elif parameters.detector_name == "eiger": |
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.
Nit: There is some duplicated code between this file and i24ssx_Extruder_Coolect_py3v2.py
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.
Yeah, that probably won't be fixed until we get around to #62 and we can finish tidying up the code...
@@ -6,6 +7,13 @@ | |||
logger = logging.getLogger("I24ssx.run") | |||
|
|||
|
|||
def _parse_input(expt: str): |
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.
If this file is the main entry point, it would be nice to be able to run this file directly, eg with a
if __name__ == "__main__":
...
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.
Ah no, the entry points are the two functions that are being run directly (see in pyproject.toml
), which is why we don't need to use if name etc... it would actually make the code slightly more complicated.
# on a python IOC to run serial on I24 with procserv | ||
|
||
# Get the directory of this script | ||
current_directory=$( realpath "$( dirname "$0" )" ) |
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.
Do we need protection against running this script when the ioc and/or when blueapi is already running?
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.
Not really... I mean, we could add something but blueAPI will not start and just raise and error if already running - on procserv or otherwise - because the port in the stomp configuration is already occupied.
Also, procserv right now can only run on one machine at a time on I24 because the control machine is still on RHEL7 so we've kind of already got two levels of "you shall not run"
Closes #573
Needs more testing on the beamline