Conversation
|
Comments from #816
|
|
We should remember to include scipy in the |
|
I am not so sure. I think it should be an optional dependency, and definitely not for this branch as it does not use Scipy at all |
|
@jenshnielsen Could you let know when it is ready for review? We're using the ATS9440, which has some quirks such as 4 channels instead of 2. It also uses ATS.py, and I'd like to double check if the updated file is still compatible |
|
@nulinspiratie @damazter @sohailc I think this is more or less ready for review. I will do one more pep8 pass over it before merging @nulinspiratie the only major api break that I know about is the change to the signature of handle parameter which now needs to take another parameter. |
| the number of samples (per channel) per second | ||
| """ | ||
| if self.sample_rate.get() == 'EXTERNAL_CLOCK': | ||
| if (self.clock_source.get() == 'EXTERNAL_CLOCK_10MHz_REF' |
There was a problem hiding this comment.
where did the decimation part of this function go?
There was a problem hiding this comment.
furthermore, is th elogic here correct? when there is an external clock, then the sample rate is still determined by the internal clock setting I think
There was a problem hiding this comment.
The decimation is handled below like its always been.
If clock_source == 'EXTERNAL_CLOCK_10MHz_REF' and there is a external_sample_rate parameter it will use that. Alternatively it will fall back to the internal sample rate if that parameter is not defined. That is to accommodate for the 2 different Alazar Cards. One has a freely settable sample rate with a 10 MHZ external clock and the other one only has one possible rate in that case.
If there is a pure external_clock there is still no automatic way to infer the sample rate.
| # We use the matching one and mark the order one | ||
| # as up to date since it's not being pushed to | ||
| # the instrument at any time and is never used | ||
| if clock_source == 'EXTERNAL_CLOCK_10MHz_REF': |
There was a problem hiding this comment.
why do you not put a call to self.get_sample_rate here?
There was a problem hiding this comment.
Because that would include the decimation which we don't want here but I have changed the code to make the decimation correction optional instead.
This reverts commit 7b15644. It does not work due to the way alazar parameters must be updated before they can be get
nulinspiratie
left a comment
nulinspiratie
left a comment
There was a problem hiding this comment.
Aside from the binary handle (to support 4-channel ATS instruments) it looks good!
| @@ -408,9 +459,10 @@ def config(self, clock_source=None, sample_rate=None, clock_edge=None, | |||
| self.parameters['coupling' + str(i)], | |||
There was a problem hiding this comment.
handle should be binary, i.e. 2**(i-1). This should only matter for 4-channel ATS, like the ATS9440
| self.parameters['bwlimit' + str(i)]) | ||
| if bwlimit is not None: | ||
| self._call_dll('AlazarSetBWLimit', | ||
| self._handle, i, |
There was a problem hiding this comment.
not sure if handle should be binary here as well, the ATS9440 doesn't have bw limit
| acquisition_controller.handle_buffer(buf.buffer) | ||
| try: | ||
| for buf in self.buffer_list: | ||
| self._call_dll('AlazarPostAsyncBuffer', |
There was a problem hiding this comment.
When performing long acquisitions, AlazarPostAsyncBuffer has sometimes return an ApiFailed error. I haven't figured out why yet. Anyone else experienced this as well? If so, does this PR fix this issue? Otherwise, would it make sense to add a try except statement here that frees the buffer memory?
Codecov Report
@@ Coverage Diff @@
## master #860 +/- ##
=======================================
Coverage 78.59% 78.59%
=======================================
Files 45 45
Lines 6415 6415
=======================================
Hits 5042 5042
Misses 1373 1373 |
Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com>
Ats9360 driver only (#860)
4 participants
This is the driver only part of #816 which is hopefully easier to review.
I still have to address @damazter points from the other pr before this is ready to review