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 instrument driver for oxford kelvinox. #707

Merged
merged 5 commits into from
Aug 21, 2017

Conversation

CJvanDiepen
Copy link
Contributor

Add instrument driver for Oxford Kelvinox IGH.

Changes proposed in this pull request:
Add instrument driver

@jenshnielsen

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.

Left a few inline comments. Otherwise looks good to me

set_cmd=self._set_still_power)
self.add_parameter('sorb_temp',
unit='K',
get_cmd=self._get_still_power,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like the wrong cmd, same below for the set cmd

@@ -0,0 +1,534 @@
# OxfordInstruments_Kelvinox_IGH.py class, to perform the communication between the Wrapper and the device
Copy link
Collaborator

Choose a reason for hiding this comment

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

OxfordInstruments_Kelvinox_IGH.py to either OxfordInstruments_Kelvinox_IGH or kelvinox.py

separators so we accept them here as well.

Returns:
A dict containing vendor, model, serial, and firmware.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This produces a warning in the docs build

/home/travis/build/QCoDeS/Qcodes/qcodes/instrument_drivers/oxford/kelvinox.py:docstring of qcodes.instrument_drivers.oxford.kelvinox.OxfordInstruments_Kelvinox_IGH.get_idn:1: WARNING: Inline emphasis start-string without end-string.

It's better to write it as `*IDN?` to escape the *

self.add_parameter('V12A_valve',
unit='%',
get_cmd=self._get_V6_valve,
set_cmd=self._set_V6_valve)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrong cmd?

"""
idparts = ['Oxford Instruments', 'Kelvinox IGH', None, None]

return dict(zip(('vendor', 'model', 'serial', 'firmware'), idparts))
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no good way to get the firmware/serial number?

self.remote()
log.info('Set still and sorb status')
self._execute(status)
self.local()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if it was in remote mode before executing this command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the way it was operated before and we don't have a reason to change it, so I think it is best to leave it like it is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok cool

@CJvanDiepen
Copy link
Contributor Author

@jenshnielsen Please take another look! If you are happy with it then I am too ;)

@jenshnielsen jenshnielsen merged commit c1760c7 into microsoft:master Aug 21, 2017
@eendebakpt eendebakpt deleted the kelvinox_driver branch November 12, 2018 10:40
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

3 participants