feat(serial): Funnel writes through a stream-buffer

Previously, race-conditions between multiple serial writes could lead to
memory corruption and weird artifacts like I2C getting stuck.  This
patch introduces a write-streambuffer which queues all messages.

Co-authored-by: Rahix <rahix@rahix.de>
Signed-off-by: danukeru <danukeru@foulab.org>

epicardium/main.c

@@ -24,7 +24,7 @@ int main(void)
 	 * Version Splash
 	 */
 	const char *version_buf = CARD10_VERSION;
-	const int offset = (160 - (int)strlen(version_buf) * 14) / 2;
+	const int offset        = (160 - (int)strlen(version_buf) * 14) / 2;
 	epic_disp_clear(0x3b7);
 	epic_disp_print(10, 20, "Epicardium", 0x290, 0x3b7);
 	epic_disp_print(offset > 0 ? offset : 0, 40, version_buf, 0x290, 0x3b7);
@@ -39,7 +39,7 @@ int main(void)
 		    (const char *)"Serial",
 		    configMINIMAL_STACK_SIZE * 2,
 		    NULL,
-		    tskIDLE_PRIORITY + 1,
+		    tskIDLE_PRIORITY + 3,
 		    NULL) != pdPASS) {
 		LOG_CRIT("startup", "Failed to create %s task!", "Serial");
 		abort();
@@ -134,6 +134,11 @@ int main(void)
 		abort();
 	}
 
+	/*
+	 * Initialize serial driver data structures.
+	 */
+	serial_init();
+
 	LOG_DEBUG("startup", "Starting FreeRTOS ...");
 	vTaskStartScheduler();
 

epicardium/modules/modules.h

@@ -24,10 +24,18 @@ void return_to_menu(void);
 
 /* ---------- Serial ------------------------------------------------------- */
 #define SERIAL_READ_BUFFER_SIZE 128
+#define SERIAL_WRITE_STREAM_BUFFER_SIZE 512
+void serial_init();
 void vSerialTask(void *pvParameters);
 void serial_enqueue_char(char chr);
 extern TaskHandle_t serial_task_id;
 
+// For the eSetBit xTaskNotify task semaphore trigger
+enum serial_notify{
+      SERIAL_WRITE_NOTIFY = 0x01,
+      SERIAL_READ_NOTIFY  = 0x02,
+};
+
 /* ---------- LED Animation / Personal States ------------------------------ */
 #define PERSONAL_STATE_LED 14
 void vLedTask(void *pvParameters);

epicardium/modules/serial.c

@@ -10,40 +10,115 @@
 #include "FreeRTOS.h"
 #include "task.h"
 #include "queue.h"
+#include "stream_buffer.h"
 
 #include <stdint.h>
 #include <stdio.h>
 
+/* The serial console in use (UART0) */
+extern mxc_uart_regs_t *ConsoleUart;
+
 /* Task ID for the serial handler */
 TaskHandle_t serial_task_id = NULL;
 
-/* The serial console in use (UART0) */
-extern mxc_uart_regs_t *ConsoleUart;
 /* Read queue, filled by both UART and CDCACM */
 static QueueHandle_t read_queue;
+/* Stream Buffer for handling all writes to serial */
+static StreamBufferHandle_t write_stream_buffer = NULL;
+
+void serial_init()
+{
+	/* Setup read queue */
+	static uint8_t buffer[sizeof(char) * SERIAL_READ_BUFFER_SIZE];
+	static StaticQueue_t read_queue_data;
+	read_queue = xQueueCreateStatic(
+		SERIAL_READ_BUFFER_SIZE, sizeof(char), buffer, &read_queue_data
+	);
+
+	/* Setup write queue */
+	static uint8_t ucWrite_stream_buffer[SERIAL_WRITE_STREAM_BUFFER_SIZE];
+	static StaticStreamBuffer_t xStream_buffer_struct;
+	write_stream_buffer = xStreamBufferCreateStatic(
+		sizeof(ucWrite_stream_buffer),
+		1,
+		ucWrite_stream_buffer,
+		&xStream_buffer_struct
+	);
+}
 
 /*
  * API-call to write a string.  Output goes to both CDCACM and UART
  */
 void epic_uart_write_str(const char *str, intptr_t length)
 {
-	uint32_t basepri = __get_BASEPRI();
-	if (xPortIsInsideInterrupt()) {
-		taskENTER_CRITICAL_FROM_ISR();
-	} else {
-		taskENTER_CRITICAL();
+	if (length == 0) {
+		return;
 	}
 
-	UART_Write(ConsoleUart, (uint8_t *)str, length);
+	/*
+	 * Check if the stream buffer is even initialized yet
+	 */
+	if (write_stream_buffer == NULL) {
+		UART_Write(ConsoleUart, (uint8_t *)str, length);
+		cdcacm_write((uint8_t *)str, length);
+		return;
+	}
 
 	if (xPortIsInsideInterrupt()) {
+		BaseType_t resched1 = pdFALSE;
+		BaseType_t resched2 = pdFALSE;
+
+		/*
+		 * Enter a critial section so no other task can write to the
+		 * stream buffer.
+		 */
+		uint32_t basepri = __get_BASEPRI();
+		taskENTER_CRITICAL_FROM_ISR();
+
+		xStreamBufferSendFromISR(
+			write_stream_buffer, str, length, &resched1
+		);
+
 		taskEXIT_CRITICAL_FROM_ISR(basepri);
+
+		if (serial_task_id != NULL) {
+			xTaskNotifyFromISR(
+				serial_task_id,
+				SERIAL_WRITE_NOTIFY,
+				eSetBits,
+				&resched2
+			);
+		}
+
+		/* Yield if this write woke up a higher priority task */
+		portYIELD_FROM_ISR(resched1 || resched2);
 	} else {
-		taskEXIT_CRITICAL();
+		size_t bytes_sent = 0;
+		size_t index      = 0;
+		do {
+			taskENTER_CRITICAL();
+			/*
+			 * Wait time needs to be zero, because we are in a
+			 * critical section.
+			 */
+			bytes_sent = xStreamBufferSend(
+				write_stream_buffer,
+				&str[index],
+				length - index,
+				0
+			);
+			index += bytes_sent;
+			taskEXIT_CRITICAL();
+			if (serial_task_id != NULL) {
+				xTaskNotify(
+					serial_task_id,
+					SERIAL_WRITE_NOTIFY,
+					eSetBits
+				);
+				portYIELD();
+			}
+		} while (index < length);
 	}
-
-	cdcacm_write((uint8_t *)str, length);
-	ble_uart_write((uint8_t *)str, length);
 }
 
 /*
@@ -101,7 +176,12 @@ void UART0_IRQHandler(void)
 static void uart_callback(uart_req_t *req, int error)
 {
 	BaseType_t xHigherPriorityTaskWoken = pdFALSE;
-	vTaskNotifyGiveFromISR(serial_task_id, &xHigherPriorityTaskWoken);
+	xTaskNotifyFromISR(
+		serial_task_id,
+		SERIAL_READ_NOTIFY,
+		eSetBits,
+		&xHigherPriorityTaskWoken
+	);
 	portYIELD_FROM_ISR(xHigherPriorityTaskWoken);
 }
 
@@ -122,16 +202,8 @@ void serial_enqueue_char(char chr)
 
 void vSerialTask(void *pvParameters)
 {
-	static uint8_t buffer[sizeof(char) * SERIAL_READ_BUFFER_SIZE];
-	static StaticQueue_t read_queue_data;
-
 	serial_task_id = xTaskGetCurrentTaskHandle();
 
-	/* Setup read queue */
-	read_queue = xQueueCreateStatic(
-		SERIAL_READ_BUFFER_SIZE, sizeof(char), buffer, &read_queue_data
-	);
-
 	/* Setup UART interrupt */
 	NVIC_ClearPendingIRQ(UART0_IRQn);
 	NVIC_DisableIRQ(UART0_IRQn);
@@ -145,6 +217,9 @@ void vSerialTask(void *pvParameters)
 		.callback = uart_callback,
 	};
 
+	uint8_t rx_data[20];
+	size_t received_bytes;
+
 	while (1) {
 		int ret = UART_ReadAsync(ConsoleUart, &read_req);
 		if (ret != E_NO_ERROR && ret != E_BUSY) {
@@ -152,18 +227,55 @@ void vSerialTask(void *pvParameters)
 			vTaskDelay(portMAX_DELAY);
 		}
 
-		ulTaskNotifyTake(pdTRUE, portTICK_PERIOD_MS * 1000);
+		ret = ulTaskNotifyTake(pdTRUE, portMAX_DELAY);
 
-		if (read_req.num > 0) {
-			serial_enqueue_char(*read_req.data);
-		}
+		if (ret & SERIAL_WRITE_NOTIFY) {
+			do {
+				received_bytes = xStreamBufferReceive(
+					write_stream_buffer,
+					(void *)rx_data,
+					sizeof(rx_data),
+					0
+				);
+
+				if (received_bytes == 0) {
+					break;
+				}
+
+				/*
+				 * The SDK-driver for UART is not reentrant
+				 * which means we need to perform UART writes
+				 * in a critical section.
+				 */
+				taskENTER_CRITICAL();
+				UART_Write(
+					ConsoleUart,
+					(uint8_t *)&rx_data,
+					received_bytes
+				);
+				taskEXIT_CRITICAL();
 
-		while (UART_NumReadAvail(ConsoleUart) > 0) {
-			serial_enqueue_char(UART_ReadByte(ConsoleUart));
+				cdcacm_write(
+					(uint8_t *)&rx_data, received_bytes
+				);
+				ble_uart_write(
+					(uint8_t *)&rx_data, received_bytes
+				);
+			} while (received_bytes > 0);
 		}
 
-		while (cdcacm_num_read_avail() > 0) {
-			serial_enqueue_char(cdcacm_read());
+		if (ret & SERIAL_READ_NOTIFY) {
+			if (read_req.num > 0) {
+				serial_enqueue_char(*read_req.data);
+			}
+
+			while (UART_NumReadAvail(ConsoleUart) > 0) {
+				serial_enqueue_char(UART_ReadByte(ConsoleUart));
+			}
+
+			while (cdcacm_num_read_avail() > 0) {
+				serial_enqueue_char(cdcacm_read());
+			}
 		}
 	}
 }

epicardium/usb/cdcacm.c

@@ -17,6 +17,7 @@
 #include "cdc_acm.h"
 
 #include "modules/log.h"
+#include "modules/modules.h"
 
 #include "FreeRTOS.h"
 #include "task.h"
@@ -118,8 +119,11 @@ void USB_IRQHandler(void)
 
 	if (serial_task_id != NULL) {
 		BaseType_t xHigherPriorityTaskWoken = pdFALSE;
-		vTaskNotifyGiveFromISR(
-			serial_task_id, &xHigherPriorityTaskWoken
+		xTaskNotifyFromISR(
+			serial_task_id,
+			SERIAL_READ_NOTIFY,
+			eSetBits,
+			&xHigherPriorityTaskWoken
 		);
 		portYIELD_FROM_ISR(xHigherPriorityTaskWoken);
 	}