-
Notifications
You must be signed in to change notification settings - Fork 40
fix: handle i2c device permissions errors gracefully #409
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
base: main
Are you sure you want to change the base?
Conversation
|
Coveralls tests seems to be due to adding more lines to a chunk of code which was already untested. That code probably should be tested, but I'm not yet sure the most optimal way to do so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! Sorry for the slow review, this got buried at the bottom of my inbox.
Coveralls tests seems to be due to adding more lines to a chunk of code which was already untested. That code probably should be tested, but I'm not yet sure the most optimal way to do so.
That's fine, small decreases are ok by me. I should probably change the threshold.
| except (OSError, VCPIOError): | ||
| pass | ||
| except VCPPermissionError as exc: | ||
| logging.error("Unable to check i2c device: %s", exc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| logging.error("Unable to check i2c device: %s", exc) | |
| logging.exception("Unable to check i2c device") |
This adds native exception info into the logging record.
| else: | ||
| logging.error( | ||
| "Unable to check i2c device %s: no device number found", device | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious, have you seen this occur? pyudev should only return None if the device number is not a unicode string: https://github.com/pyudev/pyudev/blob/7588bc8c84bf1e994b31020452124bc6618625ce/src/pyudev/device/_device.py#L626-L651
| for device in pyudev.Context().list_devices(subsystem="i2c"): | ||
| vcp = LinuxVCP(device.sys_number) | ||
| try: | ||
| if sys_no := device.sys_number: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if sys_no := device.sys_number: | |
| if (sys_no := device.sys_number) is None: |
I think 0 is a valid device.sys_number.
This fix gracefully handles i2c devices which we do not have permissions to open; this means that we no longer have to have every i2c device on the system accessible by the current user in order to use the tool.