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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c4b659
expand info/debug/warning messages if USB device `serial_number` not …
jllanfranchi Apr 7, 2024
f7a08a7
Change message from USBTMC -> USB INSTR for clarity
jllanfranchi Apr 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 39 additions & 3 deletions pyvisa_py/usb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
"""

import errno
import logging
import os
import traceback
from typing import Any, List, Tuple, Type, Union

from pyvisa import attributes, constants
Expand Down Expand Up @@ -277,9 +280,9 @@ def list_resources() -> List[str]:

try:
serial = dev.serial_number
except (NotImplementedError, ValueError):
except (NotImplementedError, ValueError) as err:
msg = (
"Found a device whose serial number cannot be read."
"Found a USB INSTR device whose serial number cannot be read."
" The partial VISA resource name is: " + fmt
)
logger.warning(
Expand All @@ -292,6 +295,39 @@ def list_resources() -> List[str]:
"usb_interface_number": intfc,
},
)
logging_level = logger.getEffectiveLevel()
if logging_level <= logging.DEBUG:
if exc_strs := traceback.format_exception(err):
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the benefit of doing this over logger.debug("error while reading serial number", exc_info=err)?

I am also not sure if this amount of pre-processing is worth it. On DEBUG I'd prefer to have all exceptions logged in general. For higher levels, we can just add the exception message to the warning's dict maybe?

Copy link
Author

Choose a reason for hiding this comment

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

I was unaware of that kwarg to logger methods! Will change to using exc_info.

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 with @arr-ee here. Please update the code accordingly.

msg = "Traceback:"
logger.debug(msg)
logger.debug("-" * len(msg))
for exc_str in exc_strs:
for line in exc_str.split("\n"):
logger.debug(line)
logger.debug("-" * len(msg))
elif logging_level <= logging.INFO:
if exc_strs := traceback.format_exception_only(err):
logger.info(
"Error raised from underlying module (pyusb): %s",
exc_strs[0].strip(),
)

dev_path = f"/dev/bus/usb/{dev.bus:03d}/{dev.address:03d}"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is linux-specific and should be gated as such (see https://github.com/pyvisa/pyvisa/blob/main/pyvisa/util.py#L1005 for example).

maybe also move the check logic into a helper method in usbutil?

Copy link
Author

Choose a reason for hiding this comment

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

As to your second point, particularly if we are going to implement this for both USB INSTR and USB RAW devices, then it certainly makes sense to move the check logic into a separate module. But I can do that anyway even just to keep the code here cleaner, omitting the gory details that are only pertinent to one OS.

As to your first comment, I thought about gating based on platform. (I also tried to figure out if there's an equivalent for FreeBSD/OS X, but failed to find that.) But I figured by the time I went through all the logic required to check that the sys.platform was something this check could work on, I could do it the Python-ic (duck-typing) way and simply look for the file's existence, check the file's permissions, and go from there, and that would essentially take care of "does this check work on my particular platform?" issues.

I'm certainly not married to that way, I get why one might not want to use the duck-type perspective for this check. This almost certainly only works on Linux & nowhere else, and maybe we discover a Linux where it doesn't work, and then we would just want to add to the gating condition. So, yeah, I'll put the check within a if sys.platform.startswith("linux"): ... check.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I can do that anyway even just to keep the code here cleaner, omitting the gory details that are only pertinent to one OS.

Yup, plus it will be easier to test (that's a tricky thing though).

But I figured by the time I went through all the logic required to check that the sys.platform was something this check could work on, I could do it the Python-ic (duck-typing) way and simply look for the file's existence, check the file's permissions, and go from there, and that would essentially take care of "does this check work on my particular platform?" issues.

This is fair! I prefer an explicit check to make the intent explicit to the readers: "this is only for linux", but my disregard for pythonic way of doing things can and should be questioned :)

I also tried to figure out if there's an equivalent for FreeBSD/OS X, but failed to find that.

Yeah it's very system-specific, and I don't think we'd have permissions issue on macos (we'd have other ones!). I was wondering if there's a "better" way to get to the device file on linux as well, but it seems that string magic should do the trick until proven otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Here also I think an explicit check will make the intent clearer for people looking at it.

if os.path.exists(dev_path) and not os.access(dev_path, os.O_RDWR):
missing_perms = []
if not os.access(dev_path, os.O_RDONLY):
missing_perms.append("read from")
if not os.access(dev_path, os.O_WRONLY):
missing_perms.append("write to")
missing_perms_str = " or ".join(missing_perms)
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.

" for more info.",
missing_perms_str,
dev_path,
)
continue

out.append(
Expand Down Expand Up @@ -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.

Copy link

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

" The partial VISA resource name is: " + fmt
)
logger.warning(
Expand Down