Conversation
damazter
left a comment
damazter
left a comment
There was a problem hiding this comment.
We had to make two changes to make the code work,
I think those are two typos
|
|
||
| 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
| 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? |
| 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.
| 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? |
|
Alright, merged. That article implies that since tests and reviews all passed you should have been able to merge too... dunno. |
3 participants
Fixes #333 by @damazter
Changes proposed in this pull request:
@alexcjohnson @giulioungaretti