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

Small improvements to visa backend #4219

Merged
merged 8 commits into from
Jun 2, 2022
Merged

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Jun 2, 2022

  • Slightly better docs and logging
  • Correct the backend name to reflect current pyvisa
  • Remove deprecated fallback for visa address with library included
  • Let pyvisa figure out the default termination char

@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #4219 (454e53f) into master (b302431) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4219      +/-   ##
==========================================
- Coverage   67.02%   66.99%   -0.04%     
==========================================
  Files         233      233              
  Lines       31525    31517       -8     
==========================================
- Hits        21130    21114      -16     
- Misses      10395    10403       +8     

By default. This should not change any thing for most instruments
but will set the default terminator to `\r` for rs232 instruments.
@jenshnielsen
Copy link
Collaborator Author

@astafan8 Could you have another look. I made a change to not set the visa termination char unless explicitly given to match pyvisa here https://pyvisa.readthedocs.io/en/1.8/resources.html#termination-characters

@astafan8
Copy link
Contributor

astafan8 commented Jun 2, 2022

I made a change to not set the visa termination char unless explicitly given to match pyvisa here https://pyvisa.readthedocs.io/en/1.8/resources.html#termination-characters

seems like the right thing to do.

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.

breaking changes notes are added, so i think it's good to go

Get value of terminator from visa_handle.read_termination
Its not clear if this should be read or write and really we should
log both so extend snapshot with explicit read and write terminators
@jenshnielsen
Copy link
Collaborator Author

bors merge

@bors bors bot merged commit e4d2f9e into microsoft:master Jun 2, 2022
@jenshnielsen jenshnielsen deleted the visa_fixes branch June 2, 2022 13:57
@gztony1227
Copy link

gztony1227 commented Jun 2, 2022

@jenshnielsen @astafan8 Thank you both for incorporating this fix.
How do you pass down terminator when defining the tek 2012B?

I made terminator as a default argument in my local 2012 driver. kwargs won't be able to pass it down to visa.py right? I don't see visa.py looking for kwargs to determine terminator.

So far I like qcodes being installed and updated on pip. I haven't used it like a repository. So it will take a while for the new version to be pip?

def __init__(self, name: str, address: str,
                 timeout: float = 20, terminator: str = '', **kwargs: Any):
        """
        Initialises the TPS2012.

        Args:
            name: Name of the instrument used by QCoDeS
            address: Instrument address as used by VISA
            timeout: visa timeout, in secs. long default (180)
              to accommodate large waveforms
        """

        super().__init__(name, address, timeout=timeout, terminator = terminator, **kwargs) 

@jenshnielsen
Copy link
Collaborator Author

jenshnielsen commented Jun 2, 2022

It would be nice if you can test the changes before we release a new version so we are sure it works.
You can install from the master branch by doing pip install git+https://github.com/QCoDeS/Qcodes.git
Once you are done testing you can then do pip uninstall qcodes and pip install qcodes to get back to the release version.
But if you have the time I would recommend lerning a bit of git to be able to install from a cloned repo

The value will be passed to the instrument with the current __init__ class

If you do TPS2012("name", "address", terminator="\n") terminator will be part of the kwargs dict to the base visa class.
Here it will be avilable via the terminator argument whic is passed to set_terminator. There is no need to explicitly add a terminator argument to TPS2012 unless we need to change the default.

What I would like you to do is.

  • install qcodes from source as above
  • test if scope = TPS2012('tek', 'ASRL5::INSTR') works
  • If that does not work test that scope = TPS2012('tek', 'ASRL5::INSTR', terminator='\n') works

I suspect that number 2 will work since that should be equivalet to what pyvisa does by default

@gztony1227
Copy link

Dear Jens @jenshnielsen and Mikhail @astafan8 ,
The version worked well. I used a new environment on a separate PC to clone tttps://github.com/QCoDeS/Qcodes.git with pycharm and tps2012B is connected well.
scope = TPS2012('tek', 'ASRL5::INSTR') works

TPS2012('tek', 'ASRL5::INSTR', terminator='\n') also works.

The line "terminator = None" as default is good.
Please go ahead and release the version when you can. Thank you both very much.

@jenshnielsen
Copy link
Collaborator Author

Thanks @gztony1227 i will go ahead and close the issue then

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