Add support for Hewlett Packard 8753E VNA

[Sionwage]

I would like to add a basic HP8753E VNA support to pymeasure.

The functionality is not ready but I would like feedback as I go and there are some specifics for coding I would like assistance with finding documentation on how to handle these.

1. How do I handle default Prologix GPIB-USB adapter parameters to work with this device?

   I found these pages that address how to handle read/write terminations and other parameters for the various adapters but I am unsure how to apply this to the prologix gpib-usb adapter I utilize.

   • This issue states I shouldn't set specific adapter settings to make the instrument work:
     • https://github.com/pymeasure/pymeasure/issues/815
   • These pages makes it look like I should be configuring adapter settings:
     • https://pymeasure.readthedocs.io/en/latest/dev/adding_instruments/instrument.html#defining-default-connection-settings
     • https://pyvisa.readthedocs.io/en/latest/api/constants.html#pyvisa.constants.InterfaceType

   I am not sure if I should be using asrl or gpib for pyvisa.constants.InterfaceType since its a serial device connecting to GPIB.

   Unfortunately, it seems like if I am not defining the Prologix settings and commanding them when using the VNA, there will be unexpected behavior or it will timeout/error out.

   It looks like it is addressed here but not implemented here https://github.com/pyvisa/pyvisa/issues/112

2. If I utilize default Prologix GPIB-USB adapter parameters for one instrument, is the functionality to change these parameters on the fly for other instruments in place? IE, if I need '++auto 1' for one instrument but not another, with pymeasure send the command to the adapter if I communicate to alternating instruments and remember when it needs to change it?

   The #112 issue makes it look like different parameters needed with the Prologix adapter for multiple instruments are not handled yet with pymeasure or pyvisa.

3. Are we allowed to utilize cached properties to save time on communicating?

My to-do list of actions still needed to polish this for others to use is:

• [ ] Need unit tests for this class that don't rely on having the instrument. Currently my unit tests rely on having the instrument on the bench and connected. I know there is some functionality at Adding Instruments->Writing tests->Protocol tests->Test generator to create tests that don't demand the instrument be attached to the computer.
• [x] Remove adapter specific commands like my workarounds using the Prologix adapter from the class or be able to toggle the Prologix specific functions on/off when initializing the class.
• [x] Write comments for each of the functions clarifying them
• [x] Start the comments for properties with ['Get', 'Measure', 'Set', or 'Control']
• [x] May need to move the __init__ block to __new__?
• [ ] Exceptions may need to use more specific exceptions from the pymeasure library
• [x] Probably need to remove the __version and if __name__=="__main__": blocks
• [x] Address other code style issues I am undoubtedly not adhering to.

[Sionwage]

Is there a way to change the branch to merge from? I have somehow completely hosed my git clone and cannot update/sync my copied repo with the changes made in pymeasure:master since opening this.

Otherwise I'll just copy out my code, nuke my fork repo, refork the repo and create a branch before putting my code back in so I can sync upstream changes.

[BenediktBurger]

You cannot change the branch name to be merged from.

You can, however use different commits in the branch:

1. create a temporary branch, where you are at right now (and maybe the files)
2. delete your current branch
3. create a new branch with the name of the current branch (for example on the current master)
4. Do the changes you want (e.g. copying the files)
5. Push your changes to your github repo (using push force)

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 61.78344% with 60 lines in your changes missing coverage. Please review.

Project coverage is 58.51%. Comparing base (4846672) to head (99b724d).

Files	Patch %	Lines
pymeasure/instruments/hp/hp8753e.py	61.53%	60 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
+ Coverage   58.48%   58.51%   +0.02%     
==========================================
  Files         262      263       +1     
  Lines       18198    18355     +157     
==========================================
+ Hits        10643    10740      +97     
- Misses       7555     7615      +60     

Flag	Coverage Δ
unittests	58.51% <61.78%> (+0.02%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Sionwage]

As I see it, I think there are two/three items still needing to get addressed before you would accept this pull request.

• PrologixAdapter fixture for unit-testing the HP8753E needs to be resolved
• May need more unit tests to cover the 40% or so of instrument code that isn't in a unit-test.
• May need to rewrite the measurement_parameter property to use Instrument.control

[BenediktBurger]

May need more unit tests to cover the 40% or so of instrument code that isn't in a unit-test.

Don't worry about less than 100% patch coverage. Tests are good, but we do not have any limit for merging pull requests.

[Sionwage]

I think I am close to done with all the requests made and just need answers to the last 3 unresolved conversations to finish.

[BenediktBurger]

Hey @Sionwage, I'm in the process of reviewing, but I'll need some time, due to time constraints. I ask for your patience.

[Sionwage]

Thanks for your contribution.

I mentioned some incosistencies and mainly documentation issues. The instrument looks already good.

I noticed quite a lot dynamic properties. Do you intend to use them as dynamic properties or is it "just in case"?

There are different configurations for the 8753E so this allows others to override the defaults set for their instrument.

[Sionwage]

Not sure how to resolve WARNING: Field list ends without a blank line; unexpected unindent.

Otherwise I think everything has been addressed now.

[BenediktBurger]

Thank you for contributing!

[Sionwage]

Thank you for contributing!

Thank you for your patience!