-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
example/hotplugtest: Ignore unavailable devices #1425
base: master
Are you sure you want to change the base?
Conversation
This should be good to be merged. Even though Windows hotplug is not there yet, we can get the hotplugtest example ready first. |
Yes, this can be applied before the Windows hotplug is ready, but I am fine with waiting with such things until 1.0.27 is out. |
I tend to think it is better to merge before 1.0.27 release. Test under Linux (Ubuntu 20.04), external USB Type-C Hub with PICKit 4 attached.
Detaching the hub.
Attaching the hub: interestingly on the hub shows in the run log but not PICKit 4.
|
I tend to think it is better to merge before 1.0.27 release. Test under Linux (Ubuntu 20.04), external USB Type-C Hub with PICKit 4 attached.
Detaching the hub.
Attaching the hub: interestingly on the hub shows in the run log but not PICKit 4.
|
On the other hand, the above output is also kind of correct. Once I use sudo, the error goes away. Attaching the hub -- it does not show PICKit 4
Detaching the hub -- it shows PICKit 4.
|
Somehow I think the output of hotplugtest is not predictable under Linux for external hubs. Adding a USB 3.0 USB Mass Storage to the USB Type-C hub. The USB Type-C hub will show as two devices.
Detaching the hub -- only shows PICKit 4 and the associated USB 2.0 hub device.
Attaching the hub: only shows the associated USB 2.0 hub and USB 3.0 Hub.
|
Take note the above comment is not toward this PR which is a good one, but rather it is toward PR #1350. |
@tormodvolden : what about merging this one? |
2960643
to
d45154a
Compare
@tormodvolden |
I think it is fair (for a test utility or example) to report on the failed open. But I agree that "Error" is bit strong. It could just say "(Cannot open device) ...". You could argue that hotplugtest has no reason to open the devices and it should just list the detected attach/detach but I am sure there are reasons it is good to try opening the devices that are reported as attached. Maybe due to some error (in the code it is meant to test) it is not able to open the reported device even if it should be able to. Hiding this would be wrong for a test tool. If it is a test tool... hotplugtest was intended as a simple example of using the hotplug API and not a full test tool to cover all kind of scenarios. Maybe we should fork off a more advanced tool for testing the library code. One step could be to track several opened handles and close the right ones on detach, for example. And not quit after two events... |
@tormodvolden Still not clear to whether you approve this small PR or not ;-) |
If I understand your code correctly, you are just suppressing the error message. There are no other changes to the behaviour of the program. I prefer not to hide these messages, as explained above. I wouldn't see that as an improvement, or something that needs to be changed. How can you tell if e.g. LIBUSB_ERROR_NOT_FOUND means that there is a bug in the driver (or missing driver) that has nothing to do with the hotplug code (the case you would like to "hide") or if the hotplug code wrongly announced a device that is not present? What if the user wants to test a particular device, but forgets to set permissions correctly or use sudo? Should the program hide the error and make the user believe it tested it OK? I will change the wording as I suggested above, since "Error" sounds a bit alarming, but it is not very important or urgent. There is maybe a misunderstanding on what "hotplugtest" example is meant for and how it works. What I suggest is to fork off a real testing tool for hotplug with bells and whistles, covering all kind of scenarios and corner cases, and leave this example simple as it is. Sorry I think I am just repeating myself :) |
@tormodvolden : yes, I am hiding error that are known to occurs with I think it helps to reduce noise and focus on real hotplug related issues, making this simple tool more efficient, reducing false problem detection, hence reducing time not well spent. I agree that having a real test tool for hotplug would be a big plus for libsub-1.0, but given the interactive nature of hotplug and the diversity of possible test setup, it's not something easy to design. In the meantime I think this little PR would help, but I leave it up to you of course. |
I fully support improving things for helping development. The context of my response is that I was explicitly @ pinged to merge this now while we are in release freeze. I intend to work on a hotplug testing utility after the release, to be used for development (if no one beats me to it). Commit 5f9abf was merged because stress_mt is used in the automated CI tests, and failed systematically. This was more important for the release verification process. |
References #1425 Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
I did this already though :) |
d45154a
to
1081545
Compare
Depending on the devices or drivers, open() may return LIBUSB_ERROR_ACCESS LIBUSB_ERROR_NOT_FOUND LIBUSB_ERROR_NOT_SUPPORTED for devices "out of reach" to libusb.
1081545
to
5a05636
Compare
Depending on the devices or drivers, open() may return
LIBUSB_ERROR_ACCESS
LIBUSB_ERROR_NOT_FOUND
LIBUSB_ERROR_NOT_SUPPORTED
for devices "out of reach" to libusb.