windows: hotplug implementation #1406
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sonatique
force-pushed
the
windows-hotplug
branch
3 times, most recently
from
4ecda70
to
4119dde
Compare
|
Thanks for working on this code! I have some applications which would benefit from hotplug notifications, but I'm not sure when I'll be able to test them. I gave the diff a quick look. One obvious issue is that you have a mixture of tabs and spaces that makes it a bit hard to read. I do want to encourage you to look into using |
|
Thanks a lot for working on this long outstanding feature for libusb Windows. I will for sure test this PR. |
|
excellent, As #86 (comment) suggests，it works well in my Project. as you‘ve said， i took a lot of tests too, it passed. |
Oups, sorry, I didn't notice this, thanks for reporting. I will correct this asap.
This is interesting: do you have any idea why people would be so reluctant in having a Windows message pump somewhere in the code? I mean, the basic mechanism used for linux udev hotplug is not fundamentally different: there is a background thread waiting on some event to unblock, and then doing a switch on something and doing the actual hotplug sutff. With udev we also need some "hidden" objects (file descriptor and event) and a thread for "pumping messages". Anyway, I will look at CM_Register_Notification, but so far I didn't manage to make it work. |
|
I found One issue it may have is that these events are delivered pretty early, and the device may not be ready for reading. |
@mcuee @tormodvolden @whitequark : I need to use tabs (no spaces) right? I didn't find this coding requirement in any document. |
sonatique
force-pushed
the
windows-hotplug
branch
2 times, most recently
from
cbfe18c
to
8726a33
Compare
|
The first test seems to be in the positive direction. Not so sure about the Detail debug log: two devices still referenced Debug log for hotplugtest.exeMore about the device (Microchip PICKit 4) which is a USB Composite Device Debug log for xusb example using Microchip PICKit 4 |
|
@mcuee : macOS build has some intermittent "stress_test" failures that are obviously not related to my changes. Any idea? |
|
Thanks for your tests @mcuee . I will have a look. One obvious issue on my side is this: |
You can ignore that issue. It seems to be specific to the CI machine. |
I think the general guideline is not to changing the coding style of existing codes. And I remember the original Windows source codes from @pbatard were using |
|
@whitequark @yume-chan : as far as I understand, using "CM_Register_Notification" requires to move to Universal (UWP) platform (https://learn.microsoft.com/fr-fr/windows/win32/api/cfgmgr32/nf-cfgmgr32-cm_register_notification) . I didn't manage to simply correctly include required header without this. If this PR gives good results as is I think we should keep it as it will enable hotplug without breaking anything. Another option is to create a new UWP library with a different implementation for windows_hotplug.c, I could explore this possibility further if there is some interest for it. By the way, I am not sure that CM_Register_Notification is not actually only hiding away some code very similar to what I did ;-) |
sonatique
force-pushed
the
windows-hotplug
branch
2 times, most recently
from
02bd73a
to
7502234
Compare
@mcuee : Would you mind testing again for this issue? I just forced push a fix and don't see it anymore on my side. Thanks! |
|
According to the documentation, Line 12 in 285b292 If we want to use libusb/libusb/os/windows_winusb.h Lines 229 to 231 in aac2e12 libusb/libusb/os/windows_winusb.c Lines 179 to 181 in 8450cc9 It will require copy-pasting all related header definitions. |
@sonatique I have looked at the disassembly of |
|
Interestingly, I don't have Vista DLLs on hand to check there. |
|
Testing results of latest git push. No more
Debug message for the test cases of attaching the external USB Type-C Hub. Debug message when detaching the external USB Type-C Hub which the Microchip PICKit 4 is connected to |
Thanks a lot for all this information. I think that the decision of whether support Windows 8+ or not is not to be taken by me. @whitequark : thanks a lot as well for these interesting facts. May I ask how you did manage to have a clear disassembly of CM_Register_Notification? I am not really experienced with disassemmbling C/C++ binaries, thanks in advance. Now, trying to get a bigger picture: what are the known drawbacks of using the message pump in the way I did it? Or, put the other way round: what benefits could we expect from moving to CM_Register_Notification? Thanks! |
|
@mcuee : thanks for reporting; I will try to reproduce. I this point I am not sure whether I introduced a problem or not. I mean: hotplug test code tries to open whatever device triggers hotplug events. Some of them may not be "open-able" at all Anyway I will either try to fix it or commit some additional tests for you to run, if you're OK. |
tormodvolden
pushed a commit
that referenced
this pull request
Mar 14, 2024
Remove all parents with a single child remaining in parent tree. This ensures that no parents of the direct parent of the device being considered are left in the list, when appearing before their child(ren) in the processing order of the context device list cleaning. References #1452 References #1406
tormodvolden
pushed a commit
that referenced
this pull request
Mar 14, 2024
|
I am probably not going to block this on the hotplug_poll() issue, but it is good if we are all aware of this potential issue, and can try to find a way to minimize it before next release. Other platforms like darwin and linux simply read events until there are no more pending events, suggesting everything has quieted down (maybe this also increases the chance for scheduling, so that the hotplug thread is more likely to get its work done). Can we do something similar on Windows? It would also be great if all application developers who have their own event detection in combination with libusb_get_device_list() can verify that it still works with this PR applied. What I can imagine as possible symptoms is that the devices detection is intermittently lagging, with a plugged device only "showing up" (maybe with failing access) when it is unplugged again (supposing single devices being plugged). |
And (for a "legacy" application) this could typically be the exact state change that triggered their own event detection. |
|
Hello @tormodvolden : sorry I didn't have time to reply or work on this but I am taking your concern seriously. I read your comments (including that one from 4 days ago) carefully and I think I understand the problem. I did not find an easy way to address it, but I am not desperate I can. I just need some amount of free time to work on it. |
|
hollo @sonatique : this feature help me a lot, thanks. but recently I have an issue on windows hotplug. if I restart (use pnputil or devcon.exe to Disconnect / Connect a USB device "softly".) a USB device, i can't get the ARRIVED or LEFT event. I restart a device like this: libusb did receive both the DBT_DEVICEREMOVECOMPLETE and DBT_DEVICEARRIVAL events after restart device. |
Interesting finding. Let's see if @sonatique can identify the root cause or not. But to me this may be a corner case we can deal with in the future. |
|
Hello @anqiren , thank you for the feedback and for the issue report. I guess I'll have to test it to understand what is happening, I have to admit I didn't try this scenario. |
yes, thanks for reply @sonatique, this is what i expected. |
This was referenced Apr 7, 2024
|
About hotplug_poll(): If I understand correctly, the Linux implementation of it processes any pending hotplug events and updates the internal list of devices before libusb_get_device_list() returns the new list, all on the calling thread, whereas the macOS implementation of it waits for things to settle down while the hotplug thread is processing everything. |
This reminds me about commit 4b732d9. Is it a possible challenge that we receive Windows events when the device appear, but we'd prefer to ignore it until it has been configured, for which we probably do not receive any event? |
|
@sonatique i think this is important for libusb, i see there is "This branch has conflicts that must be resolved", can you fix it ? |
This is just an automatic warning from GitHub, and in this case nothing to worry about. The "conflict" is trivial and in a code comment, no need to do anything. |
Is there anything else I should do to get this PR merged? Who is in charge of merging from here? |
You can carry out test and report the positive results. As for merging, this PR is a big PR and @tormodvolden may need some time to review it before merging. As of now @tormodvolden and @hjelmn are the two main developer and committer for libusb project. |
|
I also tested it by https://github.com/nxp-imx/mfgtools and wait for merge into main line. Everything is good. |
This was taking things out of context. I was referring to the merge conflict that someone was concerned about. Yes, more testing is very welcome. Meanwhile we haven't heard from the submitter in almost two months, this is fine, but also means we are not rushing anything in their absence. There is a reported issue with pnputil, I don't know how important that is. There is the missing hotplug_poll, I also don't know how important that is. For the latter issue: I would kindly ask anyone using a "home-made" hotplug scheme in their applications (calling libusb_get_device_list repeatedly) to test backwards-compatibility of this PR by using the new libusb but without making changes to the application. Especially watch out for issues with late or out-of-sync notification. Mind also that with this PR, the new hotplug notification going on behind the scene is not an opt-in functionality. This will be used by everyone, with libusb_get_device_list() now running on top of it. So it is an important change. This has a performance impact, a good one for those who need updates often but have few real USB changes, but a bad one for those who need rare updates but has a lot of real USB changes on the machine since a full enumeration task will run all the time, although I am sure the latter use case is rare (and probably not something Windows deals well with anyway) so we can live with it. Also, a well-written multi-platform program will automatically switch to "proper" hotplug operation because the hotplug capability now will be reported. So this change needs a lot of testing before it can be released. Since many of us don't even use Windows, we are grateful if the community can help testing out and report their experience. |
|
Hello @tormodvolden, @mcuee , et al. |
Apologies about the message, I thought I was replying on PR #1428 (for which I am the contributor and trying to get merged). I agree with everything you mentioned @tormodvolden. Sorry again about the confusion. |
sonatique
force-pushed
the
windows-hotplug
branch
from
85f0a61
to
f1534e4
Compare
|
I tested the latest patch again with Devices reconnect as expected after awakening. Everything is working well :) ! This is an awesome patch! @tormodvolden and @hjelmn: Did you get a chance to look at it? |
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.
This fixes an issue where an error that occurred in thread #X will create a false error message stating that thread #X+1 did not discover the correct number of devices
Implemented as is a three steps process:
1. We create a monitor on GUID_DEVINTERFACE_USB_DEVICE via a hidden window.
2. Upon notification of an event, we run the current windows backend to get
the list of devices. This updates the hotplug status of each device to one
of three values {UNCHANGED, ARRIVED, LEFT}.
3. According to the value, we generate events to libusb client via hotplug
callbacks.
sonatique
force-pushed
the
windows-hotplug
branch
from
f1534e4
to
2454f1a
Compare
|
Hello again @tormodvolden, I am back at looking at the "hotplug_poll" issue. And seeking for some advices about how to solve it.
For solving it, my base concern is that I want to avoid fully enumerating the device list every time the user call So, I am thinking about what you wrote. Say the user application registered its own device plug/unplug detection (ignoring LIBUSB_CAP_HAS_HOTPLUG). Since it is decoupled from libusb hotplug event loop, it may receive its event well before libusb event loop realize something happened. I am just paraphrasing what you wrote, for now. When the user call So: how could I implement But if the call to I had a look at linux implementation: hotplug_poll As far as my knowledge goes, there is no such "event register" to poll under Windows, we need to register a callback. This means that in order to mimic what Linux does, I would have to recreate such "event register" internally. But this register won't be in sync with whatever mechanism the user is using, and there would therefore be no guarantee that a call to The only thing I can think of for now is to add a (bool) parameter to What I can do otherwise is implement Can you think of something better? What shall I do in your opinion? I am sorry I don't have the magical solution you may have expected. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hotplug implementation for Windows.
The idea is to use WM_DEVICECHANGE message to trigger a re-enumeration, then figure out which device(s) have been removed or added and trigger corresponding changes in ctx internal device list as well as relevant hotplug events.
I tried to minimize changes on existing code, basically reusing well tested main windows_winusb and windows_usbdk "get_device_list" implementation. I had to do small changes though, but without putting this code at risk, I think.
I tried to avoid re-enumerating on WM_DEVICECHANGE notifications, but didn't figure out how to get all required information about the triggering device. And anyway, all my attempts were actually doing a full enumeration, and I realized I was writing "get_device_list" again, but without taking all subtleties into account, so I reverted using existing code as much as possible.
As far as I can tell this approach works well, hotplugtest works as expected and I didn't see any issue so far.
Note that I used the approach of having a "hidden window" and a message pump in a dedicated thread. I tried to somehow mimic how it is done for linux, but dealing with Windows API. I understand having a "hidden Windows" might sound weird but in the end I didn't find any major drawback and we probably can consider this an implementation detail.
Now that I have a working solution as a reference, I will explore whether CM_Register_Notification could be used to improve code and avoid the hidden window, as suggested here: #86 (comment)
I am totally OK to consider this PR only as a basis for discussion toward the optimal solution, but I thought it would worth publishing it since it could already be useful as is.
Beside providing hotplug events, this implementation avoid fully enumerating devices at system level every time user calls "libusb_get_device_list" since we're reusing ctx internal device list. I think that this alone should provide some desired improvements.
I had a hard time trying to comply with both coding style of Windows C and existing libusb-1.0 code for the Windows backend. I took some liberty in the new windows_hotplug.c file.
Let me know how all this works for you, I greatly welcome feedback regarding the code itself or any issue you may report when testing this code on your system. Thank you in advance!