Hi Jasper, It was interesting to read your below, because I have just been tackling more or less exactly the same issue in libhackrf [0].
It looks like you're on the right track for fixing it. In our case there were two orthogonal problems. One was that we weren't waiting for all the cancellations to complete, which you've addressed temporarily with the sleep, and are now tackling properly by counting out the cancelled transfers. Our other problem was that libhackrf was running libusb_cancel_transfer from the API call that stops the capture - which gets called from a different thread, so there was a race in which the USB handling thread could resubmit a transfer that had just completed while the cancellations were going on. I've just looked at librtlsdr and it seems it's immune to the latter problem, because rtlsdr_cancel_async() just sets a flag, and the actual cancellation is done from the same thread that handles libusb events after the handling has been interrupted. That approach is a lot simpler, but does mean that you have a delay involved, because libusb_handle_events_timeout_completed() has to either see an event or time out, before it will notice the flag and break out of its loop. That delay could be eliminated though by having rtlsdr_cancel_async() call libusb_interrupt_event_handler() after setting async_cancel = 1. That might be a change worth making, although I guess the delay is short if there is plenty of data flowing. I'm going to try adopting librtlsdr's approach for libhackrf, as it seems a lot simpler. Regards, Martin [0] https://github.com/greatscottgadgets/hackrf/pull/1029 On Mon, Jan 10, 2022 at 09:08:48PM -0000, Jasper wrote: > > Thank you Steve. > > For the async to be really watertight we probably cannot assume that all the > callbacks are executed directly after the cancels without delay. I have been > playing with a variation below that waits for all callbacks to be finished > based on counting the number of transfers cancelled. This requires a bit more > testing as has more code changes. > > For now the short Sleep has solved my issues on Windows. with device close. > Thank you for merging it. > > Kind regards, Jasper > > Author: jvde.github <jvde.git...@gmail.com> > Date: Mon Jan 10 21:40:48 2022 +0100 > > rework async_wait to ensure all events handled at close > > diff --git a/src/librtlsdr.c b/src/librtlsdr.c > index 2682d77..a4ae212 100644 > --- a/src/librtlsdr.c > +++ b/src/librtlsdr.c > @@ -125,6 +125,7 @@ struct rtlsdr_dev { > int dev_lost; > int driver_active; > unsigned int xfer_errors; > + int xfer_cb_cancel_count; > }; > > void rtlsdr_set_gpio_bit(rtlsdr_dev_t *dev, uint8_t gpio, int val); > @@ -1706,8 +1707,11 @@ static void LIBUSB_CALL _libusb_callback(struct > libusb_transfer *xfer) > dev->cb(xfer->buffer, xfer->actual_length, dev->cb_ctx); > > libusb_submit_transfer(xfer); /* resubmit transfer */ > + > dev->xfer_errors = 0; > - } else if (LIBUSB_TRANSFER_CANCELLED != xfer->status) { > + } else if (LIBUSB_TRANSFER_CANCELLED == xfer->status) { > + dev->xfer_cb_cancel_count++; > + } else { > #ifndef _WIN32 > if (LIBUSB_TRANSFER_ERROR == xfer->status) > dev->xfer_errors++; > @@ -1853,9 +1857,9 @@ int rtlsdr_read_async(rtlsdr_dev_t *dev, > rtlsdr_read_async_cb_t cb, void *ctx, > { > unsigned int i; > int r = 0; > + int ret = 0; > + int cancel_count = 0; > struct timeval tv = { 1, 0 }; > - struct timeval zerotv = { 0, 0 }; > - enum rtlsdr_async_status next_status = RTLSDR_INACTIVE; > > if (!dev) > return -1; > @@ -1865,6 +1869,7 @@ int rtlsdr_read_async(rtlsdr_dev_t *dev, > rtlsdr_read_async_cb_t cb, void *ctx, > > dev->async_status = RTLSDR_RUNNING; > dev->async_cancel = 0; > + dev->xfer_cb_cancel_count = 0; /* number of cancelled transfers */ > > dev->cb = cb; > dev->cb_ctx = ctx; > @@ -1881,6 +1886,7 @@ int rtlsdr_read_async(rtlsdr_dev_t *dev, > rtlsdr_read_async_cb_t cb, void *ctx, > > _rtlsdr_alloc_async_buffers(dev); > > + /* creating initial submits */ > for(i = 0; i < dev->xfer_buf_num; ++i) { > libusb_fill_bulk_transfer(dev->xfer[i], > dev->devh, > @@ -1890,7 +1896,6 @@ int rtlsdr_read_async(rtlsdr_dev_t *dev, > rtlsdr_read_async_cb_t cb, void *ctx, > _libusb_callback, > (void *)dev, > BULK_TIMEOUT); > - > r = libusb_submit_transfer(dev->xfer[i]); > if (r < 0) { > fprintf(stderr, "Failed to submit transfer %i\n" > @@ -1900,62 +1905,44 @@ int rtlsdr_read_async(rtlsdr_dev_t *dev, > rtlsdr_read_async_cb_t cb, void *ctx, > "echo 0 > /sys/module/usbcore" > "/parameters/usbfs_memory_mb\n", i); > dev->async_status = RTLSDR_CANCELING; > + ret = r; > break; > } > } > > - while (RTLSDR_INACTIVE != dev->async_status) { > - r = libusb_handle_events_timeout_completed(dev->ctx, &tv, > - &dev->async_cancel); > + /* handling events until device is cancelled */ > + while (RTLSDR_CANCELING != dev->async_status) { > + r = libusb_handle_events_timeout_completed(dev->ctx, &tv, > &dev->async_cancel); > + > if (r < 0) { > - /*fprintf(stderr, "handle_events returned: %d\n", r);*/ > + > if (r == LIBUSB_ERROR_INTERRUPTED) /* stray signal */ > continue; > + ret = r; > break; > } > + } > > - if (RTLSDR_CANCELING == dev->async_status) { > - next_status = RTLSDR_INACTIVE; > - > - if (!dev->xfer) > - break; > - > - for(i = 0; i < dev->xfer_buf_num; ++i) { > - if (!dev->xfer[i]) > - continue; > - > - if (LIBUSB_TRANSFER_CANCELLED != > - dev->xfer[i]->status) { > - r = > libusb_cancel_transfer(dev->xfer[i]); > - /* handle events after canceling > - * to allow transfer status to > - * propagate */ > -#ifdef _WIN32 > - Sleep(1); > -#endif > - > libusb_handle_events_timeout_completed(dev->ctx, > - > &zerotv, NULL); > - if (r < 0) > - continue; > + /* cancel and count all transfers */ > + for(i = 0; i < dev->xfer_buf_num; ++i) { > > - next_status = RTLSDR_CANCELING; > - } > - } > + if(dev->xfer[i]) { > > - if (dev->dev_lost || RTLSDR_INACTIVE == next_status) { > - /* handle any events that still need to > - * be handled before exiting after we > - * just cancelled all transfers */ > - libusb_handle_events_timeout_completed(dev->ctx, > - &zerotv, > NULL); > - break; > - } > + r = libusb_cancel_transfer(dev->xfer[i]); > + if (r == 0) cancel_count++; > } > } > > + /* handle events but expect at least "cancelled" CB calls with > LIBUSB_TRANSFER_CANCELLED */ > + do > + { > + libusb_handle_events_timeout(dev->ctx, &tv); > + } > + while (dev->xfer_cb_cancel_count != cancel_count); > + > _rtlsdr_free_async_buffers(dev); > > - dev->async_status = next_status; > + dev->async_status = RTLSDR_INACTIVE; > > return r; > } >
