Run USB as an idle task instead of a custom FD-based GSource #242
Conversation
| glib_binary_age, glib_interface_age)); | ||
| l = g_slist_append(l, m); | ||
|
|
||
| #ifdef CONF_ZLIB_VERSION |
There was a problem hiding this comment.
These missing ifdefs caused me some issues when building the code, they really should be here
| resubmit_transfer(transfer); | ||
| } | ||
|
|
||
| static int receive_data(int fd, int revents, void *cb_data) |
There was a problem hiding this comment.
With this change, drivers no longer need to implement callbacks if all they are doing is calling libusb_handle_events_timeout(). I tried to update a few of them, but frankly IDK if I have the time & energy to update all of them.
There was a problem hiding this comment.
Even if your USB source changes get merged, you should probably update the drivers in a separate patch. This just makes the patchset larger and harder to review.
|
Nothing useful to add; I really appreciate your efforts on this and thought it worth mentioning! |
|
Thanks @multiplemonomials for providing a possible solution for this incompatibility. I tried it as well and it seems to work for me at least for sampling rates up to 4MHz with the fx2 driver. For long recording times the chances, that sampling doesn't finish, increases. I assume, that timing of critical USB routines is border line. Keep up the good work and let's keep fingers crossed, that we soon have again a working release of PV on Windows. |
|
Thanks for working on this! Do you consider this change set ready for merging? |
|
I'd really like for someone with understanding of sigrok architecture to comnent on whether this is the "right" way to do it... but it seems like several people have tested it with different hardware and it's worked fine, so that's good. In particular I am worried that blocking the GSource from executing other things could have other bad effects. Do you know if that's a concern? |
|
Not directly related to this PR, but really just a sanity check for me... A colleague and I each have a Kingst LA2016 which are working with the currently nightly branch. I haven't gone through various build histories to see what might have changed (although I did have a quick look at the change log), but it definitely wasn't working the previous time I had tried it (february). |
|
I also had this problem a few months back but it seems to be solved in the nightly build. I am using one of the cheap Saleae clones. |
biot
left a comment
biot
left a comment
There was a problem hiding this comment.
Sorry it took so long to get your code reviewed. Some inline comments above, but in general I'm not convinced.
While I'm shocked that libsigrok USB on windows appears to be broken, your notes on it seem plausible: given that FDs don't work on windows, it'll fall back on timeouts, so work a little but not very well, and data will get lost.
However your patch affects all platforms, which I think you should avoid. This code works fine on other platforms, and should not be broken because it fails on Windows.
I don't know if libusb provides an alternative for FDs on Windows, but surely there's a better way than doing what's basically a busy loop -- that is never a good idea. It seems to me that a Windows-specific version of the USB code should use Windows features in libusb, and this is the way to solve this problem.
Additionally, the fact that your code still tops out or 2MHz or 4MHz points to it not being a solution at all: this is not deterministic. Surely Windows can get as close to saturating a USB bulk channel as well as Linux can.
| m = g_slist_append(NULL, g_strdup("libzip")); | ||
| m = g_slist_append(m, g_strdup_printf("%s", CONF_LIBZIP_VERSION)); | ||
| l = g_slist_append(l, m); | ||
| #endif |
There was a problem hiding this comment.
In fact libzip is a required dependency, libsigrok will not build without it. So there is no need for this #ifdef.
There was a problem hiding this comment.
OK will do. Actually funny story, I am so dependent on CLion for my dev work that, in order to develop this patch, I reimplemented about half the libsigrok build system in CMake so that I could open the project in CLion. So that's how I found this, but I am happy to revert it.
btw, if you guys are interested in a CMake build system option, I could see about polishing up that code some more and contributing it (though I am not 100% sure I still have it anymore)
There was a problem hiding this comment.
@multiplemonomials I'm speaking for myself here, but I'd appreciate the move to cmake. IDE support being one of the reasons, although the autotools setup currently seems to work fine. I suggest you hit the mailing list and we can have a discussion there?
| m = g_slist_append(NULL, g_strdup("zlib")); | ||
| m = g_slist_append(m, g_strdup_printf("%s", CONF_ZLIB_VERSION)); | ||
| l = g_slist_append(l, m); | ||
| #endif |
There was a problem hiding this comment.
If this solved a problem for you that's good, but I would recommend sticking this in a separate patch, not conflated with some serious core USB code.
| resubmit_transfer(transfer); | ||
| } | ||
|
|
||
| static int receive_data(int fd, int revents, void *cb_data) |
There was a problem hiding this comment.
Even if your USB source changes get merged, you should probably update the drivers in a separate patch. This just makes the patchset larger and harder to review.
|
@biot Thanks for the review. I went through the code a bit too and while I'm still not 100% sure what exactly is going on, I have some thoughts.
@multiplemonomials Before we go too deep into this, are you willing to work on this issue? It's been a bit too long since this was posted, but if you're willing, I'd be happy to collaborate and find a good solution. |
|
Hi, thank you for reviewing! Glad that someone more familiar with this codebase could take a look.
Strictly speaking, this isn't a busy loop. We call the blocking version of the libusb poll function, so it will do a thread sleep until something interesting happens with the USB device. So yes we are looping the same event over and over in the event loop, but this is not a busy loop that will consume more CPU than it should.
As far as I am aware, there are no additional Windows-specific features for this. There are only the blocking and nonblocking versions of the USB API functions. So you have to either use the blocking ones from their own thread, or use the nonblocking ones and set up callbacks.
Actually it appears that I was far too pessimistic when testing this. With the same FX2LAFW hardware, I could sometimes hit 4/8MHz on Linux, but after more testing I realized I can only reliably hit 2MHz there as well. So the performance of the windows one is slightly worse but not a night and day difference. Also I did some more digging and sounds like this may be a limitation of the FX2LAFW stack, it has a super small buffer on the chip so it's pretty sensitive to minor delays, and it's more sensitive the faster you run it. I'm not a USB expert so that's the most I can say X_X. |
Well, to be honest, I no longer have an immediate use case for this fix. I have switched to using Sigrock from Linux only in my workflow, which is not 100% ideal but I can live with it. Also, Cypress has now terminated production of the FX2LP chips that are used with FX2LAFW, so I will have to switch to something else in the next revision of my board -- tentatively planning on using RP2040s, which communicate over USB UART so this is not an issue (AFAIK). However, I am still happy to help out with getting this patch submitted, within reasonable constraints on time and effort. |
|
@multiplemonomials FX2LAFW never really worked for me. On a good day, I got passable results on linux when the device was the only device on the root hub. These days there are better affordable alternatives. RP2040: This is a firmware for RP2040 that is supported by sigrok: https://github.com/pico-coder/sigrok-pico . I didn't try it, but it might save you some work. Back to the topic: I'm not really excited about reworking a core feature like this, so I'd prefer if we could find a less intrusive way to patch it. |
|
Yes, I saw that! It seems promising, though it does have the caveat that the maximum continuous sample rate is much lower (500Ksps for 4 channels, instead of the 4Msps that an FX2LP can get on a good day). Being honest, with my other time constraints, and the fact that it no longer blocks my work, I don't think I can rework this PR to operate a different way. So I'm happy to leave it open, or transfer to someone else, or close it. |
7 participants
This PR is my best attempt to restore compatibility with USB devices on Windows. I described my basic approach here in detail. To briefly summarize, this PR sets it up so that the USB event source works as an idle source that continually polls libusb rather than as a custom FD GSource that tries to expose libusb's file descriptors to glib. The file descriptor method is complex and seems like it will probably be a non-starter on Windows (unless one were to go ham and implement another thread that monitors libusb and then exposes semaphores to glib...).
What I can say about this method is that it works, more or less. On my machine, with my fx2lafw based analyzer and sigrok + pulseview built from source in MSYS2:
I don't know exactly why 2MHz is the limit, and am unsure if it's an inherent limitation of the hardware and/or Windows, or if there's something not quite right about my implementation. Also, a bigger potential issue is that, since GLib does not implement any sort of round robin scheduling, and the USB source has a blocking poll method, no other event sources will be able to execute in that SR session's main loop until the USB capture is done. Frankly, I don't know sigrok well enough to know if that's an issue or not -- someone more familiar will have to weigh in.
But, for now, this fix seems like a step in the right direction, at least.