-
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
fix option detection on Keysight N51x1 driver #4342
fix option detection on Keysight N51x1 driver #4342
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4342 +/- ##
==========================================
- Coverage 68.37% 68.36% -0.02%
==========================================
Files 267 267
Lines 30961 30968 +7
==========================================
Hits 21170 21170
- Misses 9791 9798 +7 |
@simonzihlmann Thanks, could you add a news fragment as described in the pr template? |
for more information, see https://pre-commit.ci
bors merge |
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
@@ -13,6 +13,27 @@ class N51x1(VisaInstrument): | |||
def __init__(self, name: str, address: str, min_power: int = -144, max_power: int = 19, **kwargs: Any): | |||
super().__init__(name, address, terminator='\n', **kwargs) | |||
|
|||
self._options = self.ask("*OPT?") |
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.
the comment in the old code makes me think that the instrument response is a comma-separated list of numbers, right? if so, then it would've been better to split the returned string into an actualy list of strings with numbers. the if f_option in self._options
check would still work, but would be more robust, so that e.g. option 50101
does not get mistakenly treated as option 501
(but of course likely option 50101
does not exist, so this all is likely not a problem in real life)
self._options = self.ask("*OPT?").split(",")
make frequency option detection compatible with multiple options installed on device. The old version of the driver resulted in a
KeyError
if multiple options were installed on the device.I was unable to figure out from the manual / spec sheet if the frequency option will always be the first one in the string returned by the instrument. For the moment, all other options are ignored.