Dedicated code path for USB console/TS (#2465)

* new apis

* dedicated USB path

* move decl

* init

* don't init usb twice

* guard

* check that we're initialized before using TS

* comment

* guard

* guard USB

* brain doesn't go good

* make tests happy

* dead define

* cypress

* ifdef != if

* s

* include priority

* don't start a thread we know we don't need

* bad-ish merge

* const

* why did this code exist...?

Co-authored-by: Matthew Kennedy <makenne@microsoft.com>

firmware/config/boards/hellen/cypress/efifeatures.h

@@ -290,9 +290,6 @@
 #undef TS_UART_DEVICE
 #undef TS_SERIAL_DEVICE
 
-#define TS_USB_DEVICE SDU1
-//#undef TS_USB_DEVICE
-
 // todo: add CAN support
 //#define TS_CAN_DEVICE CAND1
 #define TS_CAN_AF PAL_MODE_ALTERNATIVE_CAN
@@ -303,8 +300,8 @@
 
 #define EFI_USB_SERIAL TRUE
 #define EFI_CONSOLE_USB_DEVICE SDU1
-
-#define SERIAL_USB_DRIVER BaseChannel
+// Cypress uses a fake USB device that's just a plain channel
+#define SerialUSBDriver BaseChannel
 
 #define EFI_CONSOLE_TX_PORT GPIOA
 #define EFI_CONSOLE_TX_PIN 10
@@ -402,7 +399,7 @@
 #define debugLog(fmt,...) { \
 	extern int __debugEnabled; \
 	if (__debugEnabled) { \
-		extern SERIAL_USB_DRIVER EFI_CONSOLE_USB_DEVICE; \
+		extern SerialUSBDriver EFI_CONSOLE_USB_DEVICE; \
 		extern char __debugBuffer[200]; \
 		chsnprintf(__debugBuffer, sizeof(__debugBuffer), fmt, ##__VA_ARGS__); \
 		chnWriteTimeout(&EFI_CONSOLE_USB_DEVICE, (const uint8_t *)__debugBuffer, strlen(__debugBuffer), TIME_MS2I(1000)); \

firmware/config/boards/subaru_eg33/mcuconf.h

@@ -31,12 +31,9 @@
 #define STM32_UART_USE_USART1			FALSE
 //#endif
 
-/* enable serial driver for USART2 to make usbconsole.c happy:
- * as '#define USB_SERIAL_DRIVER SD2' in usbconsole.h and
- * sdStart(&USB_SERIAL_DRIVER, NULL); is called unconditionaly */
 #undef STM32_SERIAL_USE_USART2
 #undef STM32_UART_USE_USART2
-#define STM32_SERIAL_USE_USART2			TRUE
+#define STM32_SERIAL_USE_USART2			FALSE
 #define STM32_UART_USE_USART2			FALSE
 
 #undef STM32_SERIAL_USE_USART3

firmware/console/binary/tunerstudio.cpp

@@ -120,8 +120,11 @@ static efitimems_t previousWriteReportMs = 0;
 
 static ts_channel_s tsChannel;
 
+// TODO: simplify what happens when we have multiple serial ports
+#if !EFI_USB_SERIAL
 // this thread wants a bit extra stack
 static THD_WORKING_AREA(tunerstudioThreadStack, CONNECTIVITY_THREAD_STACK);
+#endif
 
 static void resetTs(void) {
 	memset(&tsState, 0, sizeof(tsState));
@@ -572,12 +575,24 @@ static void tsProcessOne(TsChannelBase* tsChannel) {
 }
 
 void runBinaryProtocolLoop(TsChannelBase* tsChannel) {
+	// No channel configured for this thread, cancel.
+	if (!tsChannel || !tsChannel->isConfigured()) {
+		return;
+	}
+
 	// Until the end of time, process incoming messages.
 	while(true) {
 		tsProcessOne(tsChannel);
 	}
 }
 
+void TunerstudioThread::ThreadTask() {
+	auto channel = setupChannel();
+
+	runBinaryProtocolLoop(channel);
+}
+
+#if !EFI_USB_SERIAL
 static THD_FUNCTION(tsThreadEntryPoint, arg) {
 	(void) arg;
 	chRegSetThreadName("tunerstudio thread");
@@ -586,6 +601,7 @@ static THD_FUNCTION(tsThreadEntryPoint, arg) {
 
 	runBinaryProtocolLoop(&tsChannel);
 }
+#endif
 
 /**
  * Copy real configuration into the communications layer working copy
@@ -912,7 +928,10 @@ void startTunerStudioConnectivity(void) {
 	addConsoleAction("bluetooth_cancel", bluetoothCancel);
 #endif /* EFI_BLUETOOTH_SETUP */
 
+// TODO: simplify what happens when we have multiple serial ports
+#if !EFI_USB_SERIAL
 	chThdCreateStatic(tunerstudioThreadStack, sizeof(tunerstudioThreadStack), PRIO_CONSOLE, (tfunc_t)tsThreadEntryPoint, NULL);
+#endif
 }
 
 #endif

firmware/console/binary/tunerstudio.h

@@ -11,6 +11,8 @@
 
 #if EFI_TUNER_STUDIO
 #include "tunerstudio_outputs.h"
+#include "thread_controller.h"
+#include "thread_priority.h"
 
 typedef struct {
 	int queryCommandCounter;
@@ -71,4 +73,17 @@ post_packed {
 	short int count;
 } TunerStudioWriteChunkRequest;
 
+class TunerstudioThread : public ThreadController<CONNECTIVITY_THREAD_STACK> {
+public:
+	TunerstudioThread(const char* name)
+		: ThreadController(name, PRIO_CONSOLE)
+	{
+	}
+
+	// Initialize and return the channel to use for this thread.
+	virtual TsChannelBase* setupChannel() = 0;
+
+	void ThreadTask() override;
+};
+
 #endif /* EFI_TUNER_STUDIO */

firmware/console/binary/tunerstudio_io.cpp

@@ -22,17 +22,6 @@ extern LoggingWithStorage tsLogger;
 #if EFI_PROD_CODE
 #include "pin_repository.h"
 
-#if HAL_USE_SERIAL_USB
-// Assert that the USB tx/rx buffers are large enough to fit one full packet
-static_assert(SERIAL_USB_BUFFERS_SIZE >= BLOCKING_FACTOR + 10);
-#define SERIAL_USB_DRIVER SerialUSBDriver
-#define TS_USB_DEVICE EFI_CONSOLE_USB_DEVICE // SDU1
-#endif /* HAL_USE_SERIAL_USB */
-
-#ifdef TS_USB_DEVICE
-extern SERIAL_USB_DRIVER TS_USB_DEVICE;
-#endif /* TS_USB_DEVICE */
-
 #if TS_UART_DMA_MODE
 #elif TS_UART_MODE
 /* Note: This structure is modified from the default ChibiOS layout! */
@@ -56,23 +45,8 @@ static SerialConfig tsSerialConfig = { .speed = 0, .cr1 = 0, .cr2 = USART_CR2_ST
 
 
 void startTsPort(ts_channel_s *tsChannel) {
-
 	#if EFI_PROD_CODE
 	tsChannel->channel = (BaseChannel *) NULL;
-		#if defined(TS_USB_DEVICE)
-#if defined(TS_UART_DEVICE)
-#error 	"cannot have TS_UART_DEVICE and TS_USB_DEVICE"
-#endif
-			print("TunerStudio over USB serial");
-			/**
-			 * This method contains a long delay, that's the reason why this is not done on the main thread
-			 * TODO: actually now with some refactoring this IS on the main thread :(
-			 */
-			usb_serial_start();
-			// if console uses UART then TS uses USB
-			tsChannel->channel = (BaseChannel *) &TS_USB_DEVICE;
-			return;
-		#endif /* TS_USB_DEVICE */
 		#if defined(TS_UART_DEVICE) || defined(TS_SERIAL_DEVICE)
 			if (CONFIG(useSerialPort)) {
 
@@ -296,7 +270,15 @@ void TsChannelBase::sendResponse(ts_response_format_e mode, const uint8_t * buff
 	}
 }
 
-bool ts_channel_s::isReady() {
+bool ts_channel_s::isConfigured() const {
+	return
+#if TS_UART_DMA_MODE || PRIMARY_UART_DMA_MODE || TS_UART_MODE
+		this->uartp ||
+#endif
+		this->channel;
+}
+
+bool ts_channel_s::isReady() const {
 #if EFI_USB_SERIAL
 	if (isUsbSerial(this->channel)) {
 		// TS uses USB when console uses serial
@@ -305,3 +287,27 @@ bool ts_channel_s::isReady() {
 #endif /* EFI_USB_SERIAL */
 	return true;
 }
+
+#if EFI_PROD_CODE || EFI_SIMULATOR
+void BaseChannelTsChannel::write(const uint8_t* buffer, size_t size) {
+	chnWriteTimeout(m_channel, buffer, size, BINARY_IO_TIMEOUT);
+}
+
+size_t BaseChannelTsChannel::readTimeout(uint8_t* buffer, size_t size, int timeout) {
+	return chnReadTimeout(m_channel, buffer, size, timeout);
+}
+
+void BaseChannelTsChannel::flush() {
+	// nop for this channel, writes automatically flush
+}
+
+bool BaseChannelTsChannel::isReady() const {
+#if EFI_USB_SERIAL
+if (isUsbSerial(m_channel)) {
+		// TS uses USB when console uses serial
+		return is_usb_serial_ready();
+	}
+#endif /* EFI_USB_SERIAL */
+	return true;
+}
+#endif // EFI_PROD_CODE || EFI_SIMULATOR

firmware/console/binary/tunerstudio_io.h

@@ -30,7 +30,8 @@ public:
 
 	// These functions are optional to implement, not all channels need them
 	virtual void flush() { }
-	virtual bool isReady() { return true; }
+	virtual bool isConfigured() const { return true; }
+	virtual bool isReady() const { return true; }
 	virtual void stop() { }
 
 	// Base functions that use the above virtual implementation
@@ -54,20 +55,36 @@ private:
 	void writeCrcPacketLarge(uint8_t responseCode, const uint8_t* buf, size_t size);
 };
 
+#if EFI_UNIT_TEST
+struct BaseChannel;
+#endif
+
 struct ts_channel_s : public TsChannelBase {
 	void write(const uint8_t* buffer, size_t size) override;
 	size_t readTimeout(uint8_t* buffer, size_t size, int timeout) override;
-	bool isReady() override;
+	bool isReady() const override;
+	bool isConfigured() const override;
 
-#if !EFI_UNIT_TEST
 	BaseChannel * channel = nullptr;
-#endif
 
 #if TS_UART_DMA_MODE || PRIMARY_UART_DMA_MODE || TS_UART_MODE
 	UARTDriver *uartp = nullptr;
 #endif // TS_UART_DMA_MODE
 };
 
+class BaseChannelTsChannel : public TsChannelBase {
+public:
+	BaseChannelTsChannel(BaseChannel* channel) : m_channel(channel) { }
+
+	void write(const uint8_t* buffer, size_t size) override;
+	size_t readTimeout(uint8_t* buffer, size_t size, int timeout) override;
+	void flush() override;
+	bool isReady() const override;
+
+private:
+	BaseChannel* const m_channel;
+};
+
 #define CRC_VALUE_SIZE 4
 // todo: double-check this
 #define CRC_WRAPPING_SIZE (CRC_VALUE_SIZE + 3)

firmware/console/console.mk

@@ -8,6 +8,7 @@ CONSOLE_SRC_CPP = $(PROJECT_DIR)/console/status_loop.cpp \
 	$(PROJECT_DIR)/console/binary/tooth_logger.cpp \
 	$(PROJECT_DIR)/console/binary_log/log_field.cpp \
 	$(PROJECT_DIR)/console/binary_log/binary_logging.cpp \
+	$(PROJECT_DIR)/console/binary_log/usb_console.cpp \
 
 
 CONSOLE_INC=\

firmware/console/console_io.cpp

@@ -47,18 +47,6 @@
 
 EXTERN_ENGINE;
 
-#if HAL_USE_SERIAL_USB
-#include "usbcfg.h"
-#include "usbconsole.h"
-#define EFI_CONSOLE_USB_DEVICE SDU1
-#define SERIAL_USB_DRIVER SerialUSBDriver
-
-#ifdef EFI_CONSOLE_USB_DEVICE
-extern SERIAL_USB_DRIVER EFI_CONSOLE_USB_DEVICE;
-#endif /* EFI_CONSOLE_USB_DEVICE */
-
-#endif /* HAL_USE_SERIAL_USB */
-
 // 10 seconds
 #define CONSOLE_WRITE_TIMEOUT 10000
 
@@ -172,6 +160,10 @@ ts_channel_s primaryChannel;
 
 #if EFI_PROD_CODE || EFI_EGT
 
+#if HAL_USE_SERIAL_USB
+extern SerialUSBDriver EFI_CONSOLE_USB_DEVICE;
+#endif /* HAL_USE_SERIAL_USB */
+
 bool isUsbSerial(BaseChannel * channel) {
 #if HAL_USE_SERIAL_USB
 	return channel == (BaseChannel *) &EFI_CONSOLE_USB_DEVICE;
@@ -195,11 +187,7 @@ BaseChannel * getConsoleChannel(void) {
 	return (BaseChannel *) &uartChannel;
 #endif /* EFI_CONSOLE_UART_DEVICE */
 
-#if HAL_USE_SERIAL_USB
-	return (BaseChannel *) &EFI_CONSOLE_USB_DEVICE;
-#else
 	return nullptr;
-#endif /* HAL_USE_SERIAL_USB */
 }
 
 bool isCommandLineConsoleReady(void) {

firmware/console/eficonsole.h

@@ -11,4 +11,4 @@
 
 void initializeConsole(Logging *sharedLogger);
 void print(const char *fmt, ...);
-
+void startUsbConsole();

firmware/console/usb_console.cpp

@@ -0,0 +1,33 @@
+#include "global.h"
+
+#if EFI_USB_SERIAL
+
+#include "usbconsole.h"
+#include "thread_controller.h"
+#include "tunerstudio.h"
+
+
+// Assert that the USB tx/rx buffers are large enough to fit one full packet
+static_assert(SERIAL_USB_BUFFERS_SIZE >= BLOCKING_FACTOR + 10);
+
+extern SerialUSBDriver EFI_CONSOLE_USB_DEVICE;
+static BaseChannelTsChannel usbChannel((BaseChannel*)&EFI_CONSOLE_USB_DEVICE);
+
+struct UsbThread : public TunerstudioThread {
+	UsbThread() : TunerstudioThread("USB Console") { }
+
+	TsChannelBase* setupChannel() override {
+		// Start the port's USB stack
+		usb_serial_start();
+
+		return &usbChannel;
+	}
+};
+
+static UsbThread usbConsole;
+
+void startUsbConsole() {
+	usbConsole.Start();
+}
+
+#endif // EFI_USB_SERIAL

firmware/hw_layer/ports/stm32/serial_over_usb/usbconsole.c

@@ -44,13 +44,6 @@ void usb_serial_start(void) {
 #endif/* EFI_SKIP_USB_DISCONNECT */
 	usbStart(serusbcfg.usbp, &usbcfg);
 	usbConnectBus(serusbcfg.usbp);
-	
-#if HAL_USE_SERIAL
-	/*
-	 * Activates the serial driver using the driver default configuration.
-	 */
-	sdStart(&USB_SERIAL_DRIVER, NULL);
-#endif /* HAL_USE_SERIAL */
 
 	isUsbSerialInitialized = true;
 }

firmware/hw_layer/ports/stm32/serial_over_usb/usbconsole.h

@@ -7,8 +7,6 @@
 
 #pragma once
 
-#define USB_SERIAL_DRIVER SD2
-
 #ifdef __cplusplus
 extern "C"
 {

firmware/rusefi.cpp

@@ -214,6 +214,14 @@ void runRusEfi(void) {
 	prepareVoidConfiguration(&activeConfiguration);
 #endif /* EFI_ACTIVE_CONFIGURATION_IN_FLASH */
 
+#if EFI_FILE_LOGGING
+	initMmcCard();
+#endif /* EFI_FILE_LOGGING */
+
+#if EFI_USB_SERIAL
+	startUsbConsole();
+#endif
+
 	/**
 	 * Next we should initialize serial port console, it's important to know what's going on
 	 */
@@ -234,10 +242,6 @@ void runRusEfi(void) {
 	enableTriggerStimulator();
 #endif // HW_CHECK_ALWAYS_STIMULATE
 
-#if EFI_FILE_LOGGING
-	initMmcCard();
-#endif /* EFI_FILE_LOGGING */
-
 	// Config could be completely bogus - don't start anything else!
 	if (validateConfig()) {
 		initStatusLoop();