From 0961a76f26bb670f808b77172fe72f70bd5ccf52 Mon Sep 17 00:00:00 2001 From: Adrian Chadd Date: Sun, 8 Nov 2020 21:38:39 -0800 Subject: [PATCH 1/4] Fix libusb usage for at least freebsd around the worker thread and transfer cancellation On at least freebsd-13 trying to cancel a transfer whilst the libusb thread is not running results in the transfers not completing cancellation. The next time they're attempted to be re-added the libusb code thinks they're still active, and returns BUSY on the buffers. This causes gqrx to error out when one makes DSP changes or stops/starts it. You have to restart gqrx to fix it. After digging into it a bit, the libusb code expects that you're actively running the main loop in order to have some deferred actions run in the context of said main loop thread. This includes processing cancelled transfers - the callbacks have to be run (if they exist) before the buffers are properly cancelled and have their tracking metadata (a couple of private pointers and state) removed from inside of libusb. This patch does the following: * separate out adding and cancelling transfers from the libusb worker thread create/destroy path * create the libusb worker thread when opening the device * destroy the libusb worker thread when closing the device * only add and cancel transfers when starting and stopping tx/rx * handle cancelled transfers gracefully in the USB callback Whilst here, also make the libusb device memory zeroed by using calloc instead of malloc. This fixes all of the weird libusb related buffer management problems on FreeBSD. --- host/libhackrf/src/hackrf.c | 104 ++++++++++++++++++++++++++---------- 1 file changed, 77 insertions(+), 27 deletions(-) diff --git a/host/libhackrf/src/hackrf.c b/host/libhackrf/src/hackrf.c index bc1d5fe8d..adeaa0591 100644 --- a/host/libhackrf/src/hackrf.c +++ b/host/libhackrf/src/hackrf.c @@ -119,6 +119,7 @@ struct hackrf_device { void* tx_ctx; volatile bool do_exit; unsigned char buffer[TRANSFER_COUNT * TRANSFER_BUFFER_SIZE]; + bool transfers_setup; /* true if the USB transfers have been setup */ }; typedef struct { @@ -157,6 +158,8 @@ static const uint16_t hackrf_one_usb_pid = 0x6089; static const uint16_t rad1o_usb_pid = 0xcc15; static uint16_t open_devices = 0; +static int create_transfer_thread(hackrf_device* device); + static libusb_context* g_libusb_context = NULL; int last_libusb_error = LIBUSB_SUCCESS; @@ -169,7 +172,7 @@ static int cancel_transfers(hackrf_device* device) { uint32_t transfer_index; - if( device->transfers != NULL ) + if( (device->transfers != NULL) && (device->transfers_setup == true) ) { for(transfer_index=0; transfer_indextransfers[transfer_index]); } } + device->transfers_setup = false; return HACKRF_SUCCESS; } else { return HACKRF_ERROR_OTHER; @@ -269,6 +273,7 @@ static int prepare_transfers( return HACKRF_ERROR_LIBUSB; } } + device->transfers_setup = true; return HACKRF_SUCCESS; } else { // This shouldn't happen. @@ -560,7 +565,7 @@ static int hackrf_open_setup(libusb_device_handle* usb_device, hackrf_device** d } lib_device = NULL; - lib_device = (hackrf_device*)malloc(sizeof(*lib_device)); + lib_device = (hackrf_device*)calloc(1, sizeof(*lib_device)); if( lib_device == NULL ) { libusb_release_interface(usb_device, 0); @@ -584,6 +589,14 @@ static int hackrf_open_setup(libusb_device_handle* usb_device, hackrf_device** d return HACKRF_ERROR_NO_MEM; } + result = create_transfer_thread(lib_device); + if (result != 0) { + free(lib_device); + libusb_release_interface(usb_device, 0); + libusb_close(usb_device); + return result; + } + *device = lib_device; open_devices++; @@ -1521,11 +1534,12 @@ static void LIBUSB_CALL hackrf_libusb_transfer_callback(struct libusb_transfer* }else { request_exit(device); } + } else if(usb_transfer->status == LIBUSB_TRANSFER_CANCELLED) { + /* Nothing; this will happen during shutdown */ } else { /* Other cases LIBUSB_TRANSFER_NO_DEVICE LIBUSB_TRANSFER_ERROR, LIBUSB_TRANSFER_TIMED_OUT - LIBUSB_TRANSFER_STALL, LIBUSB_TRANSFER_OVERFLOW - LIBUSB_TRANSFER_CANCELLED ... + LIBUSB_TRANSFER_STALL, LIBUSB_TRANSFER_OVERFLOW .... */ request_exit(device); /* Fatal error stop transfer */ } @@ -1535,11 +1549,25 @@ static int kill_transfer_thread(hackrf_device* device) { void* value; int result; - - request_exit(device); if( device->transfer_thread_started != false ) { + /* + * Schedule cancelling transfers before halting the + * libusb thread. This should result in the transfers + * being properly marked as cancelled. + * + * Ideally this would wait for the cancellations to + * complete with the callback but for now that + * isn't super easy to do. + */ + cancel_transfers(device); + + /* + * Now call request_exit() to halt the main loop. + */ + request_exit(device); + value = NULL; result = pthread_join(device->transfer_thread, &value); if( result != 0 ) @@ -1548,36 +1576,52 @@ static int kill_transfer_thread(hackrf_device* device) } device->transfer_thread_started = false; - /* Cancel all transfers */ - cancel_transfers(device); } + /* + * Reset do_exit; we're now done here and the thread was + * already dead or is now dead. + */ + device->do_exit = false; + return HACKRF_SUCCESS; } -static int create_transfer_thread(hackrf_device* device, +static int prepare_setup_transfers(hackrf_device* device, const uint8_t endpoint_address, hackrf_sample_block_cb_fn callback) { int result; + + if( device->transfers_setup == true ) + { + return HACKRF_ERROR_BUSY; + } + + device->callback = callback; + result = prepare_transfers( + device, endpoint_address, + hackrf_libusb_transfer_callback + ); + + if( result != HACKRF_SUCCESS ) + { + return result; + } + + return HACKRF_SUCCESS; +} + +static int create_transfer_thread(hackrf_device* device) +{ + int result; if( device->transfer_thread_started == false ) { device->streaming = false; device->do_exit = false; - result = prepare_transfers( - device, endpoint_address, - hackrf_libusb_transfer_callback - ); - - if( result != HACKRF_SUCCESS ) - { - return result; - } - device->streaming = true; - device->callback = callback; result = pthread_create(&device->transfer_thread, 0, transfer_threadproc, device); if( result == 0 ) { @@ -1625,7 +1669,7 @@ int ADDCALL hackrf_start_rx(hackrf_device* device, hackrf_sample_block_cb_fn cal if( result == HACKRF_SUCCESS ) { device->rx_ctx = rx_ctx; - result = create_transfer_thread(device, endpoint_address, callback); + result = prepare_setup_transfers(device, endpoint_address, callback); } return result; } @@ -1633,7 +1677,7 @@ int ADDCALL hackrf_start_rx(hackrf_device* device, hackrf_sample_block_cb_fn cal int ADDCALL hackrf_stop_rx(hackrf_device* device) { int result; - result = kill_transfer_thread(device); + result = cancel_transfers(device); if (result != HACKRF_SUCCESS) { return result; @@ -1649,7 +1693,7 @@ int ADDCALL hackrf_start_tx(hackrf_device* device, hackrf_sample_block_cb_fn cal if( result == HACKRF_SUCCESS ) { device->tx_ctx = tx_ctx; - result = create_transfer_thread(device, endpoint_address, callback); + result = prepare_setup_transfers(device, endpoint_address, callback); } return result; } @@ -1657,7 +1701,7 @@ int ADDCALL hackrf_start_tx(hackrf_device* device, hackrf_sample_block_cb_fn cal int ADDCALL hackrf_stop_tx(hackrf_device* device) { int result; - result = kill_transfer_thread(device); + result = cancel_transfers(device); if (result != HACKRF_SUCCESS) { return result; @@ -1668,13 +1712,15 @@ int ADDCALL hackrf_stop_tx(hackrf_device* device) int ADDCALL hackrf_close(hackrf_device* device) { - int result1, result2; + int result1, result2, result3; result1 = HACKRF_SUCCESS; result2 = HACKRF_SUCCESS; - + result3 = HACKRF_SUCCESS; + if( device != NULL ) { + result3 = kill_transfer_thread(device); result1 = hackrf_stop_rx(device); result2 = hackrf_stop_tx(device); if( device->usb_device != NULL ) @@ -1690,6 +1736,10 @@ int ADDCALL hackrf_close(hackrf_device* device) } open_devices--; + if (result3 != HACKRF_SUCCESS) + { + return result3; + } if (result2 != HACKRF_SUCCESS) { return result2; @@ -2174,7 +2224,7 @@ int ADDCALL hackrf_start_rx_sweep(hackrf_device* device, hackrf_sample_block_cb_ if (HACKRF_SUCCESS == result) { device->rx_ctx = rx_ctx; - result = create_transfer_thread(device, endpoint_address, callback); + result = prepare_setup_transfers(device, endpoint_address, callback); } return result; } From 9a278d267a1672cc664b027db666337e43b33eb2 Mon Sep 17 00:00:00 2001 From: Adrian Chadd Date: Mon, 9 Nov 2020 09:37:49 -0800 Subject: [PATCH 2/4] Fix streaming hangs in consumers * Update device->streaming to reflect whether we're streaming data, rather than just whether the streaming thread is active. The streaming thread is now always active! --- host/libhackrf/src/hackrf.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/host/libhackrf/src/hackrf.c b/host/libhackrf/src/hackrf.c index adeaa0591..564a1f24d 100644 --- a/host/libhackrf/src/hackrf.c +++ b/host/libhackrf/src/hackrf.c @@ -322,6 +322,7 @@ static int detach_kernel_drivers(libusb_device_handle* usb_device_handle) static int set_hackrf_configuration(libusb_device_handle* usb_device, int config) { int result, curr_config; + result = libusb_get_configuration(usb_device, &curr_config); if( result != 0 ) { @@ -1496,7 +1497,7 @@ static void* transfer_threadproc(void* arg) int error; struct timeval timeout = { 0, 500000 }; - while( (device->streaming) && (device->do_exit == false) ) + while(device->do_exit == false ) { error = libusb_handle_events_timeout(g_libusb_context, &timeout); if( (error != 0) && (error != LIBUSB_ERROR_INTERRUPTED) ) @@ -1542,6 +1543,7 @@ static void LIBUSB_CALL hackrf_libusb_transfer_callback(struct libusb_transfer* LIBUSB_TRANSFER_STALL, LIBUSB_TRANSFER_OVERFLOW .... */ request_exit(device); /* Fatal error stop transfer */ + device->streaming = false; } } @@ -1615,13 +1617,11 @@ static int prepare_setup_transfers(hackrf_device* device, static int create_transfer_thread(hackrf_device* device) { int result; - + if( device->transfer_thread_started == false ) { device->streaming = false; device->do_exit = false; - - device->streaming = true; result = pthread_create(&device->transfer_thread, 0, transfer_threadproc, device); if( result == 0 ) { @@ -1639,7 +1639,7 @@ static int create_transfer_thread(hackrf_device* device) int ADDCALL hackrf_is_streaming(hackrf_device* device) { /* return hackrf is streaming only when streaming, transfer_thread_started are true and do_exit equal false */ - + if( (device->transfer_thread_started == true) && (device->streaming == true) && (device->do_exit == false) ) @@ -1665,24 +1665,30 @@ int ADDCALL hackrf_start_rx(hackrf_device* device, hackrf_sample_block_cb_fn cal { int result; const uint8_t endpoint_address = LIBUSB_ENDPOINT_IN | 1; + device->rx_ctx = rx_ctx; result = hackrf_set_transceiver_mode(device, HACKRF_TRANSCEIVER_MODE_RECEIVE); if( result == HACKRF_SUCCESS ) { - device->rx_ctx = rx_ctx; result = prepare_setup_transfers(device, endpoint_address, callback); } + if (result == HACKRF_SUCCESS) { + device->streaming = true; + } return result; } int ADDCALL hackrf_stop_rx(hackrf_device* device) { int result; + + device->streaming = false; result = cancel_transfers(device); if (result != HACKRF_SUCCESS) { return result; } - return hackrf_set_transceiver_mode(device, HACKRF_TRANSCEIVER_MODE_OFF); + result = hackrf_set_transceiver_mode(device, HACKRF_TRANSCEIVER_MODE_OFF); + return result; } int ADDCALL hackrf_start_tx(hackrf_device* device, hackrf_sample_block_cb_fn callback, void* tx_ctx) @@ -1695,19 +1701,23 @@ int ADDCALL hackrf_start_tx(hackrf_device* device, hackrf_sample_block_cb_fn cal device->tx_ctx = tx_ctx; result = prepare_setup_transfers(device, endpoint_address, callback); } + if (result == HACKRF_SUCCESS) { + device->streaming = true; + } return result; } int ADDCALL hackrf_stop_tx(hackrf_device* device) { int result; + device->streaming = false; result = cancel_transfers(device); if (result != HACKRF_SUCCESS) { return result; } - - return hackrf_set_transceiver_mode(device, HACKRF_TRANSCEIVER_MODE_OFF); + result = hackrf_set_transceiver_mode(device, HACKRF_TRANSCEIVER_MODE_OFF); + return result; } int ADDCALL hackrf_close(hackrf_device* device) @@ -2226,6 +2236,9 @@ int ADDCALL hackrf_start_rx_sweep(hackrf_device* device, hackrf_sample_block_cb_ device->rx_ctx = rx_ctx; result = prepare_setup_transfers(device, endpoint_address, callback); } + if (result == HACKRF_SUCCESS) { + device->streaming = true; + } return result; } From b4ea51a36bb1baef6925fe3d76574ff1b73b98f4 Mon Sep 17 00:00:00 2001 From: Adrian Chadd Date: Mon, 9 Nov 2020 09:43:40 -0800 Subject: [PATCH 3/4] add 10ms sleep after stop This seems to stop consumers that are doing quick back to back stop/start (eg gqrx changing decode mode / filter bandwidth) from hanging the device. I now don't have any weird hangs on hackrf with gqrx/freebsd/libusb! When things hang it isn't erroring out in any way; it just doesn't start receive. It doesn't look like a libusb issue; I'd have to get some USB bus sniffing to see what's going on behind the scenes. --- host/libhackrf/src/hackrf.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/host/libhackrf/src/hackrf.c b/host/libhackrf/src/hackrf.c index 564a1f24d..feda181b1 100644 --- a/host/libhackrf/src/hackrf.c +++ b/host/libhackrf/src/hackrf.c @@ -25,6 +25,9 @@ ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSI #include #include +#ifndef _WIN32 +#include +#endif #include #ifdef _WIN32 @@ -1688,6 +1691,11 @@ int ADDCALL hackrf_stop_rx(hackrf_device* device) return result; } result = hackrf_set_transceiver_mode(device, HACKRF_TRANSCEIVER_MODE_OFF); +#ifdef _WIN32 + Sleep(10); +#else + usleep(10 * 1000); +#endif return result; } @@ -1717,6 +1725,11 @@ int ADDCALL hackrf_stop_tx(hackrf_device* device) return result; } result = hackrf_set_transceiver_mode(device, HACKRF_TRANSCEIVER_MODE_OFF); +#ifdef _WIN32 + Sleep(10); +#else + usleep(10 * 1000); +#endif return result; } From 61a06b904dbf5e54da1d84473004db0472950487 Mon Sep 17 00:00:00 2001 From: adrian chadd Date: Thu, 10 Dec 2020 15:57:54 -0800 Subject: [PATCH 4/4] Handle hackrf_close() being called without TX or RX being started. My previous commits didn't handle the specific case of hackrf_close() being called without the transfers being active. In this instance the transfers haven't been setup, so calling cancel_transfers() returned an error. Instead: * refactor out the tx/rx stop command from canceling transfers * send the tx/rx stop command without canceling transfers, since .. * ... we can then destroy the transfer thread. I may also need to put an explicit cancel_transfers() before the call to send the tx/rx stop commands; I'll look at doing that in a subsequent commit. --- host/libhackrf/src/hackrf.c | 97 ++++++++++++++++++++++++++++++------- 1 file changed, 80 insertions(+), 17 deletions(-) diff --git a/host/libhackrf/src/hackrf.c b/host/libhackrf/src/hackrf.c index feda181b1..4ccaeb84f 100644 --- a/host/libhackrf/src/hackrf.c +++ b/host/libhackrf/src/hackrf.c @@ -171,11 +171,38 @@ static void request_exit(hackrf_device* device) device->do_exit = true; } +/* + * Check if the transfers are setup and owned by libusb. + * + * Returns true if the device transfers are currently setup + * in libusb, false otherwise. + */ +static int transfers_check_setup(hackrf_device* device) +{ + if( (device->transfers != NULL) && (device->transfers_setup == true) ) + return true; + return false; +} + +/* + * Cancel any transfers that are in-flight. + * + * This cancels any transfers that hvae been given to libusb for + * either transmit or receive. + * + * This must be done whilst the libusb thread is running, as + * on some platforms cancelling transfers requires some work + * to be done inside the libusb thread to completely cancel + * pending transfers. + * + * Returns HACKRF_SUCCESS if OK, HACKRF_ERROR_OTHER if the + * transfers aren't currently setup. + */ static int cancel_transfers(hackrf_device* device) { uint32_t transfer_index; - if( (device->transfers != NULL) && (device->transfers_setup == true) ) + if(transfers_check_setup(device) == true) { for(transfer_index=0; transfer_indexstreaming = false; - result = cancel_transfers(device); - if (result != HACKRF_SUCCESS) - { - return result; - } result = hackrf_set_transceiver_mode(device, HACKRF_TRANSCEIVER_MODE_OFF); #ifdef _WIN32 Sleep(10); @@ -1699,6 +1720,28 @@ int ADDCALL hackrf_stop_rx(hackrf_device* device) return result; } +/* + * Stop any pending receive. + * + * This call stops transfers and halts recieve if it is enabled. + * + * It returns HACKRF_SUCCESS if receive was started and it was + * properly stopped, an error otherwise. + */ +int ADDCALL hackrf_stop_rx(hackrf_device* device) +{ + int result; + + device->streaming = false; + result = cancel_transfers(device); + if (result != HACKRF_SUCCESS) + { + return result; + } + + return hackrf_stop_rx_cmd(device); +} + int ADDCALL hackrf_start_tx(hackrf_device* device, hackrf_sample_block_cb_fn callback, void* tx_ctx) { int result; @@ -1715,15 +1758,9 @@ int ADDCALL hackrf_start_tx(hackrf_device* device, hackrf_sample_block_cb_fn cal return result; } -int ADDCALL hackrf_stop_tx(hackrf_device* device) +static int hackrf_stop_tx_cmd(hackrf_device* device) { int result; - device->streaming = false; - result = cancel_transfers(device); - if (result != HACKRF_SUCCESS) - { - return result; - } result = hackrf_set_transceiver_mode(device, HACKRF_TRANSCEIVER_MODE_OFF); #ifdef _WIN32 Sleep(10); @@ -1733,6 +1770,27 @@ int ADDCALL hackrf_stop_tx(hackrf_device* device) return result; } +/* + * Stop any pending transmit. + * + * This call stops transfers and halts transmit if it is enabled. + * + * It returns HACKRF_SUCCESS if receive was started and it was + * properly stopped, an error otherwise. + */ +int ADDCALL hackrf_stop_tx(hackrf_device* device) +{ + int result; + device->streaming = false; + result = cancel_transfers(device); + if (result != HACKRF_SUCCESS) + { + return result; + } + + return hackrf_stop_tx_cmd(device); +} + int ADDCALL hackrf_close(hackrf_device* device) { int result1, result2, result3; @@ -1743,9 +1801,14 @@ int ADDCALL hackrf_close(hackrf_device* device) if( device != NULL ) { + result1 = hackrf_stop_rx_cmd(device); + result2 = hackrf_stop_tx_cmd(device); + + /* + * Finally kill the transfer thread, which will + * also cancel any pending transmit/receive transfers. + */ result3 = kill_transfer_thread(device); - result1 = hackrf_stop_rx(device); - result2 = hackrf_stop_tx(device); if( device->usb_device != NULL ) { libusb_release_interface(device->usb_device, 0);