TriggerScheduler cleanup (#4844)

* TriggerScheduler cleanup

* remove TRIGGER_EVENT_UNDEFINED

* remove dead overload of scheduleOrQueue

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

firmware/controllers/algo/event_registry.h

@@ -23,6 +23,8 @@ struct AngleBasedEventBase {
 	 */
 	AngleBasedEventBase *nextToothEvent = nullptr;
 
+	virtual void setAngle(angle_t angle) = 0;
+
 	virtual bool shouldSchedule(uint32_t trgEventIndex, float currentPhase, float nextPhase) const = 0;
 	virtual float getAngleFromNow(float currentPhase) const = 0;
 
@@ -44,6 +46,7 @@ public:
 struct AngleBasedEventOld : public AngleBasedEventBase {
 	event_trigger_position_s position;
 
+	void setAngle(angle_t angle) override;
 	bool shouldSchedule(uint32_t trgEventIndex, float currentPhase, float nextPhase) const override;
 	float getAngleFromNow(float currentPhase) const override;
 
@@ -53,7 +56,9 @@ struct AngleBasedEventOld : public AngleBasedEventBase {
 struct AngleBasedEventNew : public AngleBasedEventBase {
 	float enginePhase;
 
+	void setAngle(angle_t angle) override;
 	bool shouldSchedule(uint32_t trgEventIndex, float currentPhase, float nextPhase) const override;
+	bool shouldSchedule(float currentPhase, float nextPhase) const;
 	float getAngleFromNow(float currentPhase) const override;
 };
 

firmware/controllers/engine_cycle/aux_valves.cpp

@@ -25,9 +25,7 @@ static void plainPinTurnOff(NamedOutputPin *output) {
 
 
 static void scheduleOpen(AuxActor *current) {
-	engine->module<TriggerScheduler>()->scheduleOrQueue(&current->open,
-			TRIGGER_EVENT_UNDEFINED,
-			getTimeNowNt(),
+	engine->module<TriggerScheduler>()->schedule(&current->open,
 			current->extra + engine->engineState.auxValveStart,
 			{ auxPlainPinTurnOn, current }
 			);
@@ -43,9 +41,7 @@ void auxPlainPinTurnOn(AuxActor *current) {
 
 	fixAngle(duration, "duration", CUSTOM_ERR_6557);
 
-	engine->module<TriggerScheduler>()->scheduleOrQueue(&current->close,
-			TRIGGER_EVENT_UNDEFINED,
-			getTimeNowNt(),
+	engine->module<TriggerScheduler>()->schedule(&current->close,
 			current->extra + engine->engineState.auxValveEnd,
 			{ plainPinTurnOff, output }
 			);

firmware/controllers/engine_cycle/high_pressure_fuel_pump.cpp

@@ -201,8 +201,8 @@ void HpfpController::scheduleNextCycle() {
 		/**
 		 * We are good to use just one m_event instance because new events are scheduled when we turn off valve.
 		 */
-		engine->module<TriggerScheduler>()->scheduleOrQueue(
-			&m_event, TRIGGER_EVENT_UNDEFINED, 0,
+		engine->module<TriggerScheduler>()->schedule(
+			&m_event,
 			di_nextStart,
 			{ pinTurnOn, this });
 
@@ -211,8 +211,8 @@ void HpfpController::scheduleNextCycle() {
 		// Schedule this, even if we aren't opening the valve this time, since this
 		// will schedule the next lobe.
 		// todo: would it have been cleaner to schedule 'scheduleNextCycle' directly?
-		engine->module<TriggerScheduler>()->scheduleOrQueue(
-			&m_event, TRIGGER_EVENT_UNDEFINED, 0, lobe,
+		engine->module<TriggerScheduler>()->schedule(
+			&m_event, lobe,
 			{ pinTurnOff, this });
 	}
 }

firmware/controllers/engine_cycle/main_trigger_callback.cpp

@@ -290,7 +290,7 @@ void mainTriggerCallback(uint32_t trgEventIndex, efitick_t edgeTimestamp, angle_
 	/**
 	 * For spark we schedule both start of coil charge and actual spark based on trigger angle
 	 */
-	onTriggerEventSparkLogic(trgEventIndex, rpm, edgeTimestamp, currentPhase, nextPhase);
+	onTriggerEventSparkLogic(rpm, edgeTimestamp, currentPhase, nextPhase);
 }
 
 #endif /* EFI_ENGINE_CONTROL */

firmware/controllers/engine_cycle/spark_logic.cpp

@@ -317,7 +317,7 @@ void turnSparkPinHigh(IgnitionEvent *event) {
 	}
 }
 
-static void scheduleSparkEvent(bool limitedSpark, uint32_t trgEventIndex, IgnitionEvent *event,
+static void scheduleSparkEvent(bool limitedSpark, IgnitionEvent *event,
 		int rpm, efitick_t edgeTimestamp, float currentPhase, float nextPhase) {
 
 	angle_t sparkAngle = event->sparkAngle;
@@ -374,17 +374,17 @@ static void scheduleSparkEvent(bool limitedSpark, uint32_t trgEventIndex, Igniti
 	assertAngleRange(sparkAngle, "findAngle#a5", CUSTOM_ERR_6549);
 
 	bool scheduled = engine->module<TriggerScheduler>()->scheduleOrQueue(
-		&event->sparkEvent, trgEventIndex, edgeTimestamp, sparkAngle,
+		&event->sparkEvent, edgeTimestamp, sparkAngle,
 		{ fireSparkAndPrepareNextSchedule, event },
 		currentPhase, nextPhase);
 
 	if (scheduled) {
 #if SPARK_EXTREME_LOGGING
-		efiPrintf("scheduling sparkDown ind=%d %d %s now=%d later id=%d", trgEventIndex, getRevolutionCounter(), event->getOutputForLoggins()->name, (int)getTimeNowUs(), event->sparkId);
+		efiPrintf("scheduling sparkDown %d %s now=%d later id=%d", getRevolutionCounter(), event->getOutputForLoggins()->name, (int)getTimeNowUs(), event->sparkId);
 #endif /* FUEL_MATH_EXTREME_LOGGING */
 	} else {
 #if SPARK_EXTREME_LOGGING
-		efiPrintf("to queue sparkDown ind=%d %d %s now=%d for id=%d angle=%.1f", trgEventIndex, getRevolutionCounter(), event->getOutputForLoggins()->name, (int)getTimeNowUs(), event->sparkId, sparkAngle);
+		efiPrintf("to queue sparkDown %d %s now=%d for id=%d angle=%.1f", getRevolutionCounter(), event->getOutputForLoggins()->name, (int)getTimeNowUs(), event->sparkId, sparkAngle);
 #endif /* SPARK_EXTREME_LOGGING */
 
 		if (!limitedSpark && engine->enableOverdwellProtection) {
@@ -451,7 +451,7 @@ static void prepareIgnitionSchedule() {
 	initializeIgnitionActions();
 }
 
-void onTriggerEventSparkLogic(uint32_t trgEventIndex, int rpm, efitick_t edgeTimestamp, float currentPhase, float nextPhase) {
+void onTriggerEventSparkLogic(int rpm, efitick_t edgeTimestamp, float currentPhase, float nextPhase) {
 	ScopePerf perf(PE::OnTriggerEventSparkLogic);
 
 	if (!isValidRpm(rpm) || !engineConfiguration->isIgnitionEnabled) {
@@ -498,7 +498,7 @@ void onTriggerEventSparkLogic(uint32_t trgEventIndex, int rpm, efitick_t edgeTim
 			}
 #endif // EFI_LAUNCH_CONTROL
 
-			scheduleSparkEvent(limitedSpark, trgEventIndex, event, rpm, edgeTimestamp, currentPhase, nextPhase);
+			scheduleSparkEvent(limitedSpark, event, rpm, edgeTimestamp, currentPhase, nextPhase);
 		}
 	}
 }

firmware/controllers/engine_cycle/spark_logic.h

@@ -7,7 +7,7 @@
 
 #pragma once
 
-void onTriggerEventSparkLogic(uint32_t trgEventIndex, int rpm, efitick_t edgeTimestamp, float currentPhase, float nextPhase);
+void onTriggerEventSparkLogic(int rpm, efitick_t edgeTimestamp, float currentPhase, float nextPhase);
 void turnSparkPinHigh(IgnitionEvent *event);
 void fireSparkAndPrepareNextSchedule(IgnitionEvent *event);
 int getNumberOfSparks(ignition_mode_e mode);

firmware/controllers/system/timer/trigger_scheduler.cpp

@@ -6,81 +6,26 @@ bool TriggerScheduler::assertNotInList(AngleBasedEventBase *head, AngleBasedEven
        assertNotInListMethodBody(AngleBasedEventBase, head, element, nextToothEvent)
 }
 
+void TriggerScheduler::schedule(AngleBasedEventBase* event, angle_t angle, action_s action) {
+	event->setAngle(angle);
+
+	schedule(event, action);
+}
+
 /**
  * Schedules 'action' to occur at engine cycle angle 'angle'.
  *
- * If you know when a recent trigger occured, you can pass it in as 'trgEventIndex' and
- * 'edgeTimestamp'.  Otherwise pass in TRIGGER_EVENT_UNDEFINED and the work will be scheduled on
- * the next trigger edge.
- *
  * @return true if event corresponds to current tooth and was time-based scheduler
  *         false if event was put into queue for scheduling at a later tooth
  */
-bool TriggerScheduler::scheduleOrQueue(AngleBasedEventOld *event,
-		uint32_t trgEventIndex,
-		efitick_t edgeTimestamp,
-		angle_t angle,
-		action_s action) {
-	event->position.setAngle(angle);
-
-	/**
-	 * Here's the status as of Jan 2020:
-	 * Once we hit the last trigger tooth prior to needed event, schedule it by time.  We use
-	 * as much trigger position angle as possible and only use less precise RPM-based time
-	 * calculation for the last portion of the angle, the one between two teeth closest to the
-	 * desired angle moment.
-	 */
-	if (trgEventIndex != TRIGGER_EVENT_UNDEFINED && event->position.triggerEventIndex == trgEventIndex) {
-		/**
-		 * Spark should be fired before the next trigger event - time-based delay is best precision possible
-		 */
-		scheduling_s * sDown = &event->scheduling;
-
-		scheduleByAngle(
-			sDown,
-			edgeTimestamp,
-			event->position.angleOffsetFromTriggerEvent,
-			action
-		);
-
-		return true;
-	} else {
-		event->action = action;
-		/**
-		 * Spark should be scheduled in relation to some future trigger event, this way we get better firing precision
-		 */
-		{
-			chibios_rt::CriticalSectionLocker csl;
-
-			// TODO: This is O(n), consider some other way of detecting if in a list,
-			// and consider doubly linked or other list tricks.
-
-			if (!assertNotInList(m_angleBasedEventsHead, event)) {
-				// Use Append to retain some semblance of event ordering in case of
-				// time skew.  Thus on events are always followed by off events.
-				LL_APPEND2(m_angleBasedEventsHead, event, nextToothEvent);
-
-				return false;
-			}
-		}
-		engine->outputChannels.systemEventReuse++; // not atomic/not volatile but good enough for just debugging
-#if SPARK_EXTREME_LOGGING
-		efiPrintf("isPending thus not adding to queue index=%d rev=%d now=%d",
-			  trgEventIndex, getRevolutionCounter(), (int)getTimeNowUs());
-#endif /* SPARK_EXTREME_LOGGING */
-		return false;
-	}
-}
-
 bool TriggerScheduler::scheduleOrQueue(AngleBasedEventNew *event,
-		uint32_t trgEventIndex,
 		efitick_t edgeTimestamp,
 		angle_t angle,
 		action_s action,
 		float currentPhase, float nextPhase) {
-	event->enginePhase = angle;
+	event->setAngle(angle);
 
-	if (event->shouldSchedule(trgEventIndex, currentPhase, nextPhase)) {
+	if (event->shouldSchedule(currentPhase, nextPhase)) {
 		// if we're due now, just schedule the event
 		scheduleByAngle(
 			&event->scheduling,
@@ -91,26 +36,28 @@ bool TriggerScheduler::scheduleOrQueue(AngleBasedEventNew *event,
 
 		return true;
 	} else {
-		// Not due at this tooth, add to the list to execute later
-		event->action = action;
+		// If not due now, add it to the queue to be scheduled later
+		schedule(event, action);
 
-		{
-			chibios_rt::CriticalSectionLocker csl;
+		return false;
+	}
+}
 
-			// TODO: This is O(n), consider some other way of detecting if in a list,
-			// and consider doubly linked or other list tricks.
+void TriggerScheduler::schedule(AngleBasedEventBase* event, action_s action) {
+	event->action = action;
 
-			if (!assertNotInList(m_angleBasedEventsHead, event)) {
-				// Use Append to retain some semblance of event ordering in case of
-				// time skew.  Thus on events are always followed by off events.
-				LL_APPEND2(m_angleBasedEventsHead, event, nextToothEvent);
+	{
+		chibios_rt::CriticalSectionLocker csl;
 
-				return false;
-			}
+		// TODO: This is O(n), consider some other way of detecting if in a list,
+		// and consider doubly linked or other list tricks.
+
+		if (!assertNotInList(m_angleBasedEventsHead, event)) {
+			// Use Append to retain some semblance of event ordering in case of
+			// time skew.  Thus on events are always followed by off events.
+			LL_APPEND2(m_angleBasedEventsHead, event, nextToothEvent);
 		}
 	}
-
-	return false;
 }
 
 void TriggerScheduler::scheduleEventsUntilNextTriggerTooth(int rpm,
@@ -177,6 +124,10 @@ void TriggerScheduler::scheduleEventsUntilNextTriggerTooth(int rpm,
 	}
 }
 
+void AngleBasedEventOld::setAngle(angle_t angle) {
+	position.setAngle(angle);
+}
+
 bool AngleBasedEventOld::shouldSchedule(uint32_t trgEventIndex, float /*currentPhase*/, float /*nextPhase*/) const {
 	return position.triggerEventIndex == trgEventIndex;
 }
@@ -185,7 +136,15 @@ float AngleBasedEventOld::getAngleFromNow(float /*currentPhase*/) const {
 	return position.angleOffsetFromTriggerEvent;
 }
 
+void AngleBasedEventNew::setAngle(angle_t angle) {
+	this->enginePhase = angle;
+}
+
 bool AngleBasedEventNew::shouldSchedule(uint32_t /*trgEventIndex*/, float currentPhase, float nextPhase) const {
+	return shouldSchedule(currentPhase, nextPhase);
+}
+
+bool AngleBasedEventNew::shouldSchedule(float currentPhase, float nextPhase) const {
 	return isPhaseInRange(this->enginePhase, currentPhase, nextPhase);
 }
 

firmware/controllers/system/timer/trigger_scheduler.h

@@ -1,17 +1,10 @@
 #pragma once
 
-#define TRIGGER_EVENT_UNDEFINED INT32_MAX
-
 class TriggerScheduler : public EngineModule {
 public:
-	bool scheduleOrQueue(AngleBasedEventOld *event,
-			     uint32_t trgEventIndex,
-			     efitick_t edgeTimestamp,
-			     angle_t angle,
-			     action_s action);
+	void schedule(AngleBasedEventBase* event, angle_t angle, action_s action);
 
 	bool scheduleOrQueue(AngleBasedEventNew *event,
-			     uint32_t trgEventIndex,
 			     efitick_t edgeTimestamp,
 			     angle_t angle,
 			     action_s action,
@@ -26,6 +19,8 @@ public:
 	AngleBasedEventBase * getElementAtIndexForUnitTest(int index);
 
 private:
+	void schedule(AngleBasedEventBase* event, action_s action);
+
 	bool assertNotInList(AngleBasedEventBase *head, AngleBasedEventBase *element);
 
 	/**