pyvisa / pyvisa-py

A pure python PyVISA backend
https://pyvisa-py.readthedocs.io
MIT License
282 stars 120 forks source link

addition of HiSLIP specification V1.1 (current version is 2.0) #331

Closed bobmcnamara closed 1 year ago

bobmcnamara commented 2 years ago
codecov-commenter commented 2 years ago

Codecov Report

Merging #331 (3348c8f) into main (76bda99) will increase coverage by 0.31%. The diff coverage is 25.98%.

@@            Coverage Diff             @@
##             main     #331      +/-   ##
==========================================
+ Coverage   21.73%   22.05%   +0.31%     
==========================================
  Files          20       20              
  Lines        2682     3111     +429     
  Branches      415      399      -16     
==========================================
+ Hits          583      686     +103     
- Misses       2082     2409     +327     
+ Partials       17       16       -1     
Flag Coverage Δ
unittests 22.05% <25.98%> (+0.35%) :arrow_up:

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

Impacted Files Coverage Δ
pyvisa_py/highlevel.py 45.08% <0.00%> (+1.15%) :arrow_up:
pyvisa_py/protocols/rpc.py 22.02% <0.00%> (-0.04%) :arrow_down:
pyvisa_py/tcpip.py 18.70% <23.07%> (+0.89%) :arrow_up:
pyvisa_py/protocols/hislip.py 27.33% <27.33%> (ø)
pyvisa_py/sessions.py 40.50% <100.00%> (ø)
pyvisa_py/__init__.py

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

MatthieuDartiailh commented 2 years ago

Could you make a separate PR for VICP ? I will review this PR before the end of the week but I would prefer to focus on a single thing at a time.

MatthieuDartiailh commented 2 years ago

Also what is the empty setup file ?

bobmcnamara commented 2 years ago

Also what is the empty setup file ?

The contents of setup.py shouldn't have changed...I only changed the mode to make it executable.

MatthieuDartiailh commented 2 years ago

Out of curiosity why did you need to make setup.py executable in the first place ? With PEP517 one should always install through pip.

MatthieuDartiailh commented 2 years ago

@bobmcnamara please ping me when this is ready for a new review.

bobmcnamara commented 2 years ago

Out of curiosity why did you need to make setup.py executable in the first place ? With PEP517 one should always install through pip.

Good question. I was working on releasing the pyvicp package to PyPI (my first!) and I wasn't really understanding the process. I started experimenting with the pyvisa-py package and must have made setup.py executable after I got tired of typing "python3 setup.py". But I can't even recall why I was doing that in the first place.

bobmcnamara commented 2 years ago

@bobmcnamara please ping me when this is ready for a new review.

@MatthieuDartiailh when you get a chance, please take another look.

bobmcnamara commented 2 years ago

@MatthieuDartiailh I think I've made all the changes you requested. Once I add support for the mandatory attributes (which I could use your help with, BTW), I think it will be ready to release.

MatthieuDartiailh commented 2 years ago

Thanks @bobmcnamara

I added support for some attributes (please check the hislip values, I tried to get them from your code but I may have misinterpreted things). I hardcoded the hislip version which does not really make sense but I have no idea how to query it from the instrument. I noticed that you establish the communication with version 1.0. I wrote 1.1 because of the title of your PR. Should we change it to 1.0 ?

I also tried to test your implementation against my fake Keysight instrument with which I can communicate over hislip using Keysight VISA and which report using the hislip protocol at version 1.0 (in hex 0x100000) and I got this error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa\pyvisa\highlevel.py", line 3284, in open_resource
    res.open(access_mode, open_timeout)
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa\pyvisa\resources\resource.py", line 278, in open
    self.session, status = self._resource_manager.open_bare_resource(
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa\pyvisa\highlevel.py", line 3209, in open_bare_resource
    return self.visalib.open(self.session, resource_name, access_mode, open_timeout)
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa-py\pyvisa_py\highlevel.py", line 167, in open
    sess = cls(session, resource_name, parsed, open_timeout)
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa-py\pyvisa_py\tcpip.py", line 71, in __new__
    return newcls(resource_manager_session, resource_name, parsed, open_timeout)
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa-py\pyvisa_py\sessions.py", line 325, in __init__
    self.after_parsing()
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa-py\pyvisa_py\tcpip.py", line 99, in after_parsing
    self.interface = hislip.Instrument(
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa-py\pyvisa_py\protocols\hislip.py", line 384, in __init__
    init = self.initialize()
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa-py\pyvisa_py\protocols\hislip.py", line 570, in initialize
    return InitializeResponse(self._sync)
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa-py\pyvisa_py\protocols\hislip.py", line 249, in __init__
    super().__init__(sock, "InitializeResponse")
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa-py\pyvisa_py\protocols\hislip.py", line 207, in __init__
    self.header = receive_exact(sock, HEADER_SIZE)
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa-py\pyvisa_py\protocols\hislip.py", line 148, in receive_exact
    receive_exact_into(sock, recv_buffer)
  File "c:\users\matthieu.dartiailh\documents\coding\projects\pyvisa-py\pyvisa_py\protocols\hislip.py", line 164, in receive_exact_into
    data_len = sock.recv_into(view, request_size)
BlockingIOError: [WinError 10035] A non-blocking socket operation could not be completed immediately
bobmcnamara commented 2 years ago

There’s a subtle error in my title…it should say “HiSLIP spec V1.1”. For spec V1.1, the protocol version is specified as 1.0:

Protocol Version

The version of the protocol defined in this document is 1.0. In general the protocol version is not the same as the specification version.

So yes, it should be changed to 1.0.

Sorry, I hadn’t tried the connection without specifying an open_timeout value. In the case you tried, the open_timeout value being passed in is 0, which causes the failure.

I guess I should change this line:

open_timeout = open_timeout if open_timeout is not None else 1000

to this:

open_timeout = open_timeout or 1000

Or 2000? Or 5000?

MatthieuDartiailh commented 2 years ago

Thanks for clarifying. Did you check the other values I set ?

I will try playing with the open timeout and see to pipe a reasonable default.

Finally since the PR allowing to list VXI11 resources is in, I am wondering if we could provide an equivalent for hislip. Is it possible ?

bobmcnamara commented 2 years ago

What I found is in this spec: https://www.lxistandard.org/members/Adopted%20Specifications/Latest%20Version%20of%20Standards_/LXI%20Version%201.6/LXI_HiSLIP_Extended_Function_1.3_2022-05-26.pdf

It says "Devices implementing the LXI HiSLIP Function shall advertise that they accept HiSLIP connections via the HiSLIP DNS-SD service announcement", and "HiSLIP DNS-SD service announcements shall use the mDNS service type name ‘_hislip._tcp’".

Using the command "dns-sd -B _hislip._tcp" in one of the labs identified dozens of instruments. So it is possible.

There is a Python package (https://pypi.org/project/zeroconf/) that has already done all the heavy lifting to make this work. Running the script below also identified the instruments:

################################################# import time from zeroconf import ServiceBrowser, ServiceListener, Zeroconf

class MyListener(ServiceListener):

def update_service(self, zc: Zeroconf, type_: str, name: str) -> None:
    print(f"Service {name} updated")

def remove_service(self, zc: Zeroconf, type_: str, name: str) -> None:
    print(f"Service {name} removed")

def add_service(self, zc: Zeroconf, type_: str, name: str) -> None:
    info = zc.get_service_info(type_, name)
    print(f"Service {name} added, service info: {info}")

zeroconf = Zeroconf() listener = MyListener() browser = ServiceBrowser(zeroconf, "_hislip._tcp.local.", listener)

time.sleep(1) zeroconf.close() #################################################

Unfortunately that package has the LGPL license. Will that be okay, since it only has to be imported?

MatthieuDartiailh commented 2 years ago

The open_timeout is somewhat cryptic in the VISA spec. It actually only applies to the time given to the instrument to release a VISA lock not to the time after which to abort the connection attempt (section 4.3.3.2). I have had users confused by this previously. And it looks like the VXI-11 implementation does a poor job of respecting this too.

I think the usual value in other implementation is 5000 ms.

For the scanning dependency, I am always confused by Licensing, one easy way around it would be to make it an optional dependency like we did with psutil for scanning VXI-11 resources. However we should use a more explicit name for the option: hislip-discovery maybe

MatthieuDartiailh commented 2 years ago

I also confirmed that I can open a hislip resource and run a simple query using your code.

bobmcnamara commented 2 years ago

In pyvisa_py.highlevel.open(), the access_mode is ignored. Since the open_timeout only applies to locks, it should be ignored as well:

If the accessMode parameter requests a lock, then this parameter specifies the absolute time period (in milliseconds) that the resource waits to get unlocked before this operation returns an error.

MatthieuDartiailh commented 1 year ago

Looks great. But it seems you need to run black because I messed up my suggestions. Sorry for that.

bobmcnamara commented 1 year ago

It was a mypy complaint, not a black complaint. Seems to be fixed now.

MatthieuDartiailh commented 1 year ago

Great ! Thanks for your awesome work !!!

MatthieuDartiailh commented 1 year ago

If you can rebase your VICP PR I will do my best to review it shortly