use expected for event queue next event (#1859)

* use expected for event queue next event

* minor cleanup

* tests

firmware/controllers/system/timer/event_queue.cpp

@@ -82,8 +82,7 @@ bool EventQueue::insertTask(scheduling_s *scheduling, efitime_t timeX, action_s
  * This method is always invoked under a lock
  * @return Get the timestamp of the soonest pending action, skipping all the actions in the past
  */
-efitime_t EventQueue::getNextEventTime(efitime_t nowX) const {
-	
+expected<efitime_t> EventQueue::getNextEventTime(efitime_t nowX) const {
 	if (head != NULL) {
 		if (head->momentX <= nowX) {
 			/**
@@ -99,7 +98,8 @@ efitime_t EventQueue::getNextEventTime(efitime_t nowX) const {
 			return head->momentX;
 		}
 	}
-	return EMPTY_QUEUE;
+
+	return unexpected;
 }
 
 /**

firmware/controllers/system/timer/event_queue.h

@@ -7,14 +7,10 @@
 
 #include "scheduler.h"
 #include "utlist.h"
+#include "expected.h"
 
 #pragma once
 
-/**
- * this is a large value which is expected to be larger than any real time
- */
-#define EMPTY_QUEUE 0x0FFFFFFFFFFFFFFFLL
-
 #define QUEUE_LENGTH_LIMIT 1000
 
 // templates do not accept field names so we use a macro here
@@ -58,7 +54,7 @@ public:
 	int executeAll(efitime_t now);
 	bool executeOne(efitime_t now);
 
-	efitime_t getNextEventTime(efitime_t nowUs) const;
+	expected<efitime_t> getNextEventTime(efitime_t nowUs) const;
 	void clear(void);
 	int size(void) const;
 	scheduling_s *getElementAtIndexForUnitText(int index);

firmware/controllers/system/timer/single_timer_executor.cpp

@@ -35,11 +35,6 @@
 #include "engine.h"
 EXTERN_ENGINE;
 
-/**
- * these fields are global in order to facilitate debugging
- */
-static efitime_t nextEventTimeNt = 0;
-
 uint32_t hwSetTimerDuration;
 
 void globalTimerCallback() {
@@ -154,12 +149,16 @@ void SingleTimerExecutor::scheduleTimerCallback() {
 	 * Let's grab fresh time value
 	 */
 	efitick_t nowNt = getTimeNowNt();
-	nextEventTimeNt = queue.getNextEventTime(nowNt);
-	efiAssertVoid(CUSTOM_ERR_6625, nextEventTimeNt > nowNt, "setTimer constraint");
-	if (nextEventTimeNt == EMPTY_QUEUE)
-		return; // no pending events in the queue
+	expected<efitick_t> nextEventTimeNt = queue.getNextEventTime(nowNt);
+
+	if (!nextEventTimeNt) {
+		return; // no pending events in the queue
+	}
+
+	efiAssertVoid(CUSTOM_ERR_6625, nextEventTimeNt.Value > nowNt, "setTimer constraint");
+
+	int32_t hwAlarmTime = NT2US((int32_t)nextEventTimeNt.Value - (int32_t)nowNt);
 
-	int32_t hwAlarmTime = NT2US((int32_t)nextEventTimeNt - (int32_t)nowNt);
 	setHardwareUsTimer(hwAlarmTime == 0 ? 1 : hwAlarmTime);
 }
 

firmware/hw_layer/microsecond_timer.cpp

@@ -65,7 +65,9 @@ static volatile bool hwStarted = false;
  */
 void setHardwareUsTimer(int32_t deltaTimeUs) {
 	efiAssertVoid(OBD_PCM_Processor_Fault, hwStarted, "HW.started");
+
 	setHwTimerCounter++;
+
 	/**
 	 * #259 BUG error: not positive deltaTimeUs
 	 * Once in a while we night get an interrupt where we do not expect it
@@ -74,8 +76,12 @@ void setHardwareUsTimer(int32_t deltaTimeUs) {
 		timerFreezeCounter++;
 		warning(CUSTOM_OBD_LOCAL_FREEZE, "local freeze cnt=%d", timerFreezeCounter);
 	}
-	if (deltaTimeUs < 2)
-		deltaTimeUs = 2; // for some reason '1' does not really work
+
+	// We need the timer to fire after we return - 1 doesn't work as it may actually schedule in the past
+	if (deltaTimeUs < 2) {
+		deltaTimeUs = 2;
+	}
+
 	efiAssertVoid(CUSTOM_DELTA_NOT_POSITIVE, deltaTimeUs > 0, "not positive deltaTimeUs");
 	if (deltaTimeUs >= TOO_FAR_INTO_FUTURE_US) {
 		// we are trying to set callback for too far into the future. This does not look right at all
@@ -83,16 +89,21 @@ void setHardwareUsTimer(int32_t deltaTimeUs) {
 		return;
 	}
 
+	// If already set, reset the timer
 	if (GPTDEVICE.state == GPT_ONESHOT) {
 		gptStopTimerI(&GPTDEVICE);
 	}
+
 	if (GPTDEVICE.state != GPT_READY) {
 		firmwareError(CUSTOM_HW_TIMER, "HW timer state %d/%d", GPTDEVICE.state, setHwTimerCounter);
 		return;
 	}
+
 	if (hasFirmwareError()) {
 		return;
 	}
+
+	// Start the timer
 	gptStartOneShotI(&GPTDEVICE, deltaTimeUs);
 
 	lastSetTimerTimeNt = getTimeNowNt();
@@ -103,8 +114,7 @@ void setHardwareUsTimer(int32_t deltaTimeUs) {
 
 void globalTimerCallback();
 
-static void hwTimerCallback(GPTDriver *gptp) {
-	(void)gptp;
+static void hwTimerCallback(GPTDriver*) {
 	timerCallbackCounter++;
 	isTimerPending = false;
 
@@ -148,8 +158,7 @@ static constexpr GPTConfig gpt5cfg = { 1000000, /* 1 MHz timer clock.*/
 
 static scheduling_s watchDogBuddy;
 
-static void watchDogBuddyCallback(void *arg) {
-	(void)arg;
+static void watchDogBuddyCallback(void*) {
 	/**
 	 * the purpose of this periodic activity is to make watchdogControllerInstance
 	 * watchdog happy by ensuring that we have scheduler activity even in case of very broken configuration
@@ -161,9 +170,7 @@ static void watchDogBuddyCallback(void *arg) {
 static volatile bool testSchedulingHappened = false;
 static efitimems_t testSchedulingStart;
 
-static void timerValidationCallback(void *arg) {
-	(void)arg;
-
+static void timerValidationCallback(void*) {
 	testSchedulingHappened = true;
 	efitimems_t actualTimeSinceScheduling = (currentTimeMillis() - testSchedulingStart);
 	

unit_tests/tests/test_signal_executor.cpp

@@ -6,7 +6,6 @@
  */
 
 #include "global.h"
-#include <time.h>
 
 #include "test_signal_executor.h"
 #include "io_pins.h"
@@ -91,7 +90,7 @@ TEST(misc, testSignalExecutor) {
 	print("*************************************** testSignalExecutor\r\n");
 
 	EventQueue eq;
-	ASSERT_EQ(EMPTY_QUEUE, eq.getNextEventTime(0));
+	ASSERT_EQ(eq.getNextEventTime(0), unexpected);
 	scheduling_s s1;
 	scheduling_s s2;
 	scheduling_s s3;
@@ -131,7 +130,7 @@ TEST(misc, testSignalExecutor) {
 
 	callbackCounter = 0;
 	eq.insertTask(&s1, 10, callback);
-	ASSERT_EQ(10, eq.getNextEventTime(0));
+	ASSERT_EQ(10, eq.getNextEventTime(0).value_or(-1));
 
 	eq.executeAll(1);
 	ASSERT_EQ( 0,  callbackCounter) << "callbacks not expected";
@@ -139,14 +138,14 @@ TEST(misc, testSignalExecutor) {
 	eq.executeAll(11);
 	ASSERT_EQ(1, callbackCounter);
 
-	ASSERT_EQ(EMPTY_QUEUE, eq.getNextEventTime(0));
+	ASSERT_EQ(eq.getNextEventTime(0), unexpected);
 
 	eq.insertTask(&s1, 10, callback);
 	eq.insertTask(&s2, 13, callback);
-	ASSERT_EQ(10, eq.getNextEventTime(0));
+	ASSERT_EQ(10, eq.getNextEventTime(0).value_or(-1));
 
 	eq.executeAll(1);
-	ASSERT_EQ(10, eq.getNextEventTime(0));
+	ASSERT_EQ(10, eq.getNextEventTime(0).value_or(-1));
 
 	eq.executeAll(100);
 	ASSERT_EQ(0, eq.size());