Magnet driver #660
Magnet driver #660
Conversation
|
Sorry this lingered so long. I left a few comments inline but otherwise this looks good to me. I don't have a IPS120 to test on so this review is only based on reading the code |
| # This program is free software; you can redistribute it and/or modify | ||
| # it under the terms of the GNU General Public License as published by | ||
| # the Free Software Foundation; either version 2 of the License, or | ||
| # (at your option) any later version. |
There was a problem hiding this comment.
Is there a particular reason for GPL licence. QCoDeS is licensed under the MIT licence and we cannot directly include code that is GPL licensed in QCoDeS as far as I understand it.
There was a problem hiding this comment.
@jenshnielsen Would it be an option to include the driver in qcodes with an explicit mention of the GPL in the driver directory? We have another driver that is GPL, and I do not think it is possible to relicense to MIT.
There was a problem hiding this comment.
I will ask around about the policy on this and get back to you
There was a problem hiding this comment.
@jenshnielsen @eendebakpt After searching for 'GNU' in the repository I saw that there already are some instrument drivers in qcodes which are GPL licensed. I actually think that some other instrument drivers which don't have a GPL license statement in the source code are also ported from qtlab instrument drivers, which had a GPL license statement in the source code. Probably the original authors won't bother anymore, but a general guideline on how to work with GPL licensed instrument drivers in the qcodes framework would be good.
| # if mes[1] != 0: | ||
| # # see protocol descriptor for error codes | ||
| # raise Exception('IVVI rack exception "%s"' % mes[1]) | ||
| return mes |
There was a problem hiding this comment.
should the commented out code stay?
| 2: "Off magnet at field (switch closed)", | ||
| 5: "Heater fault (heater is on but current is low)", | ||
| 8: "No switch fitted"} | ||
| print(status[int(result[8])]) |
There was a problem hiding this comment.
Do we need this print statement
| "Outside positive current limit" | ||
| """ | ||
| result = self._execute('X') | ||
| logging.info(__name__ + ' : Getting system status') |
There was a problem hiding this comment.
It might be worth replacing the logger with a pattern like
log = logging.getLogger(__name__)
once and then after that
log.warning('this is a warning`)
such as in
https://github.com/QCoDeS/Qcodes/blob/master/qcodes/instrument_drivers/tektronix/AWG5014.py#L15
|
@jenshnielsen I made adjustments based on your suggestions. We have been in touch with the authors of the code this driver is based and have received permission to change the license to MIT. |
Author: CJvanDiepen <CJvanDiepen@users.noreply.github.com>
Magnet driver (#660)
Add instrument driver for magnet IPS120.
Changes proposed in this pull request:
@jenshnielsen