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

expand info/debug/warning msgs if USB device serial_number not readable #423

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jllanfranchi
Copy link

if INSTR USB device,

  • say that device is USB INSTR that failed in warning message
  • catch exception
  • debug message with traceback if logging level is DEBUG
  • info message 1-line error summary if logging level is INFO
  • if path f"/dev/bus/usb/{dev.bus:03d}/{dev.address:03d}" exists, check permissions on that file & refer to FAQ link (known issue on Linux)

if RAW USB device,

  • say that device is USB RAW that failed in warning message

I will work on documentation fixes now, but wanted to get this in front of you for feedback in the meantime, @arr-ee (et al.).

  • Working to close pyvisa issue #758
  • Executed black . && isort -c . && flake8 with no errors black & isort OK; flake8 doesn't like a few lines' lengths
  • The change is fully covered by automated unit tests (TODO; feedback for what this could be?)
  • Documented in docs/ as appropriate (TODO)
  • Added an entry to the CHANGES file (TODO)

@jllanfranchi
Copy link
Author

Examples of messages running the code

import logging
import pyvisa

logging.basicConfig(level=logging.DEBUG)
rm = pyvisa.ResourceManager("@py")
rm.list_resources()

on Linux with permissions /dev/usb/001/016 not set correctly (I also have a USB RAW device on my system, so you can see the modified message for that as well).

  • logging level set to WARN:
WARNING:pyvisa:Found a USB INSTR device whose serial number cannot be read. The partial VISA resource name is: USB0::4883::32888::???::0::INSTR
WARNING:pyvisa:User does not have permission to write to /dev/bus/usb/001/016, so the above USB INSTR device cannot be used by pyvisa; see https://pyvisa.readthedocs.io/projects/pyvisa-py/en/latest/faq.html for more info.
WARNING:pyvisa:Found a USB RAW device whose serial number cannot be read. The partial VISA resource name is: USB0::1027::24593::???::0::RAW
  • logging level set to INFO:
WARNING:pyvisa:Found a USB INSTR device whose serial number cannot be read. The partial VISA resource name is: USB0::4883::32888::???::0::INSTR
INFO:pyvisa:Error raised from underlying module (pyusb): ValueError: The device has no langid (permission issue, no string descriptors supported or device error)
WARNING:pyvisa:User does not have permission to write to /dev/bus/usb/001/016, so the above USB INSTR device cannot be used by pyvisa; see https://pyvisa.readthedocs.io/projects/pyvisa-py/en/latest/faq.html for more info.
WARNING:pyvisa:Found a USB RAW device whose serial number cannot be read. The partial VISA resource name is: USB0::1027::24593::???::0::RAW
  • with logging level set to DEBUG:
DEBUG:pyvisa:SerialSession was correctly imported.
DEBUG:pyvisa:USBSession and USBRawSession were correctly imported.
DEBUG:pyvisa:TCPIPSession was correctly imported.
DEBUG:pyvisa:GPIBSession was not imported No module named 'gpib'.
DEBUG:pyvisa:Created library wrapper for py
DEBUG:pyvisa:Created ResourceManager with session 1292698
WARNING:pyvisa:Found a USB INSTR device whose serial number cannot be read. The partial VISA resource name is: USB0::4883::32888::???::0::INSTR
DEBUG:pyvisa:Traceback:
DEBUG:pyvisa:----------
DEBUG:pyvisa:Traceback (most recent call last):
DEBUG:pyvisa:
DEBUG:pyvisa:  File "/home/justinl/src/pyvisa-py/pyvisa_py/usb.py", line 282, in list_resources
DEBUG:pyvisa:    serial = dev.serial_number
DEBUG:pyvisa:
DEBUG:pyvisa:  File "/home/justinl/miniconda3/envs/py310/lib/python3.10/site-packages/usb/core.py", line 864, in serial_number
DEBUG:pyvisa:    self._serial_number = util.get_string(self, self.iSerialNumber)
DEBUG:pyvisa:
DEBUG:pyvisa:  File "/home/justinl/miniconda3/envs/py310/lib/python3.10/site-packages/usb/util.py", line 313, in get_string
DEBUG:pyvisa:    raise ValueError("The device has no langid"
DEBUG:pyvisa:
DEBUG:pyvisa:ValueError: The device has no langid (permission issue, no string descriptors supported or device error)
DEBUG:pyvisa:
DEBUG:pyvisa:----------
WARNING:pyvisa:User does not have permission to write to /dev/bus/usb/001/016, so the above USB INSTR device cannot be used by pyvisa; see https://pyvisa.readthedocs.io/projects/pyvisa-py/en/latest/faq.html for more info.
WARNING:pyvisa:Found a USB RAW device whose serial number cannot be read. The partial VISA resource name is: USB0::1027::24593::???::0::RAW
DEBUG:asyncio:Using selector: EpollSelector

Copy link
Contributor

@arr-ee arr-ee left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this! Left some notes, will test the code in a bit.

Please note I do not have authority over the code, @MatthieuDartiailh has the last word. My comments are suggestions at best.

pyvisa_py/usb.py Outdated Show resolved Hide resolved
pyvisa_py/usb.py Outdated Show resolved Hide resolved
@@ -336,7 +372,7 @@ def list_resources() -> List[str]:
serial = dev.serial_number
except (NotImplementedError, ValueError, usb.USBError):
msg = (
"Found a device whose serial number cannot be read."
"Found a USB RAW device whose serial number cannot be read."
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we run into the same permissions issue here?

Copy link
Author

Choose a reason for hiding this comment

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

We do. I thought pyvisa didn't care about RAW devices (because it doesn't control them), but I realize now that assumption may be incorrect.

Copy link
Member

Choose a reason for hiding this comment

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

USB RAW are weird because there are supported by NI but are not part of the IVI specs. If we can provide more information we should though.

Choose a reason for hiding this comment

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

I observed in pyvisa issue Linux host and getting permissions to talk to a USBTMC device
USBTMC devices show up as both /dev/usbtmcN and /dev/bus/usb/NNN/NNN
Both instances need to have suitable permissions for pyvisa with pyvisa-py to be able to communicate

If only the /dev/usbtmcN has permissions then you get WARNING Found a device whose serial number cannot be read

pyvisa_py/usb.py Outdated
logger.warning(
"User does not have permission to %s %s, so the above USB INSTR"
" device cannot be used by pyvisa; see"
" https://pyvisa.readthedocs.io/projects/pyvisa-py/en/latest/faq.html"
Copy link
Contributor

Choose a reason for hiding this comment

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

should link to the specific anchor once docs are updated

Copy link
Author

Choose a reason for hiding this comment

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

Only issue with that is we need to keep the two in sync; a change in the docs will necessitate a change in the code. That could be annoying for a maintainer, so I left it out. But if that's what you want, I'm happy add the anchor to the link.

Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt that docs organisation will change drastically enough to make this a big issue, and since we're going to the trouble of linking we might as well link to the exact spot.

Copy link
Member

Choose a reason for hiding this comment

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

I agree a direct link would be better. Anything that can reduce the number of obscure bug reports is something I am happy to include.

@jllanfranchi
Copy link
Author

Really appreciate all your helpful feedback, @arr-ee . I will push updates according to your comments ASAP, and hopefully have a first pass at adding to the docs that can be reviewed as well.

I want to re-iterate how helpful the feedback you gave is, not really being aware of all the ins-and-outs of pyvisa & pyvisa-py (like code organization in the usbutils module; wouldn't have thought of that as an outsider).

Copy link
Member

@MatthieuDartiailh MatthieuDartiailh left a comment

Choose a reason for hiding this comment

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

Looks good. Once the minor requested changes are in I will merge.

pyvisa_py/usb.py Outdated Show resolved Hide resolved
pyvisa_py/usb.py Outdated Show resolved Hide resolved
pyvisa_py/usb.py Outdated
logger.warning(
"User does not have permission to %s %s, so the above USB INSTR"
" device cannot be used by pyvisa; see"
" https://pyvisa.readthedocs.io/projects/pyvisa-py/en/latest/faq.html"
Copy link
Member

Choose a reason for hiding this comment

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

I agree a direct link would be better. Anything that can reduce the number of obscure bug reports is something I am happy to include.

@@ -336,7 +372,7 @@ def list_resources() -> List[str]:
serial = dev.serial_number
except (NotImplementedError, ValueError, usb.USBError):
msg = (
"Found a device whose serial number cannot be read."
"Found a USB RAW device whose serial number cannot be read."
Copy link
Member

Choose a reason for hiding this comment

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

USB RAW are weird because there are supported by NI but are not part of the IVI specs. If we can provide more information we should though.

@MatthieuDartiailh
Copy link
Member

@jllanfranchi any idea when you will be able to address the comments ?

@MatthieuDartiailh
Copy link
Member

Friendly ping @jllanfranchi

jllanfranchi and others added 3 commits October 28, 2024 17:53
…readable

if INSTR USB device,
- say that device is USB INSTR that failed in warning message
- catch exception
- debug message with traceback if logging level is DEBUG
- info message 1-line error summary if logging level is INFO
- if path f"/dev/bus/usb/{dev.bus:03d}/{dev.address:03d}" exists, check
  permissions on that file & refer to FAQ link (known issue on Linux)

if RAW USB device,
- say that device is USB RAW that failed in warning message
@MatthieuDartiailh
Copy link
Member

I took the liberty to rebase and address the log and the platform gating. @arr-ee we have no entry in the FAQ for the permissions but we do have one in the installation. Would you have time to look at how to improve it ? in particular it may make sense to include @DavidLutton latest comment somewhere.

Copy link

codecov bot commented Oct 28, 2024

Codecov Report

Attention: Patch coverage is 19.04762% with 17 lines in your changes missing coverage. Please review.

Project coverage is 24.45%. Comparing base (64296aa) to head (da964f3).

Files with missing lines Patch % Lines
pyvisa_py/usb.py 19.04% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #423      +/-   ##
==========================================
- Coverage   24.47%   24.45%   -0.03%     
==========================================
  Files          23       23              
  Lines        3485     3505      +20     
  Branches      398      405       +7     
==========================================
+ Hits          853      857       +4     
- Misses       2627     2643      +16     
  Partials        5        5              
Flag Coverage Δ
unittests 24.45% <19.04%> (-0.03%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

4 participants