tcpip: List resources to fix #165

ekigwana commented 3 years ago

Got the snippet from https://github.com/python-ivi/python-vxi11/blob/master/vxi11/vxi11.py#L513

Signed-off-by: Edward Kigwana ekigwana@gmail.com

[x] Closes #165
[x] Executed black . && isort -c . && flake8 with no errors
[x] The change is fully covered by automated unit tests
[x] Documented in docs/ as appropriate
[x] Added an entry to the CHANGES file

MatthieuDartiailh commented 3 years ago

Thanks for your contributions. Did you get a chance to test it?

I hope to bring the keysight build bot back online in the next two weeks. I will likely wait for it before merging your work. Do not hesitate to ping me if I were to fall behind schedule.

ekigwana commented 3 years ago

Yes, I tested it all using the PyVISA frontend and it works. I don't have any tcpip bridge adapters to old equipment so I could not code for that. The snippent I references addresses that by attempting to open connections. This solves a major irritation when you multiple calibration stations with the same equipment on a dhcp network and folks have to run around to figure out what the new address is. Now they only have to get the equipment serial information, that + IDN and voila no need to know IP address.

MatthieuDartiailh commented 3 years ago

Thanks. I am slowly bringing the keysight based bot back online so I will try to add test for this PR and then give it a run. Do not hesitate to ping me if I take too long.

MatthieuDartiailh commented 3 years ago

bors try

codecov-io commented 3 years ago

Codecov Report

Attention: Patch coverage is 18.75000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 21.87%. Comparing base (40d4396) to head (15e2a33). Report is 235 commits behind head on main.

Files with missing lines	Patch %	Lines
pyvisa_py/tcpip.py	0.00%	10 Missing :warning:
...e/keysight_assisted_tests/test_resource_manager.py	50.00%	2 Missing and 1 partial :warning:

:exclamation: There is a different number of reports uploaded between BASE (40d4396) and HEAD (15e2a33). Click for more details.

HEAD has 133 uploads less than BASE
| Flag | BASE (40d4396) | HEAD (15e2a33) | |------|------|------| |unittests|144|12| |unittest|1|0|

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #297 +/- ## =========================================== - Coverage 38.77% 21.87% -16.91% =========================================== Files 19 19 Lines 2561 2574 +13 Branches 343 347 +4 =========================================== - Hits 993 563 -430 - Misses 1511 2002 +491 + Partials 57 9 -48 ``` | [Flag](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/297/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa) | Coverage Δ | | |---|---|---| | [unittest](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/297/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa) | `?` | | | [unittests](https://app.codecov.io/gh/pyvisa/pyvisa-py/pull/297/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa) | `21.83% <18.75%> (-0.08%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyvisa#carryforward-flags-in-the-pull-request-comment) to find out more.

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

bors[bot] commented 3 years ago

try

Build failed:

pyvisa.pyvisa-py.keysight-assisted

MatthieuDartiailh commented 3 years ago

Interestingly the self-hosted buildbot I have has 2 network card and your PR is picking up the resources on only a single one of them, could be worth investigating since that is a usual configuration ? Also It would be best to return normalized resource name using pyvisa.rname.to_canonical_name rather than names that do not include the port and the lan resource name.

MatthieuDartiailh commented 3 years ago

Playing with this locally and comparing to the output of NI-MAX, I also observe discrepancies. I am testing on a Windows 10 machine that acts as an instrument itself. NI finds the instrument under the IP 127.0.0.1 and 192.168.1.22, on the other hand pyvisa-py finds under 192.168.56.1. I am not sure what is the issue here but I will try to run some more tests during the week.

eosti commented 2 years ago

I did a little testing with this PR and found that it will only broadcast on one interface. That might explain the oddities with multiple network cards and potentially with the local device not showing up as expected (broadcasts can't respond to themselves).

I was testing with a power supply on the LAN over WiFi and a scope connected directly with a USBC->Ethernet adapter.

Both interfaces connected: only the PSU showed up
Ethernet adapter disconnected: only PSU showed up, still
Wifi disabled: only scope shows up

I assume that this is because WiFi was the default interface when it was on, so broadcasts would go through there. When WiFi is disabled, the Ethernet adapter becomes the default interface, so now broadcasts will go through that. When both are on, broadcasts only go through the default interface.

I'm not familiar with the RPC protocol library; is there a way to specify what interface to use for broadcast packets? That way you could get a list of all valid interfaces and then iterate through them, or something like that. Or maybe there's a better way, I'm really not an expert in broadcasts.

MatthieuDartiailh commented 2 years ago

The rpc library is basically bundled and has been already customized so I would not worry too much about it. Looking into it, it appears to simply call stdlib socket but I see nothing in there specific to adapters. It may be worth looking directly at the socket documentation.

eosti commented 2 years ago

Based on a bit of research, there are two ways to go about specifying an interface for broadcast.

Force a specific interface by using sock.setsockopt(socket.SOL_SOCKET, socket.SO_BINDTODEVICE, 'iface')
Exploit the OS's routing table by broadcasting to the each subnet's broadcast address, instead of the global INADDR_BROADCAST (255.255.255.255). This will automatically use the correct interface.

Method 1 needs sudo access, so that rules it out pretty quickly for me. Method 2 is doable, but it needs both the IP address and the subnet mask of each interface. IP address can be obtained from socket, but I haven't found a native way to get it otherwise.

Using the library netifaces, I was able to generate a list of broadcast addresses, and then iterate through them with pmap = rpc.BroadcastUDPPortMapperClient(ip). And I was able to see all devices across all subnets! So it is possible. My only hesitation is that netifaces isn't a standard library, so it'd need to be installed as a dependency. I don't know what the etiquette is when it comes to adding dependencies, so let me know what the best move here would be.

For reference, the snippet I've been using to generate the list of broadcast addresses:

import netifaces 
import ipaddress

broadcast_addr = []

for interface in netifaces.interfaces():
    for link in netifaces.ifaddresses(interface).get(netifaces.AF_INET, ()):
        addr = link['addr']
        mask = link['netmask']
        network = ipaddress.IPv4Network(addr + '/' + mask, strict=False)
        broadcast_addr.append(network.broadcast_address)

print(broadcast_addr)

You can then loop the discovery process for each broadcast address. I'm happy to make a PR (or edit this one), with the caveats that it would require the use of netifaces and that it'd need a bit of cleaning up (Python isn't my strongest language).

MatthieuDartiailh commented 2 years ago

I am not opposed to dependencies in general but netifaces is not maintained anymore (the repo has been archived) and does not provide wheels for Python 3.10.

It appears psutil exposes similar albeit more limited capabilities. Would it be possible to use it instead ?

eosti commented 2 years ago

Oh, good point. Got it to work with psutil:

import psutil
import socket
import ipaddress

broadcast_addr = []

for interface, snics in psutil.net_if_addrs().items():
    for snic in snics:
        # Restrict to IPv4
        if snic.family is socket.AF_INET:
            addr = snic.address
            mask = snic.netmask
            network = ipaddress.IPv4Network(addr + '/' + mask, strict=False)
            broadcast_addr.append(network.broadcast_address)

print(broadcast_addr)

I've noticed that broadcasting and waiting for responses takes a bit of time, and scales linearly with the number of IP addresses you have. It might make sense to make the default behaviour to broadcast on the default interface only (e.g. 255.255.255.255) and then add a switch to broadcast on all interfaces. This way, list_resources only is slowed down if you know that your specific setup requires it.

MatthieuDartiailh commented 2 years ago

Sounds good.

Could you either update this PR or make a new one ?

eosti commented 2 years ago

I'm working on a PR right now. ~~What would the best way be to pass a boolean into list_resources to determine if a full scan should be run or not? Ideally it'd be something that could be set using pyvisa.~~

Is there a way to access a ResourceManager's attributes from pyvisa? Could you make a class variable full_scan, then set it using rm.[something].TCPIPInstrSession.full_scan = True? Or is this totally the wrong approach? I XY-problem'ed myself -- much more efficient to optimize how the timeout period is applied when broadcasting on multiple devices.

pyvisa / pyvisa-py

tcpip: List resources to fix #165 #297

Codecov Report

try