[Silabs] Fix some uart cli hang (project-chip#35723)

jmartinez-silabs · andy31415 · restyled-commits · web-flow · commit 222338e79144 · 2024-09-24T19:28:47.000Z
* Add macros to abstact eusart api between series

* Fix for uart cpp with irq handling with series3. Tweak uart and shell task priority.

* [MATTER-4132]force implementation of _read and _write from newlib to use our UartConsole api when SILABS_LOG_OUT_UART is enabled

* remove segger_rtt_syscalls when uartlog is enabled

* Apply suggestions from code review

Co-authored-by: Andrei Litvin &lt;andy314@gmail.com&gt;

* Restyled by gn

---------

Co-authored-by: Andrei Litvin &lt;andy314@gmail.com&gt;
Co-authored-by: Restyled.io &lt;commits@restyled.io&gt;
diff --git a/examples/platform/silabs/syscalls_stubs.cpp b/examples/platform/silabs/syscalls_stubs.cpp
@@ -38,13 +38,31 @@ extern "C" {
 #include "uart.h"
 #endif
 
+int _open(char * path, int flags, ...);
 int _close(int file);
 int _fstat(int file, struct stat * st);
 int _isatty(int file);
 int _lseek(int file, int ptr, int dir);
 int _read(int file, char * ptr, int len);
 int _write(int file, const char * ptr, int len);
 
+/**************************************************************************
+ * @brief
+ *  Open a file.
+ *
+ * @param[in] file
+ *  File you want to open.
+ *
+ * @return
+ *  Returns -1 since there is not logic here to open file.
+ **************************************************************************/
+
+int __attribute__((weak)) _open(char * path, int flags, ...)
+{
+    /* Pretend like we always fail */
+    return -1;
+}
+
 /**************************************************************************
  * @brief
  *  Close a file.
@@ -170,17 +188,22 @@ int __attribute__((weak)) _lseek(int file, int ptr, int dir)
  * @return
  *  Number of characters that have been read.
  *************************************************************************/
-int __attribute__((weak)) _read(int file, char * ptr, int len)
+#if SILABS_LOG_OUT_UART
+int _read(int file, char * ptr, int len)
 {
     (void) file;
-#if SILABS_LOG_OUT_UART
     return uartConsoleRead(ptr, len);
+}
 #else
+int __attribute__((weak)) _read(int file, char * ptr, int len)
+{
+    (void) file;
     (void) ptr;
     (void) len;
-#endif
+
     return 0;
 }
+#endif // SILABS_LOG_OUT_UART
 
 /**************************************************************************
  * @brief
@@ -198,17 +221,20 @@ int __attribute__((weak)) _read(int file, char * ptr, int len)
  * @return
  *  Number of characters that have been written.
  **************************************************************************/
-int __attribute__((weak)) _write(int file, const char * ptr, int len)
+#if SILABS_LOG_OUT_UART
+int _write(int file, const char * ptr, int len)
 {
     (void) file;
-#if SILABS_LOG_OUT_UART
-    uartConsoleWrite(ptr, len);
+    return uartConsoleWrite(ptr, len);
+}
 #else
+int __attribute__((weak)) _write(int file, const char * ptr, int len)
+{
+    (void) file;
     (void) ptr;
-#endif
-
     return len;
 }
+#endif // SILABS_LOG_OUT_UART
 
 #ifdef __cplusplus
 }
diff --git a/examples/platform/silabs/uart.cpp b/examples/platform/silabs/uart.cpp
@@ -40,8 +40,12 @@ extern "C" {
 #include "rsi_rom_egpio.h"
 #include "sl_si91x_usart.h"
 #else // For EFR32
+#if (_SILICON_LABS_32B_SERIES < 3)
 #include "em_core.h"
 #include "em_usart.h"
+#else
+#include "sl_hal_eusart.h"
+#endif //_SILICON_LABS_32B_SERIES
 #include "uartdrv.h"
 #ifdef SL_BOARD_NAME
 #include "sl_board_control.h"
@@ -86,11 +90,34 @@ extern "C" {
 #define USART_IRQ HELPER2(SL_UARTDRV_EUSART_VCOM_PERIPHERAL_NO)
 #define USART_IRQHandler HELPER4(SL_UARTDRV_EUSART_VCOM_PERIPHERAL_NO)
 #define vcom_handle sl_uartdrv_eusart_vcom_handle
+
+#if (_SILICON_LABS_32B_SERIES < 3)
+#define EUSART_INT_ENABLE EUSART_IntEnable
+#define EUSART_INT_DISABLE EUSART_IntDisable
+#define EUSART_INT_CLEAR EUSART_IntClear
+#define EUSART_CLEAR_RX
+#define EUSART_GET_PENDING_INT EUSART_IntGet
+#define EUSART_ENABLE(eusart) EUSART_Enable(eusart, eusartEnable)
+#else
+#define EUSART_INT_ENABLE sl_hal_eusart_enable_interrupts
+#define EUSART_INT_DISABLE sl_hal_eusart_disable_interrupts
+#define EUSART_INT_SET sl_hal_eusart_set_interrupts
+#define EUSART_INT_CLEAR sl_hal_eusart_clear_interrupts
+#define EUSART_CLEAR_RX sl_hal_eusart_clear_rx
+#define EUSART_GET_PENDING_INT sl_hal_eusart_get_pending_interrupts
+#define EUSART_ENABLE(eusart)                                                                                                      \
+    {                                                                                                                              \
+        sl_hal_eusart_enable(eusart);                                                                                              \
+        sl_hal_eusart_enable_tx(eusart);                                                                                           \
+        sl_hal_eusart_enable_rx(eusart);                                                                                           \
+    }
+#endif //_SILICON_LABS_32B_SERIES
+
 #else
 #define USART_IRQ HELPER2(SL_UARTDRV_USART_VCOM_PERIPHERAL_NO)
 #define USART_IRQHandler HELPER4(SL_UARTDRV_USART_VCOM_PERIPHERAL_NO)
 #define vcom_handle sl_uartdrv_usart_vcom_handle
-#endif // EFR32MG24
+#endif // SL_CATALOG_UARTDRV_EUSART_PRESENT
 
 namespace {
 // In order to reduce the probability of data loss during the dmaFull callback handler we use
@@ -118,7 +145,11 @@ typedef struct
 #if SILABS_LOG_OUT_UART
 #define UART_MAX_QUEUE_SIZE 125
 #else
+#if (_SILICON_LABS_32B_SERIES < 3)
 #define UART_MAX_QUEUE_SIZE 25
+#else
+#define UART_MAX_QUEUE_SIZE 50
+#endif
 #endif
 
 #ifdef CHIP_CONFIG_LOG_MESSAGE_MAX_SIZE
@@ -138,13 +169,7 @@ constexpr osThreadAttr_t kUartTaskAttr = { .name       = "UART",
                                            .cb_size    = osThreadCbSize,
                                            .stack_mem  = uartStack,
                                            .stack_size = kUartTaskSize,
-#if SLI_SI91X_MCU_INTERFACE
-                                           // Reducing the priority of the UART task to avoid priority inversion
-                                           .priority = osPriorityNormal
-#else
-                                           .priority = osPriorityRealtime
-#endif // SLI_SI91X_MCU_INTERFACE
-};
+                                           .priority   = osPriorityBelowNormal };
 
 typedef struct
 {
@@ -309,16 +334,17 @@ void uartConsoleInit(void)
 
 #ifdef SL_CATALOG_UARTDRV_EUSART_PRESENT
     // Clear previous RX interrupts
-    EUSART_IntClear(SL_UARTDRV_EUSART_VCOM_PERIPHERAL, EUSART_IF_RXFL);
+    EUSART_INT_CLEAR(SL_UARTDRV_EUSART_VCOM_PERIPHERAL, (EUSART_IF_RXFL | EUSART_IF_RXOF));
+    EUSART_CLEAR_RX(SL_UARTDRV_EUSART_VCOM_PERIPHERAL);
 
     // Enable RX interrupts
-    EUSART_IntEnable(SL_UARTDRV_EUSART_VCOM_PERIPHERAL, EUSART_IF_RXFL);
+    EUSART_INT_ENABLE(SL_UARTDRV_EUSART_VCOM_PERIPHERAL, EUSART_IF_RXFL);
 
     // Enable EUSART
-    EUSART_Enable(SL_UARTDRV_EUSART_VCOM_PERIPHERAL, eusartEnable);
+    EUSART_ENABLE(SL_UARTDRV_EUSART_VCOM_PERIPHERAL);
 #else
     USART_IntEnable(SL_UARTDRV_USART_VCOM_PERIPHERAL, USART_IF_RXDATAV);
-#endif // EFR32MG24
+#endif // SL_CATALOG_UARTDRV_EUSART_PRESENT
 #endif // SLI_SI91X_MCU_INTERFACE == 0
 }
 
@@ -344,9 +370,15 @@ void USART_IRQHandler(void)
 #elif !defined(PW_RPC_ENABLED) && !defined(SL_WIFI)
     otSysEventSignalPending();
 #endif
-
 #ifdef SL_CATALOG_UARTDRV_EUSART_PRESENT
-    EUSART_IntClear(SL_UARTDRV_EUSART_VCOM_PERIPHERAL, EUSART_IF_RXFL);
+    // disable RXFL IRQ until data read by uartConsoleRead
+    EUSART_INT_DISABLE(SL_UARTDRV_EUSART_VCOM_PERIPHERAL, EUSART_IF_RXFL);
+    EUSART_INT_CLEAR(SL_UARTDRV_EUSART_VCOM_PERIPHERAL, EUSART_IF_RXFL);
+
+    if (EUSART_GET_PENDING_INT(SL_UARTDRV_EUSART_VCOM_PERIPHERAL) & EUSART_IF_RXOF)
+    {
+        EUSART_CLEAR_RX(SL_UARTDRV_EUSART_VCOM_PERIPHERAL);
+    }
 #endif
 }
 
@@ -418,7 +450,8 @@ int16_t uartConsoleWrite(const char * Buf, uint16_t BufLength)
     memcpy(workBuffer.data, Buf, BufLength);
     workBuffer.length = BufLength;
 
-    if (osMessageQueuePut(sUartTxQueue, &workBuffer, osPriorityNormal, 0) == osOK)
+    // this is usually a command response. Wait on queue if full.
+    if (osMessageQueuePut(sUartTxQueue, &workBuffer, osPriorityNormal, osWaitForever) == osOK)
     {
         return BufLength;
     }
@@ -445,6 +478,7 @@ int16_t uartLogWrite(const char * log, uint16_t length)
     memcpy(workBuffer.data + length, "\r\n", 2);
     workBuffer.length = length + 2;
 
+    // Don't wait when queue is full. Drop the log and return UART_CONSOLE_ERR
     if (osMessageQueuePut(sUartTxQueue, &workBuffer, osPriorityNormal, 0) == osOK)
     {
         return length;
@@ -462,6 +496,10 @@ int16_t uartConsoleRead(char * Buf, uint16_t NbBytesToRead)
 {
     uint8_t * data;
 
+#ifdef SL_CATALOG_UARTDRV_EUSART_PRESENT
+    EUSART_INT_ENABLE(SL_UARTDRV_EUSART_VCOM_PERIPHERAL, EUSART_IF_RXFL);
+#endif
+
     if (Buf == NULL || NbBytesToRead < 1)
     {
         return UART_CONSOLE_ERR;
diff --git a/src/lib/shell/streamer_silabs.cpp b/src/lib/shell/streamer_silabs.cpp
@@ -47,7 +47,13 @@ ssize_t streamer_efr_read(streamer_t * streamer, char * buffer, size_t length)
 ssize_t streamer_efr_write(streamer_t * streamer, const char * buffer, size_t length)
 {
     (void) streamer;
-    return uartConsoleWrite(buffer, (uint16_t) length);
+    int16_t bytesWritten = uartConsoleWrite(buffer, (uint16_t) length);
+    if (bytesWritten < 0) // The Write failed
+    {
+        bytesWritten = 0;
+    }
+
+    return bytesWritten;
 }
 
 static streamer_t streamer_efr = {
diff --git a/third_party/silabs/SiWx917_sdk.gni b/third_party/silabs/SiWx917_sdk.gni
@@ -773,8 +773,12 @@ template("siwx917_sdk") {
     public_deps = [
       "${segger_rtt_root}:segger_rtt",
       "${segger_rtt_root}:segger_rtt_printf",
-      "${segger_rtt_root}:segger_rtt_syscalls",
     ]
+
+    if (!sl_uart_log_output) {
+      public_deps += [ "${segger_rtt_root}:segger_rtt_syscalls" ]
+    }
+
     if (chip_crypto == "platform") {
       if (sl_si91x_crypto_flavor == "tinycrypt") {
         public_deps += [ ":siwx917_tinycrypt" ]
diff --git a/third_party/silabs/efr32_sdk.gni b/third_party/silabs/efr32_sdk.gni
@@ -1072,9 +1072,12 @@ template("efr32_sdk") {
       ":efr32_mbedtls_config",
       "${segger_rtt_root}:segger_rtt",
       "${segger_rtt_root}:segger_rtt_printf",
-      "${segger_rtt_root}:segger_rtt_syscalls",
     ]
 
+    if (!sl_uart_log_output) {
+      public_deps += [ "${segger_rtt_root}:segger_rtt_syscalls" ]
+    }
+
     if (defined(invoker.sources)) {
       sources += invoker.sources
     }

Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,13 @@ ssize_t streamer_efr_read(streamer_t * streamer, char * buffer, size_t length)`
`47`	`47`	`ssize_t streamer_efr_write(streamer_t * streamer, const char * buffer, size_t length)`
`48`	`48`	`{`
`49`	`49`	`(void) streamer;`
`50`		`- return uartConsoleWrite(buffer, (uint16_t) length);`
	`50`	`+ int16_t bytesWritten = uartConsoleWrite(buffer, (uint16_t) length);`
	`51`	`+ if (bytesWritten < 0) // The Write failed`
	`52`	`+ {`
	`53`	`+ bytesWritten = 0;`
	`54`	`+ }`
	`55`	`+`
	`56`	`+ return bytesWritten;`
`51`	`57`	`}`
`52`	`58`
`53`	`59`	`static streamer_t streamer_efr = {`
Original file line number	Diff line number	Diff line change
`@@ -1072,9 +1072,12 @@ template("efr32_sdk") {`
`1072`	`1072`	`":efr32_mbedtls_config",`
`1073`	`1073`	`"${segger_rtt_root}:segger_rtt",`
`1074`	`1074`	`"${segger_rtt_root}:segger_rtt_printf",`
`1075`		`- "${segger_rtt_root}:segger_rtt_syscalls",`
`1076`	`1075`	`]`
`1077`	`1076`
	`1077`	`+ if (!sl_uart_log_output) {`
	`1078`	`+ public_deps += [ "${segger_rtt_root}:segger_rtt_syscalls" ]`
	`1079`	`+ }`
	`1080`	`+`
`1078`	`1081`	`if (defined(invoker.sources)) {`
`1079`	`1082`	`sources += invoker.sources`
`1080`	`1083`	`}`