Refactor Keysight 344xxA code and small fixes #1169
Conversation
| """ | ||
| Instrument class for Keysight 34460A, 34461A, 34465A and 34470A multimeters. | ||
|
|
||
| Tested with: 34461A, 34465A. |
There was a problem hiding this comment.
is it possible to test the driver on 34460A and 34470A as well?
There was a problem hiding this comment.
No, I have only a 34461A. The 34465A was tested by the original author.
Anyway they should work, they are very similar
|
|
||
| # SYSTEM | ||
| self.add_parameter('error', | ||
| label='Error message', |
There was a problem hiding this comment.
according to errorparser implementation, this "Parameter" returns not only the message, but also the error code. In order to avoid confusion with the usage of this Parameter, and to avoid abuse of Parameter class as such, could you please make this a method of this class (basically making errorparser a method)?
| @@ -186,6 +170,12 @@ def __init__(self, name, address, utility_freq=50, silent=False, | |||
| #################################### | |||
| # PARAMETERS | |||
|
|
|||
| self.add_parameter('line_frequency', | |||
There was a problem hiding this comment.
should the values of this parameter be limited to vals=vals.Enum(50, 60)?
| #################################### | ||
| # PARAMETERS | ||
|
|
||
| self.add_parameter('line_frequency', |
There was a problem hiding this comment.
Shall the values of this parameter be limited to vals=vals.Enum(50, 60)?
There was a problem hiding this comment.
I don't think, it has only a get, you can't set it. Do we need validator in this case?
There was a problem hiding this comment.
oh, no, but then please add set_cmd=False to the add_parameter call - this way you show that setting is indeed not possible. You could also add a dosctring= argument where you explain that "the instrument returns the frequency of the grid that it is plugged into" or smth.
There was a problem hiding this comment.
btw, sorry for misinformation, set_cmd is False by default, so no need for explicitly adding it. But thanks for adding it anyway.
There was a problem hiding this comment.
But the line frequency is now needed in the Keysight_34465A.yaml file.
There was a problem hiding this comment.
True, I added it.
There was a problem hiding this comment.
Something else for merging?
Codecov Report
@@ Coverage Diff @@
## master #1169 +/- ##
=========================================
+ Coverage 79.78% 79.9% +0.11%
=========================================
Files 48 49 +1
Lines 6664 6673 +9
=========================================
+ Hits 5317 5332 +15
+ Misses 1347 1341 -6 |
|
@YakBizzarro, nothing more is needed, I think we're good to merge. I'll just wait for CI. |
|
Great, thank you! |
Fix code for Keysight 34461A and split the driver depending on the model, like in #772
Also it automatically detect if the digitizer option is installed.
@WilliamHPNielsen @jenshnielsen