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/infiniium additional parameters #1203

Conversation

WilliamHPNielsen
Copy link
Contributor

@WilliamHPNielsen WilliamHPNielsen commented Jul 24, 2018

Changes proposed in this pull request:

  • Add all the measurement parameters in a submodule
  • Improve driver safety slightly by switching off headers in __init__. If headers are on, instrument responses ping back the query as well as the value, and our get_parsers can't handle that

Question: should the measurement submodule be snapshotted?

@QCoDeS/core

@codecov
Copy link

codecov bot commented Jul 24, 2018

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1203   +/-   ##
=======================================
  Coverage   80.16%   80.16%           
=======================================
  Files          49       49           
  Lines        6762     6762           
=======================================
  Hits         5421     5421           
  Misses       1341     1341

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.

the code looks ok.

I guess that the parameters that are settable can be snapshotted, but in general I do not see a big point since it is a measurement submodule, and it does not seem to have "settings".

@WilliamHPNielsen
Copy link
Contributor Author

I do not see a big point since it is a measurement submodule, and it does not seem to have "settings"

That's also what I am thinking. They describe the state of the signal, not the state of the instrument.

get_parser=float,
unit='V')

def _meas(self, cmd: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want this to be a static method. Also imho the name is slightly misleading. How about _generate_meas_str or some such thing?

parameters
"""
# note: this is not really a channel, but InstrumentChannel does everything
# a 'Submodule' class should do
Copy link
Contributor

Choose a reason for hiding this comment

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

We had a discussion in a bug report about renaming InstrumentChannel for this reason (can't find the discussion right now though). Perhaps we should revisit?

Copy link
Contributor

@astafan8 astafan8 Jul 30, 2018

Choose a reason for hiding this comment

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

this can be very easily solved by renaming InstrumentChannel class to SubModule (or SubInstrument, whatever) and adding InstrumentChannel = SubModule after SubModule class definition.

SubModule seems like a good name. But what "should this class do"? Contain a piece of functionality with tight reference/coupling to the instrument that it is a part of?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the name SubModule too. I think InstrumentChannel already is that class. If you guys would like to discuss this more, then let us perhaps open a separate issue.

@WilliamHPNielsen
Copy link
Contributor Author

I'd say this is ready to land.

@WilliamHPNielsen WilliamHPNielsen merged commit 92f9677 into microsoft:master Jul 30, 2018
@WilliamHPNielsen WilliamHPNielsen deleted the driver/Infiniium_additional_parameters branch July 30, 2018 14:40
giulioungaretti pushed a commit that referenced this pull request Jul 30, 2018
Merge: dada1fe 9deb12a
Author: William H.P. Nielsen <whpn@mailbox.org>

    Merge pull request #1203 from WilliamHPNielsen/driver/Infiniium_additional_parameters
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

4 participants