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

AFG3000: Add support for arbitrary waveform generation #132

Merged
merged 14 commits into from
Jun 7, 2022

Conversation

mgunyho
Copy link
Contributor

@mgunyho mgunyho commented May 24, 2022

Hello again.

This PR adds support for uploading arbitrary waveform data to the AFG3000 series devices. It also contains an example notebook demonstrating this functionality. Tested on a real AFG3252.

I have one comment and one question:

  • The waveform data is expected to be in the range 0..1, which is then scaled to the range voltage_low1...voltage_high1. Now, for example if voltage_low1 is -0.5V and voltage_high1 is +0.5V, 0.5 gets mapped to 0V which might be a bit confusing. But I think this is still the most sensible way to do it if the voltage range is asymmetric.
  • Should we check for overflow in the waveform data? Now if the data goes outside of 0..1, it wraps around, which is probably undesired. Should we issue a warning or an exception?

Note that the commit history also contains a failed attempt at reading back the waveform data from the AFG memory, mostly as a curiosity.

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.

please disable the execution of the notebook, for details on how to do it see here https://qcodes.github.io/Qcodes/examples/writing_drivers/Creating-Instrument-Drivers.html#Documentation

@astafan8
Copy link
Contributor

@mgunyho

Now, for example if voltage_low1 is -0.5V and voltage_high1 is +0.5V, 0.5 gets mapped to 0V which might be a bit confusing. But I think this is still the most sensible way to do it if the voltage range is asymmetric.

could you explain why? in particular, why 0.5 gets mapped to 0 and not to 1? i thought that +0.5 is on the right side of the range, hence it should be mapped to 1, no? or perhaps there's a typo in your example, perhaps you meant "voltage_low1 is +0.5V and voltage_high1 is -0.5V" (low votlage is higher than high voltage)?

@mgunyho
Copy link
Contributor Author

mgunyho commented May 24, 2022

So the values we upload to the waveform are in the range 0..1, so 0.5 (note: not a voltage!) is in the middle of the range. The center of the range -0.5V..+0.5V is 0V, so that's why 0.5 is 0V. (Maybe using +-0.5V as an example range was confusing.)

@mgunyho
Copy link
Contributor Author

mgunyho commented May 24, 2022

please disable the execution of the notebook

Ach, I always forget that! Done.

@astafan8
Copy link
Contributor

So the values we upload to the waveform are in the range 0..1, so 0.5 (note: not a voltage!) is in the middle of the range. The center of the range -0.5V..+0.5V is 0V, so that's why 0.5 is 0V. (Maybe using +-0.5V as an example range was confusing.)

aah! understood, thank you! then it's all correct :)

but do you think it would be beneficial to also add function that would take an array of actual voltages, and then normalize it using voltage high/low settings and send the normalized 0..1-ranged data to the instrument? perhaps this will be less confusing for users (even though they still have to be aware of voltage high/low settings that they are using)

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2022

Codecov Report

Merging #132 (650af77) into master (fe59328) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
- Coverage   19.24%   19.22%   -0.03%     
==========================================
  Files         123      123              
  Lines       14629    14650      +21     
==========================================
  Hits         2816     2816              
- Misses      11813    11834      +21     
Impacted Files Coverage Δ
...codes_contrib_drivers/drivers/Tektronix/AFG3000.py 0.00% <0.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@mgunyho
Copy link
Contributor Author

mgunyho commented May 24, 2022

add a function that would take an array of actual voltages

Hm, this sounds like a good idea, I'll do that later today or tomorrow. Let's not merge before that.

@mgunyho
Copy link
Contributor Author

mgunyho commented May 24, 2022

Should we then skip the 0..1 range completely? So we would have upload_waveform_raw() that uploads raw code values 0..16382 and upload_waveform that directly takes voltages and does the rescaling. And then I think it would make sense to throw an error if voltage values are outside of the voltage_low/high.

@astafan8
Copy link
Contributor

Should we then skip the 0..1 range completely? So we would have upload_waveform_raw() that uploads raw code values 0..16382 and upload_waveform that directly takes voltages and does the rescaling. And then I think it would make sense to throw an error if voltage values are outside of the voltage_low/high.

agree :)

@mgunyho
Copy link
Contributor Author

mgunyho commented May 24, 2022

I started implementing this, but I realized that maybe passing the values as voltages doesn't actually make sense. When you upload a waveform, it's not associated with either of the channels, so we can't know what the voltage limits are. It's possible to output the same waveform on channel 1 and channel 2 with different limits by setting voltage_low1 to be different from voltage_low2! So I think this 0..1 solution is the least confusing way to do it.

@mgunyho
Copy link
Contributor Author

mgunyho commented May 24, 2022

I added the bounds checks, IMO this can be merged now.

@astafan8
Copy link
Contributor

So I think this 0..1 solution is the least confusing way to do it.

indeed, that's fair!

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.

could you fix the mypy errors https://github.com/QCoDeS/Qcodes_contrib_drivers/runs/6573274474?check_suite_focus=true ? they are largely caused by typos or applying operations to a "List[float]" that it does not support (a solution could be to np.array(that_list_of_floats) before doing the operations)

qcodes_contrib_drivers/drivers/Tektronix/AFG3000.py Outdated Show resolved Hide resolved
Aalto QCD labs and others added 14 commits June 7, 2022 17:08
This doesn't completely work. For some reason, after the query, the
AFG keeps spitting out data (self.visa_handle.read_raw() returns
some data), and I couldn't figure out what to do with it based on
the manual. So this is mostly a curiosity that I will revert.
This is easier to understand if the voltage_low/hi is asymmetric.
This should make mypy happy.
@astafan8 astafan8 force-pushed the afg3000-waveform branch from 650af77 to 5a327c9 Compare June 7, 2022 15:08
@astafan8 astafan8 enabled auto-merge June 7, 2022 15:08
@astafan8 astafan8 merged commit c824b73 into QCoDeS:master Jun 7, 2022
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.

3 participants