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/keithley s46 #1409

Merged
merged 29 commits into from
Dec 7, 2018
Merged

Driver/keithley s46 #1409

merged 29 commits into from
Dec 7, 2018

Conversation

sohailc
Copy link
Member

@sohailc sohailc commented Dec 4, 2018

Adds a driver for the S46 RF switch

@sohailc sohailc added the driver label Dec 4, 2018
@sohailc sohailc self-assigned this Dec 4, 2018
@codecov
Copy link

codecov bot commented Dec 4, 2018

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1409   +/-   ##
=======================================
  Coverage   74.13%   74.13%           
=======================================
  Files          85       85           
  Lines        9822     9822           
=======================================
  Hits         7282     7282           
  Misses       2540     2540

@jenshnielsen
Copy link
Collaborator

Looks good, Rather than using your own caching i think you should consider using the caching build in to the parameter already via the get_latest function. If you look at the QDac driver https://github.com/QCoDeS/Qcodes/blob/master/qcodes/instrument_drivers/QDev/QDac_channels.py you can see that this has a similar function that returns the full status of all the channels. This is called in _get_status exactly once when initialized and then updates the value of all the voltages on all the channels. After that the snapshot and other functions that needs the value of all channels can simply ensure that _get_status is called exactly once before using all the values

"""
Please do not write ':OPEN ALL' to the instrument as this will
circumvent the lock.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this slow compared to :OPEN ALL? Is that an issue or perhaps it doesn't matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its probably slower, but I do not think it matters.

super().__init__(*args, **kwargs)

def ask(self, cmd: str):
self._ask_log.append(cmd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could probably do this with the existing logging in the instrument and pytests log capturing functions

…g. A1, B2, etc)

2) Use the caching functions of the parameters instead of rolling your own
3) Use the visa logger in the test to verify that ":CLOS ?" is called only once.
"""
A four channel-per-relay instrument
"""
io_stream = StringIO()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still think it's cleaner to use pytests log capturing rather than rolling our you own https://docs.pytest.org/en/latest/logging.html

Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

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

Looks good. You might want to consider overwriting the snapshot function too like the qdac does. Once the instrument is added to the qcodes station is will do the equivalent of a inst.snapshot(update=True) which I think might still be slow?

@sohailc sohailc merged commit 7e38ca5 into microsoft:master Dec 7, 2018
@sohailc sohailc deleted the driver/keithley_s64 branch December 7, 2018 19:25
giulioungaretti pushed a commit that referenced this pull request Dec 7, 2018
Merge: 1483359 86be1ff
Author: sohail chatoor <schatoor@gmail.com>

    Merge pull request #1409 from sohailc/driver/keithley_s64
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