windows: hotplug implementation #1406
Conversation
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. |
|
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! |
|
Or maybe we we could do, in order to preserve compatibility with pre-hotplug Windows client implementation, is to implement an additional mechanism to disable/enable hotplug support at runtime and have it disabled by default on Windows. With this in place, existing implementation updating their libsub-1.0 to the newer hotplug-capable version would not see a difference since hotplug would be disabled. Implementation wanting to benefit from new hotplug should enable it. This is doable, but of course add a layer of complexity and maintenance. It is more a "political" decision than a "technical" one. So far I don't have better ideas / input. @tormodvolden : any thought? |
Hello @anqiren , I identified the cause, which is actually quite trivial, but not easy to fix with how I implemented things and what Windows gives us to actually implement anything at all. When you do the pnputil /restart-device, the device is indeed quickly removed and re-added to the system. But by the time the hotplug event thread receive the "DBT_DEVICEREMOVECOMPLETE" notification, the device is actually back online already. So when this event is processed, enumerating all now-present devices in order to find which one left (the difference with respect to the previous list), it does not find any, all devices are there. So no device is triggering an hotplug event, nothing happens. The same happens when we receive the "DBT_DEVICEARRIVAL": the implementation cannot detect the new device since when comparing with the previous list, nothing changed. So yes, you won't get LEFT / ARRIVAL events in this case. The implementation is made such that there is a sort of implicit time window or "grace period" within which devices can be quickly removed and re-added (or added then removed) without triggering any visible libusb-1.0 hotplug events. From the point of view where hotplug events are actually used only to trigger refreshes of the current device list, this is OK: "macroscopically" it is OK to say that nothing changed, but I understand that "microscopically" this doesn't look right and might cause issues to implementations relying on a 1:1 relationship in between low level events and libusb-1.0 events. When receiving DBT events we could check in the DEV_BROADCAST_DEVICEINTERFACE structure for the name of the device that triggered the system event and use this for triggering a libusb event on the correct device object. I decided not to do this when I worked on the hotplug feature because the name of the device (as far as I could understand) only reflect VID / PID / SerialNumber, but doesn't provide a unique ID per device instance. Therefore if there are more than one device on the system with same VID/PID/SerialNumber, it won't be possible to find with certainty which libusb-1.0 device object to trigger the event for. Libusb-1.0 can identify each device instance specifically but this level of details is not available at the Windows event level. Of course, when there is only one device with a given VID/PID/SerialNumber plugged at a time, then we could search for the name in the current libusub-1.0 device list and trigger events for it. But this won't work as soon as there are more than one. So, I decided to no implement this at all. Now, if the community prefer another approach, I could work on using the device name in the Windows event to match a device in the libusb-1.0 list and triggering events when there is one and only one with certainty. That would probably solve most real cases, but I don't know. I don't really like the idea that resulting events would depend on the number and type of devices plugged. What do you think @anqiren ? Or @mcuee or @tormodvolden ? Does anyone figured out a way to get a sort of per device unique id from Windows events? Thanks! |
|
@anqiren , well, actually, after I wrote my previous message, I found evidence that dbcc_name could actually be unique. |
…dbcc_name for finding corresponding device in current context list and triggering LEFT or ARRIVED event. (No longer set LEFT / ARRIVED status during re-enumeration.)
|
@anqiren I think I found a simple way to achieve a better implementation that nonetheless fix your issue, but also simplify existing code (and potentially add robustness). The assumption is that Windows will do what is necessary to create a unique name per device even when more than one devices have equal VID, PID, SerialName. I will try to find a correct way to verify this assumption, unless someone can validate it for me. @anqiren Could you please test my latest commit on this PR ("hotplug implementation: use PDEV_BROADCAST_DEVICEINTERFACE->dbcc_name (...)" and let me known whether everything works for you? @tormodvolden @mcuee @fabiensanglard @xhwang-China @whitequark and all: what do you think? Thanks |
|
@sonatique Happy to test with the Glasgow Interface Explorer which uses hotplug functionality to detect the FX2 re-enumerating. Do you happen to have a prebuilt libusb-1.0.dll I can use? |
|
@whitequark here you go: (release X64 dll, had to zip it for being able to attach it here): |
|
Yes I will carry out the tests again. |
|
More test results: using Cypress FX2LP (downloading the Cystream FW to remunerate as a new device and then start hotplugtest example, then reset the board, and download the FW again). Looks okay. I also checked other test cases and verified that there are no devices ref/unref related issues. |
Now that libusb will support hotplug events on Windows shortly (see libusb/libusb#1406), the hotplug wait path will get exercised. Currently it does not wait for long enough; it takes about 5.5 s on a representative Windows desktop machine used for testing. This commit raises re-enumeration timeout to 10.0 s, and ensure that the timeout is respected no matter how many hotplug events are received. The higher fixed timeout on older libusb on Windows isn't disruptive, and with the next libusb release will become irrelevant for most.
Now that libusb will support hotplug events on Windows shortly (see libusb/libusb#1406), the hotplug wait path will get exercised. Currently it does not wait for long enough; it takes about 5.5 s on a representative Windows desktop machine used for testing. This commit raises re-enumeration timeout to 10.0 s, and ensure that the timeout is respected no matter how many hotplug events are received. The higher fixed timeout on older libusb on Windows isn't disruptive, and with the next libusb release will become irrelevant for most.
Verified that everything works with the Glasgow Interface Explorer. I had to raise the timeout I've had a little (see GlasgowEmbedded/glasgow#601), it turns out 5 s isn't quite enough. Otherwise the existing Linux developed code worked perfectly. |
Now that libusb will support hotplug events on Windows shortly (see libusb/libusb#1406), the hotplug wait path will get exercised. Currently it does not wait for long enough; it takes about 5.5 s on a representative Windows desktop machine used for testing. This commit raises re-enumeration timeout to 10.0 s, and ensure that the timeout is respected no matter how many hotplug events are received. The higher fixed timeout on older libusb on Windows isn't disruptive, and with the next libusb release will become irrelevant for most.
|
Awesome work! We (@hsorbo and I) just took this for a spin in Frida, and noticed an edge-case when modeswitching an iDevice. Turns out |
Hello everyone! I have tested attached dll in our environment. It works properly with "single driver" device, but it doesn't work correctly with composite devices. I just can't open a composite device when it is detected by hotplug. After a start of an application, it works correctly. I suppose it is the same issue as @oleavr mentioned. A year ago, I developed my own implementation of hotplug, based on version 1.0.26. It uses some very old code + a lot of my rework. You can find the code here. I'm sorry for not creating PR, but I didn't have time to manage it that time. What is important, it works in both situations i.e. for single driver device and composite device. Maybe it can help you or I can help with this anyhow. Regarding the issue, I have found in my code, that I implemented it using the fact, that WM_DEVICECHANGE event provides an information what has changed. When it provides information that usb device was attached, my code creates usb device. Then it receives events about setup of each interface of composite device, so it configures interfaces on corresponding events. It also doesn't refresh all the devices, (I'm worry it can be slow in some situations), but only adds or removes what arrives in the event. Unfortunately, when hotplug event occures, interfaces can be not set up yet, but this is how Windows works and maybe it is not an issue. On the other hand, we don't know whether an interface receives an event or not, so we can't wait for it (e.g. It doesn't receive an event when driver is not installed, or it is disabled). If you wish, you can test my implementation by using this file: |
|
@pgorgon-hem : interesting, thank you! I am looking at your code. Note that the reason for reusing the big enumeration loop in my implementation was to try to maximize backward compatibility + not re-inventing the wheel or copy/pasting/re-writing a lot of code in another hotplug-specific function, given that we still need the big enumeration to create the starting point state. However from your message it seems that I missed some way to extract more information about the device triggering the event, that is interesting. I'll try to extract this from your code, or find another way to fix the issue, thanks, |
I'm aware of that we need starting point, and this is why I left original function and called it during init. Host devices are detected only there, and I suppose it is correct. To make all of this work I had to import a few more functions from Windows API, there was some specific need to convert 2 different handles as I remember. I can make PR of my code for reference. Then it is easier to see what has changed. What do you think? I really would like to contribute, but I don't want to compete with you in PRs. PS. I also slightly changed behavior of devices which are disabled or have no driver, because they are not operational. I know that it is different than on linux, but on linux you don't need driver to communicate with a device, so I think it is just platform specific thing. PS2. I was thinking about the compatibility issue. Maybe it is good idea to make it configurable in runtime? I did it in compile time but doing it in the same way for runtime seams only a little more work. Another idea is creating 3rd windows port, which could share the same code, but have different behavior. Then we still need some mechanism to tell the core library whether hotplug is supported or not, but switching is almost done then. And what is more important it gives us a little more space for slightly changing some behaviors and give users choice if they want to stay with current implementation or go into next. After some time, it would be even possible to change the default driver. PS3. Maybe it would be a good idea to refactor winusb_get_device_list function, because it is very unclear how it works, which slows down development. |
|
Hi @sonatique I'm trying to setup a test envrionment on windows 11 by building this branch. OS Name : Microsoft Windows 11 Pro P.S. stress_mt would also throw |
Hello @DoubleSpicy, |
|
@sonatique I was too sleepy last night, simply didn't checkout the branch, some simple tests by unplugging my joysticks (both XInput and DirectInput) looks good. I will see more possibilities in terms of joystick. P.S. @oleavr I also found similar issue when writing some joystick hooks recently, raw input events sent out to another thread would occasionally be reordered, probably is a general behavior for all windows event message queues. |
I re-tested Android @tormodvolden @mcuee: What about me merge this PR
|
|
I would very much like to see the PR merged, personally. I realize there are some minor issues but I feel that this is a "perfect is the enemy of good" case. |
I will support your recommendation. But then @tormodvolden will have the say since I am not touching the codes (mainly on testing and support side). |
|
My personal (and professional) opinions about Windows aside... I see no reason not to merge this and deal with issues as they come up. Having developers use this will help shake out the remaining bugs. |
|
I have expressed my concerns e.g. in #1406 (comment) and to repeat some of it: This is not an opt-in feature, the new hotplug code will be used by all programs and users. We must therefore assure there are no significant regressions. I would prefer to avoid any opt-in switch as discussed in #1406 (comment) in particular if it adds APIs that we need to carry. I have mentioned the missing hotplug_poll and suggested a possible failure pattern. And I have not seen reports of testing the new library code on an old application doing its own hotplug reenumeration, that would have reassured me in this regard. If anyone wants to help this getting merged, that would be a good way to contribute. (That said I believe it is common in the Windows world to bundle library copies with each application so end users won't see their application break because of a system libusb update. So I am a little less afraid of regressions on this platform that only affects a small number of applications.) Is the issue seen by @anqiren confirmed as resolved in the latest revision? There are recently some new ideas from @pgorgon-hem and @sonatique and I am curious where that discussion is going. |
I confirm this scenario works (at least on |
|
That means your application has already been prepared for using libusb's hotplug and does the right thing. The scenario I am interested in is an unmodified Windows application always doing its own hotplug detection, unaware of what libusb's new capabilities are. |
I think this is exactly the case, when we break compatibility with composite devices. App switches to new hotplug implementation, because |
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!