more graceful chibios assertion failure (#2859)

* no custom assert hook

* dead

* setjmp/longjmp

* always call tid hook

* tests get threadid

* simulator threadid

* kick

* stubs for kinetis

* make it happier

* noreturn

* oops

* comments

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

firmware/config/boards/kinetis/board_configuration.cpp

@@ -123,3 +123,15 @@ void setAdcChannelOverrides(void) {
 	removeChannel("TPS", engineConfiguration->tps1_1AdcChannel);
 	addChannel("TPS", engineConfiguration->tps1_1AdcChannel, ADC_SLOW);
 }
+
+#include <setjmp.h>
+
+void longjmp(jmp_buf /*env*/, int /*status*/) {
+	// noop, but noreturn
+	while (1) { }
+}
+
+int setjmp(jmp_buf /*env*/) {
+	// Fake return 0, not implemented
+	return 0;
+}

firmware/controllers/core/error_handling.cpp

@@ -19,7 +19,6 @@ static critical_msg_t criticalErrorMessageBuffer;
 EXTERN_ENGINE;
 
 extern int warningEnabled;
-extern bool main_loop_started;
 
 bool hasFirmwareErrorFlag = false;
 
@@ -71,6 +70,23 @@ void chDbgPanic3(const char *msg, const char * file, int line) {
 #if EFI_HD44780_LCD
 	lcdShowPanicMessage((char *) msg);
 #endif /* EFI_HD44780_LCD */
+
+	firmwareError(OBD_PCM_Processor_Fault, "assert fail %s %s:%d", msg, file, line);
+
+	// Force unlock, since we may be throwing-under-lock
+	chSysUnconditionalUnlock();
+
+	// there was a port_disable in chSysHalt, reenable interrupts so USB works
+	port_enable();
+
+	// If on the main thread, longjmp back to the init process so we can keep USB alive
+	if (chThdGetSelfX()->threadId == 0) {
+		void onAssertionFailure();
+		onAssertionFailure();
+	} else {
+		// Not the main thread, simply try to terminate ourselves and let other threads continue living (so the user can diagnose, etc)
+		chThdTerminate(chThdGetSelfX());
+	}
 }
 
 #else

firmware/controllers/engine_controller.cpp

@@ -651,10 +651,6 @@ void initEngineContoller(DECLARE_ENGINE_PARAMETER_SUFFIX) {
 	initCJ125(PASS_ENGINE_PARAMETER_SIGNATURE);
 #endif /* EFI_CJ125 */
 
-
-	// periodic events need to be initialized after fuel&spark pins to avoid a warning
-	initPeriodicEvents(PASS_ENGINE_PARAMETER_SIGNATURE);
-
 	if (hasFirmwareError()) {
 		return;
 	}

firmware/controllers/engine_controller_misc.cpp

@@ -14,7 +14,6 @@ EXTERN_ENGINE;
 
 extern ButtonDebounce startStopButtonDebounce;
 
-#if ENABLE_PERF_TRACE
 static uint8_t nextThreadId = 0;
 void threadInitHook(void* vtp) {
 	// No lock required, this is already under lock
@@ -22,6 +21,7 @@ void threadInitHook(void* vtp) {
 	tp->threadId = ++nextThreadId;
 }
 
+#if ENABLE_PERF_TRACE
 void irqEnterHook() {
 	perfEventBegin(PE::ISR);
 }
@@ -35,7 +35,6 @@ void contextSwitchHook() {
 }
 
 #else
-void threadInitHook(void*) {}
 void irqEnterHook() {}
 void irqExitHook() {}
 void contextSwitchHook() {}

firmware/egt2can.cpp

@@ -6,8 +6,6 @@
 
 egt_cs_array_t max31855_cs;
 
-int main_loop_started;
-
 void firmwareError(const char *fmt, ...) {
 
 }

firmware/hw_layer/ports/chconf_common.h

@@ -116,16 +116,6 @@ extern "C" {
 #define hasOsPanicError() (FALSE)
 #endif
 
-#define chDbgAssert(c, remark) do {                                         \
-  if (CH_DBG_ENABLE_ASSERTS != FALSE) {                                     \
-    if (!(c)) {                                                             \
-  /*lint -restore*/                                                         \
-	  firmwareError(OBD_PCM_Processor_Fault, "chDbg %s", remark);           \
-      chSysHalt(remark);                                                    \
-    }                                                                       \
-  }                                                                         \
-} while (false)
-
 #ifndef __ASSEMBLER__
  #ifdef __cplusplus
   extern "C"

firmware/rusefi.cpp

@@ -129,6 +129,8 @@
 #include "trigger_emulator_algo.h"
 #include "rusefi_lua.h"
 
+#include <setjmp.h>
+
 #if EFI_ENGINE_EMULATOR
 #include "engine_emulator.h"
 #endif /* EFI_ENGINE_EMULATOR */
@@ -227,6 +229,16 @@ static bool validateConfig() {
 	return true;
 }
 
+static jmp_buf jmpEnv;
+void onAssertionFailure() {
+	// There's been an assertion failure: instead of hanging, jump back to where we check
+	// if (setjmp(jmpEnv)) (see below for more complete explanation)
+	longjmp(jmpEnv, 1);
+}
+
+void runRusEfiWithConfig();
+void runMainLoop();
+
 void runRusEfi(void) {
 	efiAssertVoid(CUSTOM_RM_STACK_1, getCurrentRemainingStack() > 512, "init s");
 	assertEngineReference();
@@ -248,9 +260,6 @@ void runRusEfi(void) {
 	// Perform hardware initialization that doesn't need configuration
 	initHardwareNoConfig();
 
-	// Read configuration from flash memory
-	loadConfiguration(PASS_ENGINE_PARAMETER_SIGNATURE);
-
 #if EFI_USB_SERIAL
 	startUsbConsole();
 #endif
@@ -264,6 +273,9 @@ void runRusEfi(void) {
 	 */
 	initializeConsole();
 
+	// Read configuration from flash memory
+	loadConfiguration(PASS_ENGINE_PARAMETER_SIGNATURE);
+
 #if EFI_TUNER_STUDIO
 	startTunerStudioConnectivity();
 #endif /* EFI_TUNER_STUDIO */
@@ -271,11 +283,28 @@ void runRusEfi(void) {
 	// Start hardware serial ports (including bluetooth, if present)
 	startSerialChannels();
 
+	runRusEfiWithConfig();
+
+	runMainLoop();
+}
+
+void runRusEfiWithConfig() {
+	// If some config operation caused an OS assertion failure, return immediately
+	// This sets the "unwind point" that we can jump back to later with longjmp if we have
+	// an assertion failure. If that happens, setjmp() will return non-zero, so we will
+	// return immediately from this function instead of trying to init hardware again (which failed last time)
+	if (setjmp(jmpEnv)) {
+		return;
+	}
+
 	/**
 	 * Initialize hardware drivers
 	 */
 	initHardware();
 
+	// periodic events need to be initialized after fuel&spark pins to avoid a warning
+	initPeriodicEvents(PASS_ENGINE_PARAMETER_SIGNATURE);
+
 #if EFI_FILE_LOGGING
 	initMmcCard();
 #endif /* EFI_FILE_LOGGING */
@@ -314,7 +343,9 @@ void runRusEfi(void) {
 
 		runSchedulingPrecisionTestIfNeeded();
 	}
+}
 
+void runMainLoop() {
 	efiPrintf("Running main loop");
 	main_loop_started = true;
 	/**

simulator/chconf.h

@@ -636,6 +636,7 @@
  * @details User fields added to the end of the @p thread_t structure.
  */
 #define CH_CFG_THREAD_EXTRA_FIELDS                                          \
+  unsigned char threadId;                                                   \
   /* Add threads custom fields here.*/
 
 /**

unit_tests/chibios-mock/mock-threads.h

@@ -9,9 +9,10 @@ typedef uint32_t systime_t;
 class thread_t {
 public:
 	const char *name;
-	  tfunc_t           funcp;
-	  void              *arg;
+	tfunc_t           funcp;
+	void              *arg;
 
+	unsigned char threadId;
 };
 
 bool chThdShouldTerminateX(void);