-
Notifications
You must be signed in to change notification settings - Fork 11
These were left on the cryofree computer; there is: #52
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,6 +67,11 @@ def destroy_board(self, b_id): | |
self.library.ControlUnit_DeleteADQ(self.id, b_id) | ||
del self._boards[b_id-1] | ||
|
||
def enable_logging(self, level, path): | ||
""" | ||
""" | ||
self.library.ControlUnit_EnableErrorTrace(self.id, level, path) | ||
|
||
def _cleanup(self): | ||
"""Make sure we disconnect all the boards and destroy the unit. | ||
|
||
|
@@ -99,6 +104,7 @@ def open_connection(self): | |
|
||
""" | ||
cu = ADQControlUnit(self._dll) | ||
cu.enable_logging(3, b"C:\\Logs") | ||
boards = cu.list_boards() | ||
board_id = None | ||
for i, b in enumerate(boards): | ||
|
@@ -216,6 +222,8 @@ def get_traces(self, channels, duration, delay, records_per_capture, | |
get_data = self._dll.GetData.func | ||
retrieved_records = 0 | ||
while retrieved_records < records_per_capture: | ||
# WHY ??? | ||
time.sleep(0.002) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why indeed? Depending on the length of the traces this could lead to a large accumulation of traces and slow down the acquisition. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a fix for various intermittent and hard to debug issues we had and it was absolutely needed for us at the time. I suspect that this solves some sort of race condition. It's very hard to reproduce and the issue probably comes from the dll driver itself. The time we chose is a compromise between the frequency of crashes and the speed of the measurement so I think this should stay in the local cryofree version but it shouldn't go on the master branch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you have the log, you could contact the manufacturer. Last time it took about an afternoon to fix the bug. |
||
# Wait for a record to be acquired. | ||
n_records = (acq_records(cu, id_) - retrieved_records) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,11 +202,11 @@ def wait_for_completion(self, break_condition_callable=None, timeout=15, | |
while True: | ||
remaining_time = (timeout - | ||
(time.time() - timeout_start)) | ||
time.sleep(min(refresh_time, remaining_time)) | ||
if self.condition_callable(): | ||
return True | ||
if remaining_time < 0 or break_condition_callable(): | ||
return False | ||
time.sleep(min(refresh_time, remaining_time)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is meaningless, we check the condition before entering the while loop so checking again after a couple of operations won't do any good. |
||
|
||
def cancel(self, *args, **kwargs): | ||
"""Cancel the long running job. | ||
|
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.
This is ugly ! We should pass a path for logging to the instrument as part of its setting.
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.
At the very least it should be behind an os check (the ADQ can run linux) and we should try to get a user related path that is more likely to be writable.
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.
This was just a quick hack I wrote to debug the issues I was having. It can be safely removed from this patch. (And added cleanly in the future)
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.
what do you mean added cleanly ? is this needed or not ?
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.
This is not needed, I added this to debug an issue with the sp_adq14. What I meant by added cleanly was the fact that we could have a settings somewhere to toggle logging on and off where we could choose the logging directory instead of hard coding all of that.