Update CryogenicSMS driver #1346
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove typos, hardcode terminator, properly raise exceptions when attempting to measure the field in 'AMPS' mode. The commit also makes the driver compatible with SMS60C by removing an incorrect value at the start of some replies.
Codecov Report
@@ Coverage Diff @@
## master #1346 +/- ##
=======================================
Coverage 73.34% 73.34%
=======================================
Files 79 79
Lines 9098 9098
=======================================
Hits 6673 6673
Misses 2425 2425 |
jenshnielsen
requested changes
Oct 29, 2018
|
|
||
| This magnet PS driver has been tested with: | ||
| FTDI chip drivers (USB to serial), D2XX version installed. | ||
| Cryogenic SMS120C and SMS60C (though the default init arguments are not correct for the latter) |
There was a problem hiding this comment.
Could you document what needs to change?
|
|
||
| log.debug('Initializing instrument') | ||
| super().__init__(name, address, terminator=terminator, **kwargs) | ||
| super().__init__(name, address, terminator='\r\n', **kwargs) |
There was a problem hiding this comment.
This is slightly dangerous. Even if you were passing the default value this will now result in a TypeError: __init__() got multiple values for keyword argument 'terminator'
I would add some lines like the following to the start of the function above
if 'terminator' in kwargs.keys():
kwargs.pop('terminator')
warnings.warn('Passing terminator to CryogenicSMS is no longer supported and has no effect')
There was a problem hiding this comment.
That's a great way of retaining backwards compatibility (when named arguments are used)!
Co-Authored-By: lukabavdaz <[email protected]>
Also makes terminator more backwards compatible (when named arguments are used).
jenshnielsen
approved these changes
Oct 30, 2018
giulioungaretti
pushed a commit
that referenced
this pull request
Nov 2, 2018
Merge: 94acf12 a0185fd Author: Jens Hedegaard Nielsen <[email protected]> Merge pull request #1346 from lukabavdaz/feature/CryogenicSMS-update
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes proposed in this pull request:
@jenshnielsen