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

Add set_dac_unit method to D5a instrument driver #1582

Merged
merged 5 commits into from
Jun 1, 2019
Merged

Add set_dac_unit method to D5a instrument driver #1582

merged 5 commits into from
Jun 1, 2019

Conversation

qSaevar
Copy link
Contributor

@qSaevar qSaevar commented May 27, 2019

Changes proposed in this pull request:

  • Add set_dac_unit method to D5a instrument for chaning the units and gain between volts and millivolts.

@astafan8

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.

Once that comment of mine is responded to, i'll approve.

But actually - i would really love the driver to be refactored so that there is a single "variable" that knows whether volts or millivolts are used. Now there is ._gain, ._mV, and the dac_unit parameter. But i leave this refactoring up to the author.

@codecov
Copy link

codecov bot commented May 29, 2019

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1582   +/-   ##
=======================================
  Coverage   72.18%   72.18%           
=======================================
  Files         112      112           
  Lines       12293    12293           
=======================================
  Hits         8874     8874           
  Misses       3419     3419

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.

Great, thanks for removing self._mV.

I am sorry, now I noticed something that I haven't seen before. Why is set_dac_unit a parameter? Why is it not a normal method of the driver? What is the benefit of one .get()-ing the set_dac_unit parameter?

If there's none, then i'd just make it a bound method (set_dac_unit name is fine). and in the body of that method (which will be mostly copy-pasting _set_dac_unit), for validating the input argument the same validator Enum('mV', 'V') can be used - just call it's .validate(...) method with the set_dac_unit's input argument.

@qSaevar
Copy link
Contributor Author

qSaevar commented May 31, 2019

@astafan8 as there is no real need for .get()-ing the set_dac_unit parameter I converted it to a method as you suggested.

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.

Great, thanks!

i took the initiative to make the gain value selection logic more explicit than "if-else" stuff. Do you mind?

Once the checks say "green", i'll merge.

@astafan8 astafan8 changed the title Add dac_unit parameter to D5a instrument driver. Add set_dac_unit method to D5a instrument driver May 31, 2019
@qSaevar
Copy link
Contributor Author

qSaevar commented May 31, 2019

No, I don't mind. Thanks

@astafan8 astafan8 merged commit e2bce7c into microsoft:master Jun 1, 2019
giulioungaretti pushed a commit that referenced this pull request Jun 1, 2019
Merge: 1fda5c5 c3106cd
Author: Mikhail Astafev <[email protected]>

    Merge pull request #1582 from qutech-sd/add_dac_unit_parameter
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.

2 participants