Fix: Keithley 2000 modes #334
Conversation
| @@ -142,25 +164,32 @@ def __init__(self, name, address, reset=False, **kwargs): | |||
| self.connect_message() | |||
|
|
|||
| def trigger(self): | |||
| if self.trigger_continuous() == 'off': | |||
| if self.trigger_continuous(): | |||
There was a problem hiding this comment.
should be
if not self.trigger_continuous()
There was a problem hiding this comment.
Yep I forgot to add that, also the other one
| self.write('INIT') | ||
| self._trigger_sent = True | ||
|
|
||
| def _read_next_value(self): | ||
| # Prevent a timeout when no trigger has been sent | ||
| if self.trigger_continuous() == 'off' and not self._trigger_sent: | ||
| if self.trigger_continuous() and not self._trigger_sent: |
There was a problem hiding this comment.
should be
if not self.trigger_continuous() and not self._trigger_sent
|
@alexcjohnson @giulioungaretti I think this can be merged? |
| super().__init__(name, address, **kwargs) | ||
|
|
||
| self._trigger_sent = False | ||
|
|
||
| # Unfortunately the strings have to contain quotation marks and a | ||
| # newline character, as this is how the instrument returns it. |
There was a problem hiding this comment.
Pass the terminator='\n' along to super().__init__ (in fact, perhaps only there, there's probably no reason to put it in the K2000 constructor is there?) and pyvisa will take out the newlines for you.
There was a problem hiding this comment.
Maybe this is expected behavior, but when using a val_mapping the newline doesn't get chopped off. As evidenced by the usage in the constructor and the fact that the current _mode_map works.
There was a problem hiding this comment.
it's in the signature of this constructor, but you don't do anything with it after that. But if you instead put it in the super().__init__ call, it really should take the newlines out.
There was a problem hiding this comment.
See for example the K2600 driver
There was a problem hiding this comment.
Ah I see... didn't think of adding it to the super() call.
|
|
||
| return parser(self.ask(cmd)) | ||
|
|
||
| def _set_mode_param(self, parameter, value): | ||
| cmd = '{}:{} {}'.format(self.mode(), parameter, value) | ||
| """ Read the current Keithley mode and set a parameter """ | ||
| if type(value) is bool: |
There was a problem hiding this comment.
not blocking, but it's usually preferable to use isinstance(value, bool)
|
Looks good to me - my comments are nonblocking bits of cleanup. Thanks for the pep🎱 fixes! Looks like @damazter needs to approve his review with your changes before you can merge, but it's a💃 from my standpoint. |
|
@alexcjohnson Apparently I am not authorized to merge this PR, did something change recently? |
|
@Rubenknex I'm not entirely sure what restrictions @giulioungaretti put in place, but now that tests pass (the failure before was stochastic unrelated to your changes) are you allowed to merge? |
|
@alexcjohnson Nope, it refers me to this page https://help.github.com/articles/about-protected-branches/ |
|
Alright, merged. That article implies that since tests and reviews all passed you should have been able to merge too... dunno. |
Fixes #333 by @damazter
Changes proposed in this pull request:
@alexcjohnson @giulioungaretti