Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lcontami
Copy link
Member

  • some functions for using the psa
  • a small change in the acquisition card driver
  • a few errors corrected ?
  • new safeguards for the cryomagnetic sources that are super buggy during communication

These are probably old and I did not test the PSA driver.
However the acquisition card and CS4 work fine with this code.

- some functions for using the psa
- a small change in the acquisition card driver
- a few errors corrected ?
- new safeguards for the cryomagnetic sources that are super buggy during communication
@MatthieuDartiailh MatthieuDartiailh self-requested a review January 30, 2019 15:31
if self.condition_callable():
return True
if remaining_time < 0 or break_condition_callable():
return False
time.sleep(min(refresh_time, remaining_time))
Copy link
Member

Choose a reason for hiding this comment

The 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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@rassouly rassouly Jun 14, 2019

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

@@ -99,6 +104,7 @@ def open_connection(self):

"""
cu = ADQControlUnit(self._dll)
cu.enable_logging(3, b"C:\\Logs")
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

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)

Copy link
Member Author

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 ?

Copy link
Contributor

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.

@MatthieuDartiailh
Copy link
Member

Travis fails because we call directly a fixture function in a test (which we should not do), this should be an easy fix.

@MatthieuDartiailh
Copy link
Member

#53 should fix the tests failures, so a simple rebase should allow Travis to go green.

@MatthieuDartiailh
Copy link
Member

This is not a really clean solution for restarting a driver. I will try to have a look next week.

Base automatically changed from master to main January 19, 2021 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants