From 361c4ac54fa89bbf199f2edaa5a3cc320af9261f Mon Sep 17 00:00:00 2001 From: Martin Ling Date: Tue, 8 Feb 2022 11:48:43 +0000 Subject: [PATCH 1/2] Move transceiver mode changes out of USB ISR. This is a defensive change to make the transceiver code easier to reason about, and to avoid the possibility of races such as that seen in #1042. Previously, set_transceiver_mode() was called in the vendor request handler for the SET_TRANSCEIVER_MODE request, as well in the callback for a USB configuration change. Both these calls are made from the USB0 ISR, so could interrupt the rx_mode(), tx_mode() and sweep_mode() functions at any point. It was hard to tell if this was safe. Instead, set_transceiver_mode() has been removed, and its work is split into three parts: - request_transceiver_mode(), which is safe to call from ISR context. All this function does is update the requested mode and increment a sequence number. This builds on work already done in PR #1029, but the interface has been simplified to use a shared volatile structure. - transceiver_startup(), which transitions the transceiver from an idle state to the configuration required for a specific mode, including setting up the RF path, configuring the M0, adjusting LEDs and UI etc. - transceiver_shutdown(), which transitions the transceiver back to an idle state. The *_mode() functions that implement the transceiver modes now call transceiver_startup() before starting work, and transceiver_shutdown() before returning, and all this happens in the main thread of execution. As such, it is now guaranteed that all the steps involved happen in a consistent order, with the transceiver starting from an idle state, and being returned to an idle state before control returns to the main loop. For consistency of interface, an off_mode() function has been added to implement the behaviour of the OFF transceiver mode. Since the transceiver is already guaranteed to be in an idle state when this is called, the only work required is to set the UI mode and wait for a new mode request. --- firmware/hackrf_usb/hackrf_usb.c | 23 +++---- firmware/hackrf_usb/usb_api_sweep.c | 8 ++- firmware/hackrf_usb/usb_api_transceiver.c | 73 +++++++++++++---------- firmware/hackrf_usb/usb_api_transceiver.h | 14 ++++- 4 files changed, 72 insertions(+), 46 deletions(-) diff --git a/firmware/hackrf_usb/hackrf_usb.c b/firmware/hackrf_usb/hackrf_usb.c index cf985489..7fa95c74 100644 --- a/firmware/hackrf_usb/hackrf_usb.c +++ b/firmware/hackrf_usb/hackrf_usb.c @@ -146,7 +146,7 @@ void usb_configuration_changed( usb_device_t* const device ) { /* Reset transceiver to idle state until other commands are received */ - set_transceiver_mode(TRANSCEIVER_MODE_OFF); + request_transceiver_mode(TRANSCEIVER_MODE_OFF); if( device->configuration->number == 1 ) { // transceiver configuration led_on(LED1); @@ -263,29 +263,30 @@ int main(void) { operacake_init(operacake_allow_gpio); while(true) { + transceiver_request_t request; // Briefly disable USB interrupt so that we can // atomically retrieve both the transceiver mode // and the mode change sequence number. They are - // changed together by the set_transceiver_mode - // vendor request handler. + // changed together by request_transceiver_mode() + // called from the USB ISR. nvic_disable_irq(NVIC_USB0_IRQ); - - transceiver_mode_t mode = transceiver_mode(); - uint32_t seq = transceiver_mode_seq(); - + request = transceiver_request; nvic_enable_irq(NVIC_USB0_IRQ); - switch (mode) { + switch (request.mode) { + case TRANSCEIVER_MODE_OFF: + off_mode(request.seq); + break; case TRANSCEIVER_MODE_RX: - rx_mode(seq); + rx_mode(request.seq); break; case TRANSCEIVER_MODE_TX: - tx_mode(seq); + tx_mode(request.seq); break; case TRANSCEIVER_MODE_RX_SWEEP: - sweep_mode(seq); + sweep_mode(request.seq); break; case TRANSCEIVER_MODE_CPLD_UPDATE: cpld_update(); diff --git a/firmware/hackrf_usb/usb_api_sweep.c b/firmware/hackrf_usb/usb_api_sweep.c index 52075cdb..f0ccad77 100644 --- a/firmware/hackrf_usb/usb_api_sweep.c +++ b/firmware/hackrf_usb/usb_api_sweep.c @@ -47,7 +47,7 @@ static uint32_t step_width = 0; static uint32_t offset = 0; static enum sweep_style style = LINEAR; -/* Do this before starting sweep mode with set_transceiver_mode(). */ +/* Do this before starting sweep mode with request_transceiver_mode(). */ usb_request_status_t usb_vendor_request_init_sweep( usb_endpoint_t* const endpoint, const usb_transfer_stage_t stage) { @@ -96,9 +96,11 @@ void sweep_mode(uint32_t seq) { uint8_t *buffer; bool transfer = false; + transceiver_startup(TRANSCEIVER_MODE_RX_SWEEP); + baseband_streaming_enable(&sgpio_config); - while (transceiver_mode_seq() == seq) { + while (transceiver_request.seq == seq) { // Set up IN transfer of buffer 0. if ( m0_state.offset >= 16384 && phase == 1) { transfer = true; @@ -165,4 +167,6 @@ void sweep_mode(uint32_t seq) { blocks_queued = 0; } } + + transceiver_shutdown(); } diff --git a/firmware/hackrf_usb/usb_api_transceiver.c b/firmware/hackrf_usb/usb_api_transceiver.c index 736b394f..c9a2ccd9 100644 --- a/firmware/hackrf_usb/usb_api_transceiver.c +++ b/firmware/hackrf_usb/usb_api_transceiver.c @@ -237,34 +237,43 @@ usb_request_status_t usb_vendor_request_set_freq_explicit( } } -static volatile transceiver_mode_t _transceiver_mode = TRANSCEIVER_MODE_OFF; -static volatile uint32_t _transceiver_mode_seq = 0; static volatile hw_sync_mode_t _hw_sync_mode = HW_SYNC_MODE_OFF; void set_hw_sync_mode(const hw_sync_mode_t new_hw_sync_mode) { _hw_sync_mode = new_hw_sync_mode; } -transceiver_mode_t transceiver_mode(void) { - return _transceiver_mode; +volatile transceiver_request_t transceiver_request = { + .mode = TRANSCEIVER_MODE_OFF, + .seq = 0, +}; + +// Must be called from an atomic context (normally USB ISR) +void request_transceiver_mode(transceiver_mode_t mode) +{ + transceiver_request.mode = mode; + transceiver_request.seq++; } -uint32_t transceiver_mode_seq(void) { - return _transceiver_mode_seq; -} - -void set_transceiver_mode(const transceiver_mode_t new_transceiver_mode) { +void transceiver_shutdown(void) +{ baseband_streaming_disable(&sgpio_config); operacake_sctimer_reset_state(); usb_endpoint_flush(&usb_endpoint_bulk_in); usb_endpoint_flush(&usb_endpoint_bulk_out); - _transceiver_mode = new_transceiver_mode; + led_off(LED2); + led_off(LED3); + rf_path_set_direction(&rf_path, RF_PATH_DIRECTION_OFF); + m0_state.tx = false; +} - hackrf_ui()->set_transceiver_mode(_transceiver_mode); +void transceiver_startup(const transceiver_mode_t mode) { - switch (_transceiver_mode) { + hackrf_ui()->set_transceiver_mode(mode); + + switch (mode) { case TRANSCEIVER_MODE_RX_SWEEP: case TRANSCEIVER_MODE_RX: led_off(LED3); @@ -278,24 +287,13 @@ void set_transceiver_mode(const transceiver_mode_t new_transceiver_mode) { rf_path_set_direction(&rf_path, RF_PATH_DIRECTION_TX); m0_state.tx = true; break; - case TRANSCEIVER_MODE_OFF: default: - led_off(LED2); - led_off(LED3); - rf_path_set_direction(&rf_path, RF_PATH_DIRECTION_OFF); - m0_state.tx = false; + break; } - - if( _transceiver_mode != TRANSCEIVER_MODE_OFF ) { - activate_best_clock_source(); - - hw_sync_enable(_hw_sync_mode); - - m0_state.offset = 0; - } - - _transceiver_mode_seq++; + activate_best_clock_source(); + hw_sync_enable(_hw_sync_mode); + m0_state.offset = 0; } usb_request_status_t usb_vendor_request_set_transceiver_mode( @@ -309,7 +307,7 @@ usb_request_status_t usb_vendor_request_set_transceiver_mode( case TRANSCEIVER_MODE_TX: case TRANSCEIVER_MODE_RX_SWEEP: case TRANSCEIVER_MODE_CPLD_UPDATE: - set_transceiver_mode(endpoint->setup.value); + request_transceiver_mode(endpoint->setup.value); usb_transfer_schedule_ack(endpoint->in); return USB_REQUEST_STATUS_OK; default: @@ -336,9 +334,11 @@ usb_request_status_t usb_vendor_request_set_hw_sync_mode( void rx_mode(uint32_t seq) { unsigned int phase = 1; + transceiver_startup(TRANSCEIVER_MODE_RX); + baseband_streaming_enable(&sgpio_config); - while (_transceiver_mode_seq == seq) { + while (transceiver_request.seq == seq) { // Set up IN transfer of buffer 0. if (16384 <= m0_state.offset && 1 == phase) { usb_transfer_schedule_block( @@ -360,11 +360,15 @@ void rx_mode(uint32_t seq) { phase = 1; } } + + transceiver_shutdown(); } void tx_mode(uint32_t seq) { unsigned int phase = 1; + transceiver_startup(TRANSCEIVER_MODE_TX); + memset(&usb_bulk_buffer[0x0000], 0, 0x8000); // Set up OUT transfer of buffer 1. usb_transfer_schedule_block( @@ -376,7 +380,7 @@ void tx_mode(uint32_t seq) { // Start transmitting zeros while the host fills buffer 1. baseband_streaming_enable(&sgpio_config); - while (_transceiver_mode_seq == seq) { + while (transceiver_request.seq == seq) { // Set up OUT transfer of buffer 0. if (16384 <= m0_state.offset && 1 == phase) { usb_transfer_schedule_block( @@ -398,4 +402,13 @@ void tx_mode(uint32_t seq) { phase = 1; } } + + transceiver_shutdown(); +} + +void off_mode(uint32_t seq) +{ + hackrf_ui()->set_transceiver_mode(TRANSCEIVER_MODE_OFF); + + while (transceiver_request.seq == seq); } diff --git a/firmware/hackrf_usb/usb_api_transceiver.h b/firmware/hackrf_usb/usb_api_transceiver.h index 0101bfb9..126e2f7a 100644 --- a/firmware/hackrf_usb/usb_api_transceiver.h +++ b/firmware/hackrf_usb/usb_api_transceiver.h @@ -27,6 +27,13 @@ #include #include +typedef struct { + transceiver_mode_t mode; + uint32_t seq; +} transceiver_request_t; + +extern volatile transceiver_request_t transceiver_request; + void set_hw_sync_mode(const hw_sync_mode_t new_hw_sync_mode); usb_request_status_t usb_vendor_request_set_transceiver_mode( usb_endpoint_t* const endpoint, @@ -56,11 +63,12 @@ usb_request_status_t usb_vendor_request_set_freq_explicit( usb_request_status_t usb_vendor_request_set_hw_sync_mode( usb_endpoint_t* const endpoint, const usb_transfer_stage_t stage); -transceiver_mode_t transceiver_mode(void); -uint32_t transceiver_mode_seq(void); -void set_transceiver_mode(const transceiver_mode_t new_transceiver_mode); +void request_transceiver_mode(transceiver_mode_t mode); +void transceiver_startup(transceiver_mode_t mode); +void transceiver_shutdown(void); void start_streaming_on_hw_sync(); void rx_mode(uint32_t seq); void tx_mode(uint32_t seq); +void off_mode(uint32_t seq); #endif/*__USB_API_TRANSCEIVER_H__*/ From 838ad0726c598b58036120cdd4601b5a6c30af04 Mon Sep 17 00:00:00 2001 From: Martin Ling Date: Sun, 13 Feb 2022 16:11:31 +0000 Subject: [PATCH 2/2] Flush bulk endpoints from USB ISR on mode change request. The previous change moved this flush from the vendor request handler to the transceiver_shutdown() function which runs outside ISR context. However, without this flush in the ISR, the firmware can sometimes end up stuck in TX or RX mode after a request to go IDLE. It's still not clear how this happens, but keeping the flush in the USB ISR fixes it, and as soon as a mode change is requested we know we are going to be flushing anyway, so there is no harm to do so here. This commit does not remove the flush in transceiver_shutdown(), because it is possible that the ISR will return just before the transceiver loop queues a new transfer. Rather than try to avoid that race, we can just flush again later. --- firmware/hackrf_usb/usb_api_transceiver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/firmware/hackrf_usb/usb_api_transceiver.c b/firmware/hackrf_usb/usb_api_transceiver.c index c9a2ccd9..f795808a 100644 --- a/firmware/hackrf_usb/usb_api_transceiver.c +++ b/firmware/hackrf_usb/usb_api_transceiver.c @@ -251,6 +251,9 @@ volatile transceiver_request_t transceiver_request = { // Must be called from an atomic context (normally USB ISR) void request_transceiver_mode(transceiver_mode_t mode) { + usb_endpoint_flush(&usb_endpoint_bulk_in); + usb_endpoint_flush(&usb_endpoint_bulk_out); + transceiver_request.mode = mode; transceiver_request.seq++; }