Skip to content
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

Keithley 6500 parameters had invalid SCPI commands #1541

Merged
merged 6 commits into from
Apr 18, 2019
Merged

Keithley 6500 parameters had invalid SCPI commands #1541

merged 6 commits into from
Apr 18, 2019

Conversation

qSaevar
Copy link
Contributor

@qSaevar qSaevar commented Apr 16, 2019

Changes proposed in this pull request:
In Keithley_6500 driver fix parameters:

  • trigger_source
  • trigger_delay
  • display_enable

@astafan8

@codecov
Copy link

codecov bot commented Apr 16, 2019

Codecov Report

Merging #1541 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1541   +/-   ##
=======================================
  Coverage   71.23%   71.23%           
=======================================
  Files         103      103           
  Lines       11994    11994           
=======================================
  Hits         8544     8544           
  Misses       3450     3450

@jenshnielsen
Copy link
Collaborator

It looks like the dmm supports compatibility mode with old dmms. I suspect the original driver was written to support this

@qSaevar
Copy link
Contributor Author

qSaevar commented Apr 16, 2019

@jenshnielsen It is stated in the class docstring that this is based on the keithley 2000 (where the SCPI commands are valid) and that this is a bet-version. Don't know if there has been a 6500 in the past where the three parameters that I changed worked. My guess is not.

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK (module Jens' comment). Left some minor suggestions.

@jenshnielsen
Copy link
Collaborator

@qSaevar Yes my point is that the 6500 can be put into a 2000 compatibility mode. I think that could be what this driver is targeting. See #1533 for something similar example for the the 2450

@qSaevar
Copy link
Contributor Author

qSaevar commented Apr 17, 2019

@jenshnielsen Ok, I see you point. So, solutions: Flag in the constructor for compatibility mode set by the user? Second driver for the full feature support? Some auto detection for the mode the device is in?

@jenshnielsen
Copy link
Collaborator

@qSaevar Yes I think we should test which mode it's in and error appropriately if it's in the wrong mode. I support using the default mode and not any compatibility mode

@qSaevar
Copy link
Contributor Author

qSaevar commented Apr 17, 2019

@jenshnielsen ok, so I'll add something like:
if language not SCPI:
raise MeaningfulError
At the end of the constructor.

@qSaevar
Copy link
Contributor Author

qSaevar commented Apr 17, 2019

I did add a check for the correct command set.

@qSaevar
Copy link
Contributor Author

qSaevar commented Apr 18, 2019

@astafan8 I addressed your comments, @jenshnielsen concerns and updated the branch. Is there anything else that needs to be done?

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good now module one comment. After it is fixed, i'll merge.

@astafan8 astafan8 merged commit cfa2ff3 into microsoft:master Apr 18, 2019
@qSaevar qSaevar deleted the bug/DEM-707/Issues-with-KeithleyDMM6500-parameters branch April 18, 2019 11:29
giulioungaretti pushed a commit that referenced this pull request Apr 18, 2019
Merge: 6b8f4d1 6058273
Author: Mikhail Astafev <[email protected]>

    Merge pull request #1541 from qutech-sd/bug/DEM-707/Issues-with-KeithleyDMM6500-parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants