Various IPS120 driver improvements #1402
Conversation
spxtr
force-pushed
the
ips120gpib
branch
2 times, most recently
from
6363043 to
0b13aa9
Compare
Codecov Report
@@ Coverage Diff @@
## master #1402 +/- ##
==========================================
- Coverage 73.88% 73.86% -0.03%
==========================================
Files 92 92
Lines 10420 10411 -9
==========================================
- Hits 7699 7690 -9
Misses 2721 2721 |
There was a problem hiding this comment.
This looks good.
But I am tempting to ask you to go through the whole driver, and perform the same refactoring for other parameters: extracting the number<->string dictionaries out of the methods into class attributes as you did for _STATUS_MODE2 and _STATUS_SWITCH_HEATER :)
There was a problem hiding this comment.
Agreed with @astafan8, It would be great if you have the time to perform all these conversions. I suggested inline to use self.log I realize that the driver does not do that in general. We are in the middle of converting the drivers to that pattern to make it simpler to get log messages from a specific driver only. It would be great if you are willing to do this conversion over the driver.
Otherwise this looks great
| mode, | ||
| "Unknown")) | ||
| if mode in status: | ||
| log.info('Setting remote control status to %s' % status[mode]) |
There was a problem hiding this comment.
I think this should be self.log See https://qcodes.github.io/Qcodes/examples/Creating%20Instrument%20Drivers.html#Logging
|
OK, I made the driver use I have tested something very similar to this on a real machine, but not this exact change, so please let me test it again before merging. |
|
It looks like there are a few small syntax errors. Other than than I think this is ready to merge once you confirm that it has been tested on a real device |
Also bring the driver up to date in terms of style.
|
@spxtr Did you ever test this on a real device |
|
We've warmed up the system for maintenance, once we cool it back down I'll test this. Should be in a week or two. |
| 0: "Amps, Magnet sweep: fast", | ||
| 1: "Tesla, Magnet sweep: fast", | ||
| 4: "Amps, Magnet sweep: slow", | ||
| 5: "Tesla, Magnet sweep: slow"} |
There was a problem hiding this comment.
isn't this dict missing 8 and 9? see docstring here https://github.com/QCoDeS/Qcodes/pull/1402/files#diff-05c52e149e6fdc1a4a23377634638f2cR862
|
Yes, I tested this on a real machine. At least the basic functions like manipulating the switch heater, running to fields, and entering/leaving persistent mode worked. |
Codecov Report
@@ Coverage Diff @@
## master #1402 +/- ##
=======================================
Coverage 73.88% 73.88%
=======================================
Files 92 92
Lines 10420 10420
=======================================
Hits 7699 7699
Misses 2721 2721 |
Merge: 18054f5 36f4d99 Author: Mikhail Astafev <[email protected]> Merge pull request #1402 from spxtr/ips120gpib
First I make it support both GPIB and serial. Next, fix some bugs in the driver. I haven't yet tested this on my own machine, will do so soon.
@jenshnielsen