execute critical tasks on one big thread (#462)

* wire up main loop

* remove PeriodicTimerController

* patch up defines, comments

* happy simulator

* format

* simulator

* ETB runs via main loop

* happy tests

* protection against invalid LoopPeriod

* stack size

* ccm

* fix |= operator

* move perf trace

* ordering

* ordering

firmware/config/stm32h7ems/efifeatures.h

@@ -33,7 +33,7 @@
 #define ENABLE_PERF_TRACE TRUE
 
 // H7 runs faster "slow" ADC to make up for reduced oversampling
-#define SLOW_ADC_RATE 1000
+#define ADC_UPDATE_RATE LoopPeriod::Period1000hz
 
 #undef LUA_USER_HEAP
 #define LUA_USER_HEAP 100000

firmware/controllers/actuators/electronic_throttle.cpp

@@ -659,14 +659,7 @@ void EtbController::autoCalibrateTps() {
 	}
 }
 
-#if !EFI_UNIT_TEST
-/**
- * Things running on a timer (instead of a thread) don't participate it the RTOS's thread priority system,
- * and operate essentially "first come first serve", which risks starvation.
- * Since ETB is a safety critical device, we need the hard RTOS guarantee that it will be scheduled over other less important tasks.
- */
-#include "periodic_thread_controller.h"
-#else
+#if EFI_UNIT_TEST
 #define chThdSleepMilliseconds(x) {}
 #endif // EFI_UNIT_TEST
 
@@ -754,23 +747,6 @@ static EtbImpl<EtbController2> etb2;
 static_assert(ETB_COUNT == 2);
 static EtbController* etbControllers[] = { &etb1, &etb2 };
 
-#if !EFI_UNIT_TEST
-
-struct DcThread final : public PeriodicController<512> {
-	DcThread() : PeriodicController("DC", PRIO_ETB, ETB_LOOP_FREQUENCY) {}
-
-	void PeriodicTask(efitick_t) override {
-		// Simply update all controllers
-		for (int i = 0 ; i < ETB_COUNT; i++) {
-			etbControllers[i]->update();
-		}
-	}
-};
-
-static DcThread dcThread CCM_OPTIONAL;
-
-#endif // EFI_UNIT_TEST
-
 void etbPidReset() {
 	for (int i = 0 ; i < ETB_COUNT; i++) {
 		if (auto controller = engine->etbControllers[i]) {
@@ -957,14 +933,6 @@ void doInitElectronicThrottle() {
 		startupPositionError = true;
 	}
 #endif /* EFI_UNIT_TEST */
-
-#if !EFI_UNIT_TEST
-	static bool started = false;
-	if (started == false) {
-		dcThread.start();
-		started = true;
-	}
-#endif
 }
 
 void initElectronicThrottle() {

firmware/controllers/algo/engine.cpp

@@ -121,8 +121,6 @@ void Engine::updateTriggerWaveform() {
 }
 
 void Engine::periodicSlowCallback() {
-	ScopePerf perf(PE::EnginePeriodicSlowCallback);
-
 #if EFI_SHAFT_POSITION_INPUT
 	// Re-read config in case it's changed
 	triggerCentral.primaryTriggerConfiguration.update();

firmware/controllers/algo/engine.h

@@ -76,9 +76,6 @@
 #include "global_execution_queue.h"
 #endif /* EFI_UNIT_TEST */
 
-#define FAST_CALLBACK_PERIOD_MS 5
-#define SLOW_CALLBACK_PERIOD_MS 50
-
 struct AirmassModelBase;
 
 #define MAF_DECODING_CACHE_SIZE 256

firmware/controllers/core/core.mk

@@ -1,4 +1,5 @@
 
 CONTROLLERS_CORE_SRC_CPP = \
+	$(PROJECT_DIR)/controllers/core/main_loop.cpp \
 	$(PROJECT_DIR)/controllers/core/state_sequence.cpp \
 	$(PROJECT_DIR)/controllers/core/big_buffer.cpp \

firmware/controllers/core/main_loop.cpp

@@ -0,0 +1,94 @@
+#include "pch.h"
+
+#include "periodic_thread_controller.h"
+#include "electronic_throttle.h"
+
+#define MAIN_LOOP_RATE 1000
+
+class MainLoop : public PeriodicController<1024> {
+public:
+	MainLoop();
+	void PeriodicTask(efitick_t nowNt) override;
+
+private:
+	template <LoopPeriod TFlag>
+	LoopPeriod makePeriodFlag() const;
+
+	LoopPeriod makePeriodFlags();
+
+	int m_cycleCounter = 0;
+};
+
+static MainLoop mainLoop CCM_OPTIONAL;
+
+void initMainLoop() {
+	mainLoop.start();
+}
+
+MainLoop::MainLoop()
+	: PeriodicController("MainLoop", PRIO_MAIN_LOOP, MAIN_LOOP_RATE)
+{
+}
+
+void MainLoop::PeriodicTask(efitick_t nowNt) {
+	ScopePerf perf(PE::MainLoop);
+
+	LoopPeriod p = makePeriodFlags();
+
+#if HAL_USE_ADC
+	if (p & ADC_UPDATE_RATE) {
+		updateSlowAdc(nowNt);
+	}
+#endif // HAL_USE_ADC
+
+#if EFI_ELECTRONIC_THROTTLE_BODY
+	if (p & ETB_UPDATE_RATE) {
+		for (int i = 0 ; i < ETB_COUNT; i++) {
+			engine->etbControllers[i]->update();
+		}
+	}
+#endif // EFI_ELECTRONIC_THROTTLE_BODY
+
+	if (p & SLOW_CALLBACK_RATE) {
+		doPeriodicSlowCallback();
+	}
+
+	if (p & FAST_CALLBACK_RATE) {
+		engine->periodicFastCallback();
+	}
+}
+
+template <LoopPeriod flag>
+static constexpr int loopCounts() {
+	constexpr auto hz = hzForPeriod(flag);
+
+	// check that this cleanly divides
+	static_assert(MAIN_LOOP_RATE % hz == 0);
+
+	return MAIN_LOOP_RATE / hz;
+}
+
+template <LoopPeriod TFlag>
+LoopPeriod MainLoop::makePeriodFlag() const {
+	if (m_cycleCounter % loopCounts<TFlag>() == 0) {
+		return TFlag;
+	} else {
+		return LoopPeriod::None;
+	}
+}
+
+LoopPeriod MainLoop::makePeriodFlags() {
+	if (m_cycleCounter >= MAIN_LOOP_RATE) {
+		m_cycleCounter = 0;
+	}
+
+	LoopPeriod lp = LoopPeriod::None;
+	lp |= makePeriodFlag<LoopPeriod::Period1000hz>();
+	lp |= makePeriodFlag<LoopPeriod::Period500hz>();
+	lp |= makePeriodFlag<LoopPeriod::Period250hz>();
+	lp |= makePeriodFlag<LoopPeriod::Period20hz>();
+
+	m_cycleCounter++;
+
+	return lp;
+}

firmware/controllers/core/main_loop.h

@@ -0,0 +1,47 @@
+#pragma once
+
+void initMainLoop();
+
+enum class LoopPeriod : uint8_t {
+	None = 0,
+	Period1000hz = 1 << 0,
+	Period500hz = 1 << 1,
+	Period250hz = 1 << 2,
+	Period20hz = 1 << 3,
+};
+
+inline constexpr LoopPeriod& operator|=(LoopPeriod& a, const LoopPeriod& b) {
+	a = static_cast<LoopPeriod>(static_cast<uint8_t>(a) | static_cast<uint8_t>(b));
+	return a;
+}
+
+inline constexpr bool operator&(LoopPeriod a, LoopPeriod b) {
+	return 0 != (static_cast<uint8_t>(a) & static_cast<uint8_t>(b));
+}
+
+constexpr int hzForPeriod(LoopPeriod p) {
+	switch (p) {
+		case LoopPeriod::None: return 0;
+		case LoopPeriod::Period1000hz: return 1000;
+		case LoopPeriod::Period500hz: return 500;
+		case LoopPeriod::Period250hz: return 200;
+		case LoopPeriod::Period20hz: return 20;
+	}
+
+	return 0;
+}
+
+constexpr float loopPeriodMs(LoopPeriod p) {
+	return 1000.0f / hzForPeriod(p);
+}
+
+#ifndef ADC_UPDATE_RATE
+#define ADC_UPDATE_RATE LoopPeriod::Period500hz
+#endif
+
+#define ETB_UPDATE_RATE LoopPeriod::Period500hz
+#define FAST_CALLBACK_RATE LoopPeriod::Period250hz
+#define SLOW_CALLBACK_RATE LoopPeriod::Period20hz
+
+#define FAST_CALLBACK_PERIOD_MS loopPeriodMs(FAST_CALLBACK_RATE)
+#define SLOW_CALLBACK_PERIOD_MS loopPeriodMs(SLOW_CALLBACK_RATE)

firmware/controllers/engine_controller.cpp

@@ -65,9 +65,6 @@
 #include "logic_analyzer.h"
 #endif /* EFI_LOGIC_ANALYZER */
 
-#include "periodic_task.h"
-
-
 #if ! EFI_UNIT_TEST
 #include "init.h"
 #endif /* EFI_UNIT_TEST */
@@ -93,37 +90,6 @@ void initDataStructures() {
 #endif // EFI_ENGINE_CONTROL
 }
 
-#if !EFI_UNIT_TEST
-
-static void doPeriodicSlowCallback();
-
-class PeriodicFastController : public PeriodicTimerController {
-protected:
-	void PeriodicTask() override {
-		engine->periodicFastCallback();
-
-		if (m_slowCallbackCounter == 0) {
-			doPeriodicSlowCallback();
-
-			// Check that an integer number of fast callbacks fit in a slow callback
-			static_assert((SLOW_CALLBACK_PERIOD_MS % FAST_CALLBACK_PERIOD_MS) == 0);
-
-			m_slowCallbackCounter = SLOW_CALLBACK_PERIOD_MS / FAST_CALLBACK_PERIOD_MS;
-		}
-
-		m_slowCallbackCounter--;
-	}
-
-	int getPeriodMs() override {
-		return FAST_CALLBACK_PERIOD_MS;
-	}
-
-private:
-	size_t m_slowCallbackCounter = 0;
-};
-
-static PeriodicFastController fastController;
-
 static void resetAccel() {
 	engine->module<TpsAccelEnrichment>()->resetAE();
 
@@ -133,7 +99,9 @@ static void resetAccel() {
 	}
 }
 
-static void doPeriodicSlowCallback() {
+void doPeriodicSlowCallback() {
+	ScopePerf perf(PE::EnginePeriodicSlowCallback);
+
 #if EFI_ENGINE_CONTROL && EFI_SHAFT_POSITION_INPUT
 	efiAssertVoid(ObdCode::CUSTOM_ERR_6661, getCurrentRemainingStack() > 64, "lowStckOnEv");
 
@@ -177,10 +145,6 @@ static void doPeriodicSlowCallback() {
 
 }
 
-void initPeriodicEvents() {
-	fastController.start();
-}
-
 char * getPinNameByAdcChannel(const char *msg, adc_channel_e hwChannel, char *buffer) {
 #if HAL_USE_ADC
 	if (!isAdcChannelValid(hwChannel)) {
@@ -349,7 +313,6 @@ static void initConfigActions() {
 	addConsoleActionI("get_byte", getByte);
 	addConsoleActionII("get_bit", getBit);
 }
-#endif /* EFI_UNIT_TEST */
 
 void LedBlinkingTask::onSlowCallback() {
 	updateRunningLed();

firmware/controllers/engine_controller.h

@@ -11,7 +11,6 @@
 // todo: huh we also have validateConfiguration()?!
 bool validateConfig();
 char * getPinNameByAdcChannel(const char *msg, adc_channel_e hwChannel, char *buffer);
-void initPeriodicEvents();
 void initEngineController();
 void commonInitEngineController();
 void initStartStopButton();
@@ -19,3 +18,5 @@ void initStartStopButton();
 void initDataStructures();
 
 void slowStartStopButtonCallback();
+
+void doPeriodicSlowCallback();

firmware/controllers/system/periodic_task.cpp

@@ -1,21 +0,0 @@
-/*
- * @file periodic_task.cpp
- *
- * @date: Jul 8, 2019
- * @author Andrey Belomutskiy, (c) 2012-2020
- */
-
-#include "periodic_task.h"
-#include "os_util.h"
-#include "perf_trace.h"
-
-void runAndScheduleNext(PeriodicTimerController *controller) {
-#if !EFI_UNIT_TEST
-	{
-		ScopePerf perf(PE::PeriodicTimerControllerPeriodicTask);
-		controller->PeriodicTask();
-	}
-
-	chVTSetAny(&controller->timer, TIME_MS2I(controller->getPeriodMs()), (vtfunc_t) &runAndScheduleNext, controller);
-#endif /* EFI_UNIT_TEST */
-}

firmware/controllers/system/periodic_task.h

@@ -1,46 +0,0 @@
-/*
- * @file periodic_task.h
- *
- * @date Jul 8, 2019
- * @author Andrey Belomutskiy, (c) 2012-2020
- */
-
-#pragma once
-
-#include "global.h"
-
-class PeriodicTimerController;
-
-void runAndScheduleNext(PeriodicTimerController *controller);
-
-/**
- * this is an intermediate implementation - we should probably move from using virtual_timer_t which works on interrupts
- * into sharing one thread with potentially lower priority instead
- */
-
-class PeriodicTimerController {
-
-public:
-#if !EFI_UNIT_TEST
-	virtual_timer_t timer;
-#endif /* EFI_UNIT_TEST */
-
-	virtual void PeriodicTask() = 0;
-
-	/**
-	 * This method is invoked after corresponding PeriodicTask() invocation
-	 */
-	virtual int getPeriodMs() = 0;
-
-	/**
-	 * This invokes PeriodicTask() immediately and starts the cycle of invocations and sleeps
-	 */
-	virtual void start() {
-#if !EFI_UNIT_TEST
-		chVTObjectInit(&timer);
-#endif // EFI_UNIT_TEST
-
-		runAndScheduleNext(this);
-	}
-};
-

firmware/controllers/system/system.mk

@@ -3,7 +3,6 @@ SYSTEMSRC_CPP =	\
 	$(PROJECT_DIR)/controllers/system/efi_output.cpp \
 	$(PROJECT_DIR)/controllers/system/injection_gpio.cpp \
 	$(PROJECT_DIR)/controllers/system/efi_gpio.cpp \
-	$(PROJECT_DIR)/controllers/system/periodic_task.cpp \
 	$(PROJECT_DIR)/controllers/system/dc_motor.cpp \
 	$(PROJECT_DIR)/controllers/system/timer/scheduler.cpp \
 	$(PROJECT_DIR)/controllers/system/timer/trigger_scheduler.cpp \

firmware/controllers/thread_priority.h

@@ -8,9 +8,9 @@
 
 #pragma once
 
-// ADC and ETB get highest priority - not much else actually runs the engine
-#define PRIO_ADC (NORMALPRIO + 10)
-#define PRIO_ETB (NORMALPRIO + 9)
+// Main loop gets highest priority - it does all the critical
+// non-interrupt work to actually run the engine
+#define PRIO_MAIN_LOOP (NORMALPRIO + 10)
 
 // GPIO chips should be fast and go right back to sleep, plus can be timing sensitive
 #define PRIO_GPIOCHIP (NORMALPRIO + 8)

firmware/development/perf_trace.h

@@ -32,7 +32,7 @@ enum class PE : uint8_t {
 	SingleTimerExecutorDoExecute,
 	SingleTimerExecutorScheduleTimerCallback,
 	PeriodicControllerPeriodicTask,
-	PeriodicTimerControllerPeriodicTask,
+	MainLoop,
 	AdcCallbackFast,
 	AdcProcessSlow,
 	AdcConversionSlow,

firmware/hw_layer/adc/adc_inputs.cpp

@@ -264,36 +264,28 @@ void waitForSlowAdc(uint32_t lastAdcCounter) {
 	}
 }
 
-class SlowAdcController : public PeriodicController<256> {
-public:
-	SlowAdcController() 
-		: PeriodicController("ADC", PRIO_ADC, SLOW_ADC_RATE)
+void updateSlowAdc(efitick_t nowNt) {
 	{
-	}
+		ScopePerf perf(PE::AdcConversionSlow);
 
-	void PeriodicTask(efitick_t nowNt) override {
-		{
-			ScopePerf perf(PE::AdcConversionSlow);
-
-			if (!readSlowAnalogInputs(slowAdcSamples)) {
-				return;
-			}
-
-			// Ask the port to sample the MCU temperature
-			mcuTemperature = getMcuTemperature();
+		if (!readSlowAnalogInputs(slowAdcSamples)) {
+			return;
 		}
 
-		{
-			ScopePerf perf(PE::AdcProcessSlow);
-
-			slowAdcCounter++;
-
-			AdcSubscription::UpdateSubscribers(nowNt);
-
-			protectedGpio_check(nowNt);
-		}
+		// Ask the port to sample the MCU temperature
+		mcuTemperature = getMcuTemperature();
 	}
-};
+
+	{
+		ScopePerf perf(PE::AdcProcessSlow);
+
+		slowAdcCounter++;
+
+		AdcSubscription::UpdateSubscribers(nowNt);
+
+		protectedGpio_check(nowNt);
+	}
+}
 
 void addFastAdcChannel(const char* /*name*/, adc_channel_e setting) {
 	if (!isAdcChannelValid(setting)) {
@@ -319,8 +311,6 @@ void removeFastAdcChannel(const char *name, adc_channel_e setting) {
 // Weak link a stub so that every board doesn't have to implement this function
 __attribute__((weak)) void setAdcChannelOverrides() { }
 
-static CCM_OPTIONAL SlowAdcController slowAdcController;
-
 void initAdcInputs() {
 	efiPrintf("initAdcInputs()");
 
@@ -334,9 +324,6 @@ void initAdcInputs() {
 #if EFI_INTERNAL_ADC
 	portInitAdc();
 
-	// Start the slow ADC thread
-	slowAdcController.start();
-
 #if EFI_USE_FAST_ADC
 	fastAdc.init();
 

firmware/hw_layer/adc/adc_inputs.h

@@ -11,10 +11,6 @@
 #include "global.h"
 #include "adc_math.h"
 
-#ifndef SLOW_ADC_RATE
-#define SLOW_ADC_RATE 500
-#endif
-
 float getAnalogInputDividerCoefficient(adc_channel_e);
 
 inline bool isAdcChannelValid(adc_channel_e hwChannel) {
@@ -41,6 +37,7 @@ enum class AdcChannelMode : char {
 };
 
 void initAdcInputs();
+void updateSlowAdc(efitick_t nowNt);
 
 // deprecated - migrate to 'getAdcChannelBrainPin'
 int getAdcChannelPin(adc_channel_e hwChannel);

firmware/hw_layer/adc/adc_subscription.cpp

@@ -94,7 +94,7 @@ TODO: this code is similar to initIfValid, what is the plan? shall we extract he
 	// Populate the entry
 	entry->VoltsPerAdcVolt = voltsPerAdcVolt;
 	entry->Channel = channel;
-	entry->Filter.configureLowpass(SLOW_ADC_RATE, lowpassCutoff);
+	entry->Filter.configureLowpass(hzForPeriod(ADC_UPDATE_RATE), lowpassCutoff);
 	entry->HasUpdated = false;
 
 	// Set the sensor last - it's the field we use to determine whether this entry is in use

firmware/hw_layer/hardware.cpp

@@ -416,9 +416,6 @@ void initHardware() {
 
 #if HAL_USE_ADC
 	initAdcInputs();
-
-	// wait for first set of ADC values so that we do not produce invalid sensor data
-	waitForSlowAdc(1);
 #endif /* HAL_USE_ADC */
 
 #if EFI_SOFTWARE_KNOCK

firmware/pch/pch.h

@@ -37,6 +37,7 @@
 #include "hardware.h"
 #include "thread_priority.h"
 #include "tooth_logger.h"
+#include "main_loop.h"
 
 #if EFI_UNIT_TEST
 #include <gtest/gtest.h>

firmware/rusefi.cpp

@@ -235,7 +235,7 @@ void runRusEfi() {
 	runRusEfiWithConfig();
 
 	// periodic events need to be initialized after fuel&spark pins to avoid a warning
-	initPeriodicEvents();
+	initMainLoop();
 
 	runMainLoop();
 }

java_console/models/src/main/java/com/rusefi/tracing/EnumNames.java

@@ -16,7 +16,7 @@ public class EnumNames {
 	"SingleTimerExecutorDoExecute",
 	"SingleTimerExecutorScheduleTimerCallback",
 	"PeriodicControllerPeriodicTask",
-	"PeriodicTimerControllerPeriodicTask",
+	"INVALID2",
 	"AdcCallbackFast",
 	"AdcProcessSlow",
 	"AdcConversionSlow",

simulator/simulator/rusEfiFunctionalTest.cpp

@@ -116,7 +116,7 @@ void rusEfiFunctionalTest(void) {
 
 	runChprintfTest();
 
-	initPeriodicEvents();
+	initMainLoop();
 
 	setTriggerEmulatorRPM(DEFAULT_SIM_RPM);
 	engineConfiguration->engineSnifferRpmThreshold = DEFAULT_SNIFFER_THR;