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

Driver for Tektronix Oscilloscopes: MSO/DPO5000/B, DPO7000/C, DPO70000/B/C/D/DX, DSA70000/B/C/D, and MSO70000/C/DX #1579

Merged
merged 36 commits into from
Jun 6, 2019

Conversation

sohailc
Copy link
Member

@sohailc sohailc commented May 23, 2019

This PR adds a driver for the following oscilloscopes: Tektronix MSO/DPO5000/B, DPO7000/C, DPO70000/B/C/D/DX, DSA70000/B/C/D, and MSO70000/C/DX Series Digital Oscilloscopes.

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 a few comments.

But my biggest problem is the conciseness of the example notebook :) could you make it more descriptive and explain how the driver works and how users can use it? preferably with some plain text next to the code :)

qcodes/instrument_drivers/tektronix/DPO7200xx.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/tektronix/DPO7200xx.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/tektronix/DPO7200xx.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/tektronix/DPO7200xx.py Outdated Show resolved Hide resolved
qcodes/instrument_drivers/tektronix/DPO7200xx.py Outdated Show resolved Hide resolved
… endianness/bytes per samples/signed/unsigned.

WIP: Figuring out how to perform/plot spectral analysis.
@sohailc
Copy link
Member Author

sohailc commented May 25, 2019

@astafan8 Thanks so much for your comments. This was really helpful and I am adjusting my PR according to your comments. My latest commit is still work in progress. Let me send you a message when I think we should have a look at my PR again.

Thanks again!

@sohailc
Copy link
Member Author

sohailc commented Jun 4, 2019

@astafan8 Can we have another look at this? I did my best to apply the modifications you suggested.

Thanks,

Sohail

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.

Thanks for the improvements!

I left a minor comment, and once you confirm that this code has been tested on the actual instrument and works ok, i'll merge.

More yaml-simulation-based tests would be better, but i leave this up to you.

@astafan8 astafan8 changed the title Tek dpo 72004 c Driver for Tektronix Oscilloscopes: MSO/DPO5000/B, DPO7000/C, DPO70000/B/C/D/DX, DSA70000/B/C/D, and MSO70000/C/DX Jun 4, 2019
@astafan8 astafan8 added the driver label Jun 4, 2019
astafan8
astafan8 previously approved these changes Jun 4, 2019
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.

I'll merge after:

  • There is some sphinx issue that needs to be fixed for CI.
  • Was this code tested on a real device? (need a confirmation)

@codecov
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

Merging #1579 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1579   +/-   ##
=======================================
  Coverage   72.34%   72.34%           
=======================================
  Files         116      116           
  Lines       12390    12390           
=======================================
  Hits         8964     8964           
  Misses       3426     3426

@sohailc
Copy link
Member Author

sohailc commented Jun 5, 2019

@astafan8, please do not merge this yet. There are some missing features I forgot to add.

Specifically, I need to add a submodule for triggering configuration.

…ements. E.g. Instead of having to do `tek.measurement[0].type('amplitude')` followed by `tek.measurement[0].value()`, we can now simply do `tek.measurement[0].amplitude()`
@astafan8 astafan8 dismissed their stale review June 5, 2019 23:28

De-approving the PR until a new feature is added

@astafan8
Copy link
Contributor

astafan8 commented Jun 5, 2019

@sohailc ok, fine.

But why does that feature have to be added in THIS PR? Can't we merge this first, and then the submodule is added in another PR?

2) measurements is a list of tuples (name, unit)
@sohailc
Copy link
Member Author

sohailc commented Jun 5, 2019

I thought it handier to add all needed features in this PR. Anyway, I have added the submodule now so if it passes the tests and you approve of the code, can you please merge it in.

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.

Good!

@astafan8 astafan8 merged commit 657399c into microsoft:master Jun 6, 2019
giulioungaretti pushed a commit that referenced this pull request Jun 6, 2019
Merge: 5f6fd68 712c4b4
Author: Mikhail Astafev <astafan8@gmail.com>

    Merge pull request #1579 from sohailc/tek_dpo_72004C
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants