Remove lockAnyContext, replace with CriticalSectionLocker (#1938)

* switch to CriticalSectionLocker

* that's just about all

* clean up last usage

* include hpp for sim

* need the cpp wrappers in the makefile too

* include dir

* include

firmware/config/stm32f4ems/chconf.h

@@ -508,12 +508,6 @@
 #define CH_DBG_SYSTEM_STATE_CHECK           TRUE
 #endif
 
-/**
- * micro-optimization: use same (lower-level) api for lock/unlock regardless on context
- * this saves us one branching
- */
-#define USE_PORT_LOCK FALSE
-
 /**
  * @brief   Debug option, parameters checks.
  * @details If enabled then the checks on the API functions input

firmware/config/stm32f7ems/chconf.h

@@ -508,12 +508,6 @@
 #define CH_DBG_SYSTEM_STATE_CHECK           TRUE
 #endif
 
-/**
- * micro-optimization: use same (lower-level) api for lock/unlock regardless on context
- * this saves us one branching
- */
-#define USE_PORT_LOCK FALSE
-
 /**
  * @brief   Debug option, parameters checks.
  * @details If enabled then the checks on the API functions input

firmware/console/console_io.cpp

@@ -182,11 +182,26 @@ static msg_t _put(void *ip, uint8_t b) {
 #else
 	// uartSendTimeout() needs interrupts to wait for the end of transfer, so we have to unlock them temporary
 	bool wasLocked = isLocked();
-	if (wasLocked)
+	if (wasLocked) {
-		unlockAnyContext();
+		if (isIsrContext()) {
+			chSysUnlockFromISR()
+			;
+		} else {
+			chSysUnlock()
+			;
+		}
+	}
+
 	_putt(ip, b, CONSOLE_WRITE_TIMEOUT);
-	if (wasLocked)
+	
-		lockAnyContext();
+	// Relock if we were locked before
+	if (wasLocked) {
+		if (isIsrContext()) {
+			chSysLockFromISR();
+		} else {
+			chSysLock();
+		}
+	}
 #endif /* UART_USE_BLOCKING_WRITE */
 	return MSG_OK;
 }

firmware/controllers/algo/engine.cpp

@@ -19,6 +19,7 @@
 #include "speed_density.h"
 #include "advance_map.h"
 #include "os_util.h"
+#include "os_access.h"
 #include "settings.h"
 #include "aux_valves.h"
 #include "map_averaging.h"
@@ -107,7 +108,7 @@ void Engine::initializeTriggerWaveform(Logging *logger DECLARE_ENGINE_PARAMETER_
 
 #if EFI_ENGINE_CONTROL && EFI_SHAFT_POSITION_INPUT
 	// we have a confusing threading model so some synchronization would not hurt
-	bool alreadyLocked = lockAnyContext();
+	chibios_rt::CriticalSectionLocker csl;
 
 	TRIGGER_WAVEFORM(initializeTriggerWaveform(logger,
 			engineConfiguration->ambiguousOperationMode,
@@ -138,10 +139,6 @@ void Engine::initializeTriggerWaveform(Logging *logger DECLARE_ENGINE_PARAMETER_
 				config);
 	}
 
-	if (!alreadyLocked) {
-		unlockAnyContext();
-	}
-
 	if (!TRIGGER_WAVEFORM(shapeDefinitionError)) {
 		prepareOutputSignals(PASS_ENGINE_PARAMETER_SIGNATURE);
 	}

firmware/controllers/engine_cycle/map_averaging.cpp

@@ -112,13 +112,12 @@ static void endAveraging(void *arg);
 static void startAveraging(scheduling_s *endAveragingScheduling) {
 	efiAssertVoid(CUSTOM_ERR_6649, getCurrentRemainingStack() > 128, "lowstck#9");
 
-	bool wasLocked = lockAnyContext();
+	{
-	// with locking we would have a consistent state
+		// with locking we will have a consistent state
-	mapAdcAccumulator = 0;
+		chibios_rt::CriticalSectionLocker csl;
-	mapMeasurementsCounter = 0;
+		mapAdcAccumulator = 0;
-	isAveraging = true;
+		mapMeasurementsCounter = 0;
-	if (!wasLocked) {
+		isAveraging = true;
-		unlockAnyContext();
 	}
 
 	mapAveragingPin.setHigh();
@@ -169,22 +168,19 @@ void mapAveragingAdcCallback(adcsample_t adcValue) {
 	readIndex = writeIndex;
 
 	// todo: migrate to the lock-free implementation
-	bool alreadyLocked = lockAnyContext();
+	{
-	;
+		// with locking we will have a consistent state
-	// with locking we would have a consistent state
+		chibios_rt::CriticalSectionLocker csl;
-
+		mapAdcAccumulator += adcValue;
-	mapAdcAccumulator += adcValue;
+		mapMeasurementsCounter++;
-	mapMeasurementsCounter++;
+	}
-	if (!alreadyLocked)
-		unlockAnyContext();
-	;
 }
 #endif
 
 static void endAveraging(void *arg) {
 	(void) arg;
 #if ! EFI_UNIT_TEST
-	bool wasLocked = lockAnyContext();
+	chibios_rt::CriticalSectionLocker csl;
 #endif
 	isAveraging = false;
 	// with locking we would have a consistent state
@@ -205,11 +201,6 @@ static void endAveraging(void *arg) {
 	} else {
 		warning(CUSTOM_UNEXPECTED_MAP_VALUE, "No MAP values");
 	}
-#endif
-#if ! EFI_UNIT_TEST
-	if (!wasLocked)
-		unlockAnyContext();
-	;
 #endif
 	mapAveragingPin.setLow();
 }

firmware/controllers/system/timer/signal_executor_sleep.cpp

@@ -23,6 +23,7 @@
  */
 
 #include "global.h"
+#include "os_access.h"
 #include "scheduler.h"
 #include "main_trigger_callback.h"
 
@@ -69,7 +70,8 @@ static void doScheduleForLater(scheduling_s *scheduling, int delayUs, action_s a
 		return;
 	}
 
-	bool alreadyLocked = lockAnyContext();
+	chibios_rt::CriticalSectionLocker csl;
+
 	scheduling->action = action;
 	int isArmed = chVTIsArmedI(&scheduling->timer);
 	if (isArmed) {
@@ -88,9 +90,6 @@ static void doScheduleForLater(scheduling_s *scheduling, int delayUs, action_s a
 #endif /* EFI_SIMULATOR */
 
 	chVTSetI(&scheduling->timer, delaySt, (vtfunc_t)timerCallback, scheduling);
-	if (!alreadyLocked) {
-		unlockAnyContext();
-	}
 }
 
 void SleepExecutor::scheduleForLater(scheduling_s *scheduling, int delayUs, action_s action) {

firmware/development/engine_sniffer.cpp

@@ -192,7 +192,8 @@ void WaveChart::addEvent3(const char *name, const char * msg) {
 		return;
 	}
 
-	bool alreadyLocked = lockOutputBuffer(); // we have multiple threads writing to the same output buffer
+	// we have multiple threads writing to the same output buffer
+	chibios_rt::CriticalSectionLocker csl;
 
 	if (counter == 0) {
 		startTimeNt = nowNt;
@@ -226,10 +227,6 @@ void WaveChart::addEvent3(const char *name, const char * msg) {
 		logging.appendChar(CHART_DELIMETER);
 		logging.terminate();
 	}
-
-	if (!alreadyLocked) {
-		unlockOutputBuffer();
-	}
 #endif /* EFI_TEXT_LOGGING */
 }
 

firmware/global.h

@@ -110,16 +110,3 @@ typedef unsigned int time_t;
 #define NT2US(nt) ((nt) / US_TO_NT_MULTIPLIER)
 
 #define UNIT_TEST_BUSY_WAIT_CALLBACK() {}
-
-#ifdef __cplusplus
-extern "C"
-{
-#endif
-
-bool lockAnyContext(void);
-void unlockAnyContext(void);
-
-#ifdef __cplusplus
-}
-#endif
-

firmware/hw_layer/drivers/gpio/mc33810.c

@@ -347,11 +347,9 @@ static int mc33810_wake_driver(struct mc33810_priv *chip)
 
 	if (isIsrContext()) {
 		// this is for normal runtime
-		int wasLocked = lockAnyContext();
+		syssts_s sts = chSysGetStatusAndLockX();
 		chSemSignalI(&mc33810_wake);
-		if (!wasLocked) {
+		chSysRestoreStatusX(sts);
-			unlockAnyContext();
-		}
 	} else {
 		// this is for start-up to not hang up
 		chSemSignal(&mc33810_wake);

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

@@ -67,15 +67,6 @@ static size_t _writet(void *ip, const uint8_t *bp, size_t n, sysinterval_t timeo
 static msg_t _put(void *ip, uint8_t b) {
 	(void)ip;
 	UsbDeviceCdcCom_SendByte(b);
-/*
-	// uartSendTimeout() needs interrupts to wait for the end of transfer, so we have to unlock them temporary
-	bool wasLocked = isLocked();
-	if (wasLocked)
-		unlockAnyContext();
-	_putt(ip, b, CONSOLE_WRITE_TIMEOUT);
-	if (wasLocked)
-		lockAnyContext();
-*/
 	return MSG_OK;
 }
 static size_t _write(void *ip, const uint8_t *bp, size_t n) {

firmware/rusefi.cpp

@@ -152,9 +152,8 @@ void rebootNow(void) {
  */
 static void scheduleReboot(void) {
 	scheduleMsg(&sharedLogger, "Rebooting in 3 seconds...");
-	lockAnyContext();
+	chibios_rt::CriticalSectionLocker csl;
 	chVTSetI(&resetTimer, TIME_MS2I(3000), (vtfunc_t) rebootNow, NULL);
-	unlockAnyContext();
 }
 
 void runRusEfi(void) {

firmware/util/datalogging.cpp

@@ -120,11 +120,8 @@ void Logging::vappendPrintf(const char *fmt, va_list arg) {
 		firmwareError(CUSTOM_ERR_6604, "lowstck#5b %s", chThdGetSelfX()->name);
 	}
 #endif // EFI_ENABLE_ASSERTS
-	int wasLocked = lockAnyContext();
+	chibios_rt::CriticalSectionLocker csl;
 	intermediateLogging.vappendPrintfI(this, fmt, arg);
-	if (!wasLocked) {
-		unlockAnyContext();
-	}
 #endif // EFI_UNIT_TEST
 }
 

firmware/util/datalogging.h

@@ -85,9 +85,6 @@ extern "C"
 {
 #endif /* __cplusplus */
 
-#define lockOutputBuffer lockAnyContext
-#define unlockOutputBuffer unlockAnyContext
-
 void printMsg(Logging *logging, const char *fmt, ...);
 
 /**

firmware/util/loggingcentral.cpp

@@ -10,6 +10,7 @@
  */
 
 #include "global.h"
+#include "os_access.h"
 #include "efilib.h"
 
 #if EFI_UNIT_TEST || EFI_SIMULATOR
@@ -64,7 +65,7 @@ static LoggingCentral loggingCentral;
  * This method appends the content of specified thread-local logger into the global buffer
  * of logging content.
  */
-void scheduleLogging(Logging *logging) {
+static void scheduleLoggingInternal(Logging *logging) {
 #if EFI_TEXT_LOGGING
 #ifdef EFI_PRINT_MESSAGES_TO_TERMINAL
 	print(logging->buffer);
@@ -73,28 +74,25 @@ void scheduleLogging(Logging *logging) {
 	// this could be done without locking
 	int newLength = efiStrlen(logging->buffer);
 
-	bool alreadyLocked = lockOutputBuffer();
+	chibios_rt::CriticalSectionLocker csl;
 	if (loggingCentral.accumulatedSize + newLength >= MAX_DL_CAPACITY) {
 		/**
 		 * if no one is consuming the data we have to drop it
 		 * this happens in case of serial-over-USB, todo: find a better solution?
 		 */
-		if (!alreadyLocked) {
-			unlockOutputBuffer();
-		}
-		logging->reset();
 		return;
 	}
 	// memcpy is faster then strcpy because it is not looking for line terminator
 	memcpy(accumulationBuffer + loggingCentral.accumulatedSize, logging->buffer, newLength + 1);
 	loggingCentral.accumulatedSize += newLength;
-	if (!alreadyLocked) {
-		unlockOutputBuffer();
-	}
-	logging->reset();
 #endif /* EFI_TEXT_LOGGING */
 }
 
+void scheduleLogging(Logging* logging) {
+	scheduleLoggingInternal(logging);
+	logging->reset();
+}
+
 /**
  * Actual communication layer invokes this method when it's ready to send some data out
  *
@@ -106,7 +104,7 @@ char * swapOutputBuffers(int *actualOutputBufferSize) {
 	int expectedOutputSize;
 #endif /* EFI_ENABLE_ASSERTS */
 	{ // start of critical section
-		bool alreadyLocked = lockOutputBuffer();
+		chibios_rt::CriticalSectionLocker csl;
 		/**
 		 * we cannot output under syslock, we simply rotate which buffer is which
 		 */
@@ -120,10 +118,6 @@ char * swapOutputBuffers(int *actualOutputBufferSize) {
 		accumulationBuffer = temp;
 		loggingCentral.accumulatedSize = 0;
 		accumulationBuffer[0] = 0;
-
-		if (!alreadyLocked) {
-			unlockOutputBuffer();
-		}
 	} // end of critical section
 
 	*actualOutputBufferSize = efiStrlen(outputBuffer);
@@ -166,7 +160,8 @@ void scheduleMsg(Logging *logging, const char *format, ...) {
 		warning(CUSTOM_ERR_LOGGING_NULL, "logging NULL");
 		return;
 	}
-	int wasLocked = lockAnyContext();
+
+	chibios_rt::CriticalSectionLocker csl;
 	logging->reset(); // todo: is 'reset' really needed here?
 	appendMsgPrefix(logging);
 
@@ -177,9 +172,6 @@ void scheduleMsg(Logging *logging, const char *format, ...) {
 
 	appendMsgPostfix(logging);
 	scheduleLogging(logging);
-	if (!wasLocked) {
-		unlockAnyContext();
-	}
 #endif /* EFI_TEXT_LOGGING */
 #endif /* EFI_UNIT_TEST */
 }

firmware/util/os_util.c

@@ -26,7 +26,7 @@
 #include "os_util.h"
 
 void chVTSetAny(virtual_timer_t *vtp, systime_t time, vtfunc_t vtfunc, void *par) {
-	bool wasLocked = lockAnyContext();
+	syssts_t sts = chSysGetStatusAndLockX();
 
 	/**
 	 * todo: this could be simplified once we migrate to ChibiOS 3.0
@@ -37,53 +37,11 @@ void chVTSetAny(virtual_timer_t *vtp, systime_t time, vtfunc_t vtfunc, void *par
 	}
 
 	chVTSetI(vtp, time, vtfunc, par);
-	if (!wasLocked) {
+	chSysRestoreStatusX(sts);
-		unlockAnyContext();
-	}
-}
-
-/**
- * @return TRUE if already in locked context
- * TODO: refactor to new 'syssts_t sts = chSysGetStatusAndLockX();' pattern
- */
-bool lockAnyContext(void) {
-	int alreadyLocked = isLocked();
-	if (alreadyLocked)
-		return true;
-#if USE_PORT_LOCK
-	port_lock();
-#else /* #if USE_PORT_LOCK */
-	if (isIsrContext()) {
-		chSysLockFromISR()
-		;
-	} else {
-		chSysLock()
-		;
-	}
-#endif /* #if USE_PORT_LOCK */
-	return false;
-}
-
-/**
- * TODO: refactor to new 'chSysRestoreStatusX(sts);' pattern
- */
-void unlockAnyContext(void) {
-#if USE_PORT_LOCK
-	port_unlock();
-#else /* #if USE_PORT_LOCK */
-	if (isIsrContext()) {
-		chSysUnlockFromISR()
-		;
-	} else {
-		chSysUnlock()
-		;
-	}
-#endif /* #if USE_PORT_LOCK */
 }
 
 #endif /* EFI_UNIT_TEST */
 
-
 /**
  * See also getRemainingStack()
  */
@@ -100,4 +58,3 @@ int getMaxUsedStack(uint8_t *ptr, int size) {
 #endif /* EFI_UNIT_TEST */
 	return 0;
 }
-

simulator/Makefile

@@ -121,6 +121,7 @@ include $(CHIBIOS)/os/hal/osal/rt/osal.mk
 include $(CHIBIOS)/os/rt/rt.mk
 include $(CHIBIOS)/os/common/ports/SIMIA32/compilers/GCC/port.mk
 # Other files (optional).
+include $(CHIBIOS)/os/various/cpp_wrappers/chcpp.mk
 
 ifeq ($(OS),Windows_NT)
   include ${CHIBIOS}/os/hal/ports/simulator/win32/platform.mk
@@ -191,6 +192,7 @@ INCDIR = . \
           $(KERNINC) \
           $(OSALINC) \
           $(HALINC) \
+          $(CHCPPINC) \
           $(PLATFORMINC) \
           $(BOARDINC) \
           $(UTIL_INC) \

simulator/chconf.h

@@ -481,8 +481,6 @@
   #define ON_LOCK_HOOK
   #define ON_UNLOCK_HOOK
 
-#define USE_PORT_LOCK FALSE
-
 /**
  * @brief   Debug option, parameters checks.
  * @details If enabled then the checks on the API functions input

simulator/simulator/global.h

@@ -67,9 +67,6 @@ void printToConsole(char *p);
 
 int getRemainingStack(thread_t *otp);
 
-// todo: move somewhere else?
-bool lockAnyContext(void);
-void unlockAnyContext(void);
 void applyNewConfiguration(void);
 
 #ifdef __cplusplus

simulator/simulator/os_access.h

@@ -19,6 +19,9 @@ extern "C"
 
 #ifdef __cplusplus
 }
+
+// ChibiOS c++ wrappers
+#include "ch.hpp"
 #endif /* __cplusplus */
 
 #define HAS_OS_ACCESS

unit_tests/global.h

@@ -95,8 +95,11 @@ void print(const char *fmt, ...);
 
 #define CONFIG_PARAM(x) (x)
 
-#define lockAnyContext() false
+#ifdef __cplusplus
-
+namespace chibios_rt {
-#define unlockAnyContext() {}
+	// Noop for unit tests - this does real lock in FW/sim
+	class CriticalSectionLocker { };
+}
+#endif
 
 #define UNIT_TEST_BUSY_WAIT_CALLBACK() { 	timeNowUs++; }