expansion and serial fixes and new api

by HaxSam & WillyJL

CHANGELOG.md

@@ -36,6 +36,8 @@
   - SmartRider parser (by @jaylikesbunda)
 * Apps: **Check out more Apps updates and fixes by following** [this link](https://github.com/xMasterX/all-the-plugins/commits/dev)
 ## Other changes
+* FuriHalSerial: Fix RXFNE interrupt hang, aka freezing with UART output when Expansion Modules are enabled (by @WillyJL)
+* Expansion: add is_connected api (by @HaxSam & @WillyJL)
 * RFID 125khz: Fix strange bug with LCD backlight going off after doing "Write"
 * GUI: Added `submenu_remove_item()` to API, was needed for NFC Type 4 related changes (by @WillyJL)
 * SubGHz: Fix possible frequency analyzer deadlock when holding Ok (by @WillyJL)

applications/services/expansion/expansion.c

@@ -18,6 +18,7 @@ typedef enum {
     ExpansionStateDisabled,
     ExpansionStateEnabled,
     ExpansionStateRunning,
+    ExpansionStateConnectionEstablished,
 } ExpansionState;
 
 typedef enum {
@@ -27,10 +28,13 @@ typedef enum {
     ExpansionMessageTypeReloadSettings,
     ExpansionMessageTypeModuleConnected,
     ExpansionMessageTypeModuleDisconnected,
+    ExpansionMessageTypeConnectionEstablished,
+    ExpansionMessageTypeIsConnected,
 } ExpansionMessageType;
 
 typedef union {
     FuriHalSerialId serial_id;
+    bool* is_connected;
 } ExpansionMessageData;
 
 typedef struct {
@@ -67,13 +71,21 @@ static void expansion_detect_callback(void* context) {
     UNUSED(status);
 }
 
-static void expansion_worker_callback(void* context) {
+static void expansion_worker_callback(void* context, ExpansionWorkerCallbackReason reason) {
     furi_assert(context);
     Expansion* instance = context;
 
-    ExpansionMessage message = {
+    ExpansionMessage message;
-        .type = ExpansionMessageTypeModuleDisconnected,
+    switch(reason) {
-        .api_lock = NULL, // Not locking the API here to avoid a deadlock
+    case ExpansionWorkerCallbackReasonExit:
+        message.type = ExpansionMessageTypeModuleDisconnected;
+        message.api_lock = NULL; // Not locking the API here to avoid a deadlock
+        break;
+
+    case ExpansionWorkerCallbackReasonConnected:
+        message.type = ExpansionMessageTypeConnectionEstablished;
+        message.api_lock = api_lock_alloc_locked();
+        break;
     };
 
     const FuriStatus status = furi_message_queue_put(instance->queue, &message, FuriWaitForever);
@@ -106,7 +118,9 @@ static void
     UNUSED(data);
     if(instance->state == ExpansionStateDisabled) {
         return;
-    } else if(instance->state == ExpansionStateRunning) {
+    } else if(
+        instance->state == ExpansionStateRunning ||
+        instance->state == ExpansionStateConnectionEstablished) {
         expansion_worker_stop(instance->worker);
         expansion_worker_free(instance->worker);
     } else {
@@ -124,7 +138,9 @@ static void expansion_control_handler_set_listen_serial(
     if(instance->state != ExpansionStateDisabled && instance->serial_id == data->serial_id) {
         return;
 
-    } else if(instance->state == ExpansionStateRunning) {
+    } else if(
+        instance->state == ExpansionStateRunning ||
+        instance->state == ExpansionStateConnectionEstablished) {
         expansion_worker_stop(instance->worker);
         expansion_worker_free(instance->worker);
 
@@ -182,7 +198,8 @@ static void expansion_control_handler_module_disconnected(
     Expansion* instance,
     const ExpansionMessageData* data) {
     UNUSED(data);
-    if(instance->state != ExpansionStateRunning) {
+    if(instance->state != ExpansionStateRunning &&
+       instance->state != ExpansionStateConnectionEstablished) {
         return;
     }
 
@@ -192,6 +209,23 @@ static void expansion_control_handler_module_disconnected(
         instance->serial_id, expansion_detect_callback, instance);
 }
 
+static void expansion_control_handler_connection_established(
+    Expansion* instance,
+    const ExpansionMessageData* data) {
+    UNUSED(data);
+    if(instance->state != ExpansionStateRunning &&
+       instance->state != ExpansionStateConnectionEstablished) {
+        return;
+    }
+
+    instance->state = ExpansionStateConnectionEstablished;
+}
+
+static void
+    expansion_control_handler_is_connected(Expansion* instance, const ExpansionMessageData* data) {
+    *data->is_connected = instance->state == ExpansionStateConnectionEstablished;
+}
+
 typedef void (*ExpansionControlHandler)(Expansion*, const ExpansionMessageData*);
 
 static const ExpansionControlHandler expansion_control_handlers[] = {
@@ -201,6 +235,8 @@ static const ExpansionControlHandler expansion_control_handlers[] = {
     [ExpansionMessageTypeReloadSettings] = expansion_control_handler_reload_settings,
     [ExpansionMessageTypeModuleConnected] = expansion_control_handler_module_connected,
     [ExpansionMessageTypeModuleDisconnected] = expansion_control_handler_module_disconnected,
+    [ExpansionMessageTypeConnectionEstablished] = expansion_control_handler_connection_established,
+    [ExpansionMessageTypeIsConnected] = expansion_control_handler_is_connected,
 };
 
 static int32_t expansion_control(void* context) {
@@ -295,6 +331,22 @@ void expansion_disable(Expansion* instance) {
     api_lock_wait_unlock_and_free(message.api_lock);
 }
 
+bool expansion_is_connected(Expansion* instance) {
+    furi_check(instance);
+    bool is_connected;
+
+    ExpansionMessage message = {
+        .type = ExpansionMessageTypeIsConnected,
+        .data.is_connected = &is_connected,
+        .api_lock = api_lock_alloc_locked(),
+    };
+
+    furi_message_queue_put(instance->queue, &message, FuriWaitForever);
+    api_lock_wait_unlock_and_free(message.api_lock);
+
+    return is_connected;
+}
+
 void expansion_set_listen_serial(Expansion* instance, FuriHalSerialId serial_id) {
     furi_check(instance);
     furi_check(serial_id < FuriHalSerialIdMax);

applications/services/expansion/expansion.h

@@ -50,6 +50,15 @@ void expansion_enable(Expansion* instance);
  */
 void expansion_disable(Expansion* instance);
 
+/**
+ * @brief Check if an expansion module is connected.
+ *
+ * @param[in,out] instance pointer to the Expansion instance.
+ *
+ * @returns true if the module is connected and initialized, false otherwise.
+ */
+bool expansion_is_connected(Expansion* instance);
+
 /**
  * @brief Enable support for expansion modules on designated serial port.
  *

applications/services/expansion/expansion_worker.c

@@ -35,7 +35,8 @@ typedef enum {
     ExpansionWorkerFlagError = 1 << 2,
 } ExpansionWorkerFlag;
 
-#define EXPANSION_ALL_FLAGS (ExpansionWorkerFlagData | ExpansionWorkerFlagStop)
+#define EXPANSION_ALL_FLAGS \
+    (ExpansionWorkerFlagData | ExpansionWorkerFlagStop | ExpansionWorkerFlagError)
 
 struct ExpansionWorker {
     FuriThread* thread;
@@ -225,6 +226,7 @@ static bool expansion_worker_handle_state_handshake(
 
         if(furi_hal_serial_is_baud_rate_supported(instance->serial_handle, baud_rate)) {
             instance->state = ExpansionWorkerStateConnected;
+            instance->callback(instance->cb_context, ExpansionWorkerCallbackReasonConnected);
             // Send response at previous baud rate
             if(!expansion_worker_send_status_response(instance, ExpansionFrameErrorNone)) break;
             furi_hal_serial_set_br(instance->serial_handle, baud_rate);
@@ -360,6 +362,8 @@ static int32_t expansion_worker(void* context) {
         expansion_worker_state_machine(instance);
     }
 
+    furi_hal_serial_async_rx_stop(instance->serial_handle);
+
     if(instance->state == ExpansionWorkerStateRpcActive) {
         expansion_worker_rpc_session_close(instance);
     }
@@ -371,7 +375,7 @@ static int32_t expansion_worker(void* context) {
 
     // Do not invoke worker callback on user-requested exit
     if((instance->exit_reason != ExpansionWorkerExitReasonUser) && (instance->callback != NULL)) {
-        instance->callback(instance->cb_context);
+        instance->callback(instance->cb_context, ExpansionWorkerCallbackReasonExit);
     }
 
     return 0;

applications/services/expansion/expansion_worker.h

@@ -17,14 +17,20 @@
  */
 typedef struct ExpansionWorker ExpansionWorker;
 
+typedef enum {
+    ExpansionWorkerCallbackReasonExit,
+    ExpansionWorkerCallbackReasonConnected,
+} ExpansionWorkerCallbackReason;
+
 /**
  * @brief Worker callback type.
  *
  * @see expansion_worker_set_callback()
  *
  * @param[in,out] context pointer to a user-defined object.
+ * @param[in] reason reason for the callback.
  */
-typedef void (*ExpansionWorkerCallback)(void* context);
+typedef void (*ExpansionWorkerCallback)(void* context, ExpansionWorkerCallbackReason reason);
 
 /**
  * @brief Create an expansion worker instance.

targets/f7/furi_hal/furi_hal_serial.c

@@ -817,6 +817,21 @@ static void furi_hal_serial_async_rx_configure(
     FuriHalSerialHandle* handle,
     FuriHalSerialAsyncRxCallback callback,
     void* context) {
+    // Disable RXFNE interrupts before unsetting the user callback that reads data
+    // Otherwise interrupt runs without reading data and without clearing RXFNE flag
+    // This would cause a system hang as the same interrupt runs in loop forever
+    if(!callback) {
+        if(handle->id == FuriHalSerialIdUsart) {
+            LL_USART_DisableIT_RXNE_RXFNE(USART1);
+            furi_hal_interrupt_set_isr(FuriHalInterruptIdUart1, NULL, NULL);
+            furi_hal_serial_usart_deinit_dma_rx();
+        } else if(handle->id == FuriHalSerialIdLpuart) {
+            LL_LPUART_DisableIT_RXNE_RXFNE(LPUART1);
+            furi_hal_interrupt_set_isr(FuriHalInterruptIdLpUart1, NULL, NULL);
+            furi_hal_serial_lpuart_deinit_dma_rx();
+        }
+    }
+
     // Handle must be configured before enabling RX interrupt
     // as it might be triggered right away on a misconfigured handle
     furi_hal_serial[handle->id].rx_byte_callback = callback;
@@ -824,27 +839,17 @@ static void furi_hal_serial_async_rx_configure(
     furi_hal_serial[handle->id].rx_dma_callback = NULL;
     furi_hal_serial[handle->id].context = context;
 
-    if(handle->id == FuriHalSerialIdUsart) {
     if(callback) {
+        if(handle->id == FuriHalSerialIdUsart) {
             furi_hal_serial_usart_deinit_dma_rx();
             furi_hal_interrupt_set_isr(
                 FuriHalInterruptIdUart1, furi_hal_serial_usart_irq_callback, NULL);
             LL_USART_EnableIT_RXNE_RXFNE(USART1);
-        } else {
-            furi_hal_interrupt_set_isr(FuriHalInterruptIdUart1, NULL, NULL);
-            furi_hal_serial_usart_deinit_dma_rx();
-            LL_USART_DisableIT_RXNE_RXFNE(USART1);
-        }
         } else if(handle->id == FuriHalSerialIdLpuart) {
-        if(callback) {
             furi_hal_serial_lpuart_deinit_dma_rx();
             furi_hal_interrupt_set_isr(
                 FuriHalInterruptIdLpUart1, furi_hal_serial_lpuart_irq_callback, NULL);
             LL_LPUART_EnableIT_RXNE_RXFNE(LPUART1);
-        } else {
-            furi_hal_interrupt_set_isr(FuriHalInterruptIdLpUart1, NULL, NULL);
-            furi_hal_serial_lpuart_deinit_dma_rx();
-            LL_LPUART_DisableIT_RXNE_RXFNE(LPUART1);
         }
     }
 }
@@ -944,33 +949,39 @@ static void furi_hal_serial_dma_configure(
     FuriHalSerialHandle* handle,
     FuriHalSerialDmaRxCallback callback,
     void* context) {
-    furi_check(handle);
+    // Disable RXFNE interrupts before unsetting the user callback that reads data
-
+    // Otherwise interrupt runs without reading data and without clearing RXFNE flag
+    // This would cause a system hang as the same interrupt runs in loop forever
+    if(!callback) {
         if(handle->id == FuriHalSerialIdUsart) {
-        if(callback) {
-            furi_hal_serial_usart_init_dma_rx();
-            furi_hal_interrupt_set_isr(
-                FuriHalInterruptIdUart1, furi_hal_serial_usart_irq_callback, NULL);
-        } else {
             LL_USART_DisableIT_RXNE_RXFNE(USART1);
             furi_hal_interrupt_set_isr(FuriHalInterruptIdUart1, NULL, NULL);
             furi_hal_serial_usart_deinit_dma_rx();
-        }
         } else if(handle->id == FuriHalSerialIdLpuart) {
-        if(callback) {
-            furi_hal_serial_lpuart_init_dma_rx();
-            furi_hal_interrupt_set_isr(
-                FuriHalInterruptIdLpUart1, furi_hal_serial_lpuart_irq_callback, NULL);
-        } else {
             LL_LPUART_DisableIT_RXNE_RXFNE(LPUART1);
             furi_hal_interrupt_set_isr(FuriHalInterruptIdLpUart1, NULL, NULL);
             furi_hal_serial_lpuart_deinit_dma_rx();
         }
     }
+
+    // Handle must be configured before enabling RX interrupt
+    // as it might be triggered right away on a misconfigured handle
     furi_hal_serial[handle->id].rx_byte_callback = NULL;
     furi_hal_serial[handle->id].handle = handle;
     furi_hal_serial[handle->id].rx_dma_callback = callback;
     furi_hal_serial[handle->id].context = context;
+
+    if(callback) {
+        if(handle->id == FuriHalSerialIdUsart) {
+            furi_hal_serial_usart_init_dma_rx();
+            furi_hal_interrupt_set_isr(
+                FuriHalInterruptIdUart1, furi_hal_serial_usart_irq_callback, NULL);
+        } else if(handle->id == FuriHalSerialIdLpuart) {
+            furi_hal_serial_lpuart_init_dma_rx();
+            furi_hal_interrupt_set_isr(
+                FuriHalInterruptIdLpUart1, furi_hal_serial_lpuart_irq_callback, NULL);
+        }
+    }
 }
 
 void furi_hal_serial_dma_rx_start(