From 2792828dce2556f5bc6b335456434deb887771ce Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Thu, 19 Nov 2020 03:56:02 -0800 Subject: [PATCH] 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 | 6 --- firmware/config/stm32f7ems/chconf.h | 6 --- firmware/console/console_io.cpp | 23 +++++++-- firmware/controllers/algo/engine.cpp | 7 +-- .../engine_cycle/map_averaging.cpp | 35 +++++--------- .../system/timer/signal_executor_sleep.cpp | 7 ++- firmware/development/engine_sniffer.cpp | 7 +-- firmware/global.h | 13 ----- firmware/hw_layer/drivers/gpio/mc33810.c | 6 +-- .../cypress/serial_over_usb/usbconsole.c | 9 ---- firmware/rusefi.cpp | 3 +- firmware/util/datalogging.cpp | 5 +- firmware/util/datalogging.h | 3 -- firmware/util/loggingcentral.cpp | 30 +++++------- firmware/util/os_util.c | 47 +------------------ simulator/Makefile | 2 + simulator/chconf.h | 2 - simulator/simulator/global.h | 3 -- simulator/simulator/os_access.h | 3 ++ unit_tests/global.h | 9 ++-- 20 files changed, 67 insertions(+), 159 deletions(-) diff --git a/firmware/config/stm32f4ems/chconf.h b/firmware/config/stm32f4ems/chconf.h index a05a9ff776..0308fd60fa 100644 --- a/firmware/config/stm32f4ems/chconf.h +++ b/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 diff --git a/firmware/config/stm32f7ems/chconf.h b/firmware/config/stm32f7ems/chconf.h index e0e638900c..686c86223e 100644 --- a/firmware/config/stm32f7ems/chconf.h +++ b/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 diff --git a/firmware/console/console_io.cpp b/firmware/console/console_io.cpp index 5fb84178e4..7ae394992a 100644 --- a/firmware/console/console_io.cpp +++ b/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) - unlockAnyContext(); + if (wasLocked) { + 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; } diff --git a/firmware/controllers/algo/engine.cpp b/firmware/controllers/algo/engine.cpp index 706a29ec68..53734d50f8 100644 --- a/firmware/controllers/algo/engine.cpp +++ b/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); } diff --git a/firmware/controllers/engine_cycle/map_averaging.cpp b/firmware/controllers/engine_cycle/map_averaging.cpp index b20a1f19b6..d01d99dd33 100644 --- a/firmware/controllers/engine_cycle/map_averaging.cpp +++ b/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 - mapAdcAccumulator = 0; - mapMeasurementsCounter = 0; - isAveraging = true; - if (!wasLocked) { - unlockAnyContext(); + { + // with locking we will have a consistent state + chibios_rt::CriticalSectionLocker csl; + mapAdcAccumulator = 0; + mapMeasurementsCounter = 0; + isAveraging = true; } 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 would have a consistent state - - mapAdcAccumulator += adcValue; - mapMeasurementsCounter++; - if (!alreadyLocked) - unlockAnyContext(); - ; + { + // with locking we will have a consistent state + chibios_rt::CriticalSectionLocker csl; + mapAdcAccumulator += adcValue; + mapMeasurementsCounter++; + } } #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(); } diff --git a/firmware/controllers/system/timer/signal_executor_sleep.cpp b/firmware/controllers/system/timer/signal_executor_sleep.cpp index 22a488b126..cb26df8124 100644 --- a/firmware/controllers/system/timer/signal_executor_sleep.cpp +++ b/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) { diff --git a/firmware/development/engine_sniffer.cpp b/firmware/development/engine_sniffer.cpp index eea1559690..bae88cebda 100644 --- a/firmware/development/engine_sniffer.cpp +++ b/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 */ } diff --git a/firmware/global.h b/firmware/global.h index 0587a02674..3733206794 100644 --- a/firmware/global.h +++ b/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 - diff --git a/firmware/hw_layer/drivers/gpio/mc33810.c b/firmware/hw_layer/drivers/gpio/mc33810.c index a25109e699..8b7d97ee91 100644 --- a/firmware/hw_layer/drivers/gpio/mc33810.c +++ b/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) { - unlockAnyContext(); - } + chSysRestoreStatusX(sts); } else { // this is for start-up to not hang up chSemSignal(&mc33810_wake); diff --git a/firmware/hw_layer/ports/cypress/serial_over_usb/usbconsole.c b/firmware/hw_layer/ports/cypress/serial_over_usb/usbconsole.c index 24489995d3..6f88fb7f21 100644 --- a/firmware/hw_layer/ports/cypress/serial_over_usb/usbconsole.c +++ b/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) { diff --git a/firmware/rusefi.cpp b/firmware/rusefi.cpp index ef63ab8e93..13451d89a9 100644 --- a/firmware/rusefi.cpp +++ b/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) { diff --git a/firmware/util/datalogging.cpp b/firmware/util/datalogging.cpp index 449b64cde4..ce1b27399e 100644 --- a/firmware/util/datalogging.cpp +++ b/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 } diff --git a/firmware/util/datalogging.h b/firmware/util/datalogging.h index 66c23b94ba..81b566ec2c 100644 --- a/firmware/util/datalogging.h +++ b/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, ...); /** diff --git a/firmware/util/loggingcentral.cpp b/firmware/util/loggingcentral.cpp index a7ab1a2024..100113bfc4 100644 --- a/firmware/util/loggingcentral.cpp +++ b/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 */ } diff --git a/firmware/util/os_util.c b/firmware/util/os_util.c index dbe305d491..d533a6d4fe 100644 --- a/firmware/util/os_util.c +++ b/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) { - 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 */ + chSysRestoreStatusX(sts); } #endif /* EFI_UNIT_TEST */ - /** * See also getRemainingStack() */ @@ -100,4 +58,3 @@ int getMaxUsedStack(uint8_t *ptr, int size) { #endif /* EFI_UNIT_TEST */ return 0; } - diff --git a/simulator/Makefile b/simulator/Makefile index cd740317f0..8f9fbb25f0 100644 --- a/simulator/Makefile +++ b/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) \ diff --git a/simulator/chconf.h b/simulator/chconf.h index cd5779b7e1..71623709b0 100644 --- a/simulator/chconf.h +++ b/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 diff --git a/simulator/simulator/global.h b/simulator/simulator/global.h index 7e4e8a16eb..0c1233021a 100644 --- a/simulator/simulator/global.h +++ b/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 diff --git a/simulator/simulator/os_access.h b/simulator/simulator/os_access.h index dcfd7d8648..a1d4400ab7 100644 --- a/simulator/simulator/os_access.h +++ b/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 diff --git a/unit_tests/global.h b/unit_tests/global.h index a16ad622ec..26d435de76 100644 --- a/unit_tests/global.h +++ b/unit_tests/global.h @@ -95,8 +95,11 @@ void print(const char *fmt, ...); #define CONFIG_PARAM(x) (x) -#define lockAnyContext() false - -#define unlockAnyContext() {} +#ifdef __cplusplus +namespace chibios_rt { + // Noop for unit tests - this does real lock in FW/sim + class CriticalSectionLocker { }; +} +#endif #define UNIT_TEST_BUSY_WAIT_CALLBACK() { timeNowUs++; }