-
Notifications
You must be signed in to change notification settings - Fork 320
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
Keithley 2400 protect read commands #1171
Keithley 2400 protect read commands #1171
Conversation
In the Keithley 2400 doing a read with output disabled is an error that leaves the insturment in a state where no commands can be sent to it without restart
Codecov Report
@@ Coverage Diff @@
## master #1171 +/- ##
=======================================
Coverage 79.66% 79.66%
=======================================
Files 47 47
Lines 6662 6662
=======================================
Hits 5307 5307
Misses 1355 1355 |
get_parser=self._resistance_parser, | ||
label='Resistance', | ||
unit='Ohm') | ||
|
||
def _get_volt_and_curr(self) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the function called “volt and curr “ while READ returns 4 values? Should the name of this function be more generic?
This does not only give out volt and curr
There is something unusual in the implementation of this visa instrument not specific to QCoDeS or pyvisa. If you do a visa read command with no preceding write command you get the result of The error is as expected that the command is not supported with output off but it still locks up the system. (The error can be seen after manually disconnecting from the instrument front panel and reconnecting ) |
if output == 1: | ||
msg = self.ask(':READ?') | ||
else: | ||
msg = "nan,nan,nan,nan,nan" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not raise an exception here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I thought about that. My main motivation for not doing that was to prevent adding it to the station from triggering an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My worry is that if you add it to a station and do some measurements you might miss the fact that it's not actually recording any data. I don't think there's a perfect solution, though I would prefer an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jenshnielsen, sorry, I did not understand what you meant. Despite that I tend to agree with @spxtr.
Do you think it is enough to add a note to the driver docstring (or docstring of each affected parameter) that "output" should be on before reading values, otherwise an exception will be thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be more clear. I think it's fine for the get to throw when called directly, and yes documenting is a good idea.
I was more concerned with the automatic get that happens at setup time but the snapshot will catch that exception anyway so either way should be fine.
bab8178
to
871057c
Compare
nice! if everyone agrees now, please merge @jenshnielsen |
Merge: d2abd73 b26bebe Author: Jens Hedegaard Nielsen <[email protected]> Merge pull request #1171 from jenshnielsen/keithley_2400_protect_read
In the Keithley 2400 doing a read with output disabled is an error that leaves the instrument in a state where no commands can be sent to it without restart
To overcome this we check that output is on before doing a read. If not we return 'nan' as a result
Fixes #1149