Skip to content
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

Limit maximum buffer size as documented for AlazarBeforeAsyncRead #1535

Merged

Conversation

jenshnielsen
Copy link
Collaborator

As documented here http://www.alazartech.com/Support/Download%20Files/ATS-SDK-Guide-7.2.3.pdf#section*.110

We do not use the headers so that is not taken into account. My testing on the 9360 seems to indicate that it actually supports upto 96 MB but to stay on the safe side I have gone with the docs. We do not support and never will the old pci cards with lower limits so we do not need to take that into account

@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #1535 into master will decrease coverage by 0.01%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master    #1535      +/-   ##
==========================================
- Coverage   71.33%   71.31%   -0.02%     
==========================================
  Files         104      104              
  Lines       12041    12045       +4     
==========================================
+ Hits         8589     8590       +1     
- Misses       3452     3455       +3

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some clarification comments. And let's of course wait for the response from AlazarTech.

qcodes/instrument_drivers/AlazarTech/ATS.py Outdated Show resolved Hide resolved
# 64 MB see docs of AlazarBeforeAsyncRead
# http://www.alazartech.com/Support/Download%20Files/ATS-SDK-Guide-7.2.3.pdf#section*.110

requested_buffer_size = (bits_per_sample * samples_per_record *
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it hard to understand why number_of_channels of channels is not involved in this formula. The respective formula in SDK pdf also does not contain number_of_channels. Why is that? Are on-board-memory-buffers considered per channel, and host-memory-buffers not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This buffer size is only a fraction of the total memory (1/64) in this case. I don't think that the limitation comes from filling the internal memory but from some other implementation detail that they don't reveal to use

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This buffer size is only a fraction of the total buffer size (1/64) in this case.

Sorry, i'm getting more confused. What did you mean with "this buffer size" and "total buffer size" and where does "1/64" comes from? and how does this relate to taking number_of_channels into account or not?

... from some other implementation detail ...

i agree with you, i have the same feeling.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry should have said total onboard memory. I mean given that we don't know the reason for this limitation (other than it's for sure not filling all the memory cf the card which would for sure depend on number of channels) all questions about this are speculations. This limitation might just be that the whatever fpga logic that captures data from a single channel only has a memory address space large enough to address 64 MB and there is a matching address space for the next channel. We really don't know at this stage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks for explaining! I hope AlazarTech come back with useful answers :)

qcodes/instrument_drivers/AlazarTech/ATS.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/AlazarTech/ATS.py Outdated Show resolved Hide resolved
@jenshnielsen
Copy link
Collaborator Author

@QCoDeS/core We don't really have a useful answer from AlazarTech. They confirm that this limit is here (at least on some cards) but also recommend to make the buffer smaller than this in general < 8 MB.
I think we should put this in as is since it at least makes it much less confusing for users.

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get it in and make our users happy! :)

qcodes/instrument_drivers/AlazarTech/ATS.py Outdated Show resolved Hide resolved
Co-Authored-By: jenshnielsen <jenshnielsen@gmail.com>
@jenshnielsen jenshnielsen merged commit c25215f into microsoft:master May 2, 2019
giulioungaretti pushed a commit that referenced this pull request May 2, 2019
Merge: 5a2ed14 5f1e987
Author: Jens Hedegaard Nielsen <Jens.Nielsen@microsoft.com>

    Merge pull request #1535 from jenshnielsen/alazar_limit_buffer_size
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.

None yet

2 participants