Overhaul handling of transfer errors and use of streaming flag. #1069
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
In the case of a libusb error, we still need the transfer thread running, in order to handle outstanding cancellations and to signal the condition variable when that is done.
If result < 0 here, libusb_submit_transfer returned an error, so we need to shut down. If !resubmit, then cancel_transfers() was already called by one of the stop or close functions, so streaming is already false.
This stops the RX callback from being called again with further data once it has returned nonzero, or after a transfer had an error status.
If a transfer was cancelled, we are on our way to shutdown. If hackrf_stop_tx() or hackrf_stop_rx() were called, they will already have cleared this flag, but it is not cleared in hackrf_close(), and for consistency with other paths it makes sense to clear it here.
Since we always do these together, move it into the function.
There are now only two possible outcomes to this function: either we successfully resubmitted a transfer, or the transfer is finished and we end up calling transfer_finished(). So we can go ahead and simplify it accordingly.
Moving this into cancel_transfers() avoids duplicating it in the two stop functions.
Avoids conditionally duplicating this across three other places.
This simplifies prepare_setup_transfers(), which was just setting the flag if prepare_transfers() returned success, and passing on its return value.
This just set the do_exit flag, and was now only called in one place.
This was referenced Mar 18, 2022
|
Tested on Linux (funtoo) and Darwin (10.15.7). |
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.
This PR ensures correct behaviour during shutdown, and reviews and simplifies usage of the
streaminganddo_exitflags. The changes are as follows:request_exit()to end the transfer thread if an error occurs. It was no longer safe to do this after the changes in Fixes for concurrency bugs in start/stop operations #1029, because we need the transfer thread to keep running, to handle cancellations and signal when these are complete. Instead, if it sees an error, the callback just records the transfer as finished, and clears thestreamingflag. This fixes libhackrf hangs if HackRF is disconnected #1068.streamingflag is cleared. This ensures that:streaminganddo_exit. The result is:streamingflag is:prepare_transfers, which is called viaprepare_setup_transfersby:hackrf_start_txhackrf_start_rxhackrf_start_rx_sweepcancel_transfers, which is called by:hackrf_stop_txhackrf_stop_rxhackrf_closeviakill_transfer_threadtransfer_finished, which is called byhackrf_libusb_transfer_callbackif any transfer finishes without being successfully resubmitted.transfer_threadprociflibusb_handle_events_timeoutreturns an error.do_exitflag is set only inkill_transfer_thread, which is called only fromhackrf_close. The flag is cleared again after joining the thread.