allow logging multiple warnings at once (#4414)

* allow logging multiple warnings at once

* comment

* .Code

* Revert ".Code"

This reverts commit 2b986bd50035aeec051d11aafd83fdbc1694351c.

* add a better comparison operator instead of conversion

* dot code

* Revert "dot code"

This reverts commit 35f6ae1007c41e3b30ea129a324f33ab4205036d.

* force gcc-10 maybe?

* Revert "force gcc-10 maybe?"

This reverts commit 4ddf8bcefddd354ccdc1da682c5fff1e68d44273.

* vq

* dot code

* dead

* dot code

* return may be null

* static_vector; maybe cyclic_buffer is broken or wrong tool for the job?

* move static vector

* put cyclic buffer back how it was since we don't use it now

firmware/console/status_loop.cpp

@@ -797,7 +797,7 @@ void updateTunerStudioState() {
 	tsOutputChannels->hasCriticalError = hasFirmwareError();
 #endif // HW_CHECK_MODE
 
-	tsOutputChannels->isWarnNow = engine->engineState.warnings.isWarningNow(timeSeconds, true);
+	tsOutputChannels->isWarnNow = engine->engineState.warnings.isWarningNow();
 #if EFI_HIP_9011_DEBUG
 	tsOutputChannels->isKnockChipOk = (instance.invalidResponsesCount == 0);
 #endif /* EFI_HIP_9011 */
@@ -818,7 +818,7 @@ void updateTunerStudioState() {
 	tsOutputChannels->warningCounter = engine->engineState.warnings.warningCounter;
 	tsOutputChannels->lastErrorCode = engine->engineState.warnings.lastErrorCode;
 	for (int i = 0; i < 8;i++) {
-		tsOutputChannels->recentErrorCode[i] = engine->engineState.warnings.recentWarnings.get(i);
+		tsOutputChannels->recentErrorCode[i] = engine->engineState.warnings.recentWarnings.get(i).Code;
 	}
 
 	tsOutputChannels->startStopStateToggleCounter = engine->startStopStateToggleCounter;

firmware/controllers/algo/engine2.cpp

@@ -34,27 +34,51 @@ WarningCodeState::WarningCodeState() {
 void WarningCodeState::clear() {
 	warningCounter = 0;
 	lastErrorCode = 0;
-	timeOfPreviousWarning = DEEP_IN_THE_PAST_SECONDS;
 	recentWarnings.clear();
 }
 
 void WarningCodeState::addWarningCode(obd_code_e code) {
 	warningCounter++;
 	lastErrorCode = code;
-	if (!recentWarnings.contains(code)) {
+
+	warning_t* existing = recentWarnings.find(code);
+
+	if (!existing) {
 		chibios_rt::CriticalSectionLocker csl;
 
-		// We don't bother double checking
-		recentWarnings.add((int)code);
+		// Add the code to the list
+		existing = recentWarnings.add(warning_t(code));
 	}
+
+	if (existing) {
+		// Reset the timer on the code to now
+		existing->LastTriggered.reset();
+	}
+
+	// Reset the "any warning" timer too
+	timeSinceLastWarning.reset();
 }
 
 /**
  * @param forIndicator if we want to retrieving value for TS indicator, this case a minimal period is applued
  */
-bool WarningCodeState::isWarningNow(efitimesec_t now, bool forIndicator) const {
-	int period = forIndicator ? maxI(3, engineConfiguration->warningPeriod) : engineConfiguration->warningPeriod;
-	return absI(now - timeOfPreviousWarning) < period;
+bool WarningCodeState::isWarningNow() const {
+	int period = maxI(3, engineConfiguration->warningPeriod);
+
+	return !timeSinceLastWarning.hasElapsedSec(period);
+}
+
+// Check whether a particular warning is active
+bool WarningCodeState::isWarningNow(obd_code_e code) const {
+	warning_t* warn = recentWarnings.find(code);
+
+	// No warning found at all
+	if (!warn) {
+		return false;
+	}
+
+	// If the warning is old, it is not active
+	return !warn->LastTriggered.hasElapsedSec(maxI(3, engineConfiguration->warningPeriod));
 }
 
 void FuelConsumptionState::consumeFuel(float grams, efitick_t nowNt) {

firmware/controllers/algo/engine_parts.h

@@ -7,7 +7,7 @@
 
 #pragma once
 
-#include "cyclic_buffer.h"
+#include "static_vector.h"
 #include "timer.h"
 
 #define MOCK_ADC_SIZE 26
@@ -43,17 +43,42 @@ public:
 	gear_e gearSelectorPosition;
 };
 
-typedef cyclic_buffer<int, 8> warningBuffer_t;
+struct warning_t {
+	Timer LastTriggered;
+	obd_code_e Code = OBD_None;
+
+	warning_t() { }
+
+	explicit warning_t(obd_code_e code)
+		: Code(code)
+	{
+	}
+
+	// Equality just checks the code, timer doesn't matter
+	bool operator ==(const warning_t& other) const {
+		return other.Code == Code;
+	}
+
+	// Compare against a plain OBD code
+	bool operator ==(const obd_code_e other) const {
+		return other == Code;
+	}
+};
+
+typedef static_vector<warning_t, 8> warningBuffer_t;
 
 class WarningCodeState {
 public:
 	WarningCodeState();
 	void addWarningCode(obd_code_e code);
-	bool isWarningNow(efitimesec_t now, bool forIndicator) const;
+	bool isWarningNow() const;
+	bool isWarningNow(obd_code_e code) const;
 	void clear();
 	int warningCounter;
 	int lastErrorCode;
-	efitimesec_t timeOfPreviousWarning;
+
+	Timer timeSinceLastWarning;
+
 	// todo: we need a way to post multiple recent warnings into TS
 	warningBuffer_t recentWarnings;
 };

firmware/controllers/core/error_handling.cpp

@@ -144,13 +144,12 @@ bool warning(obd_code_e code, const char *fmt, ...) {
 #endif /* EFI_SIMULATOR */
 
 #if EFI_SIMULATOR || EFI_PROD_CODE
-	engine->engineState.warnings.addWarningCode(code);
+	// we just had this same warning, let's not spam
+	if (engine->engineState.warnings.isWarningNow(code) || !warningEnabled) {
+		return true;
+	}
 
-	// todo: move this logic into WarningCodeState?
-	efitimesec_t now = getTimeNowSeconds();
-	if (engine->engineState.warnings.isWarningNow(now, false) || !warningEnabled)
-		return true; // we just had another warning, let's not spam
-	engine->engineState.warnings.timeOfPreviousWarning = now;
+	engine->engineState.warnings.addWarningCode(code);
 
 	va_list ap;
 	va_start(ap, fmt);

firmware/util/containers/static_vector.h

@@ -0,0 +1,46 @@
+#pragma once
+
+template <typename T, int TSlots>
+struct static_vector {
+
+	void clear() {
+		m_size = 0;
+	}
+
+	template <typename TSearch>
+	T* find(const TSearch& search) const {
+		for (size_t i = 0; i < m_size; i++) {
+			if (m_storage[i] == search) {
+				return const_cast<T*>(&m_storage[i]);
+			}
+		}
+
+		return nullptr;
+	}
+
+	T* add(const T& value) {
+		if (m_size >= TSlots) {
+			// vector full, discard
+			return nullptr;
+		}
+
+		auto& location = m_storage[m_size];
+
+		location = value;
+		m_size++;
+
+		return &location;
+	}
+
+	T& get(size_t i) {
+		return m_storage[i];
+	}
+
+	size_t getCount() const {
+		return m_size;
+	}
+
+private:
+	size_t m_size = 0;
+	T m_storage[TSlots];
+};

firmware/util/efitime.h

@@ -42,12 +42,6 @@
 // See USF2NT above for when to use MSF2NT
 #define MSF2NT(msTimeFloat) USF2NT(MS2US(msTimeFloat))
 
-/**
- * We use this 'deep in past, before ECU has ever started' value as a way to unify
- * handling of first ever event and an event which has happened after a large pause in engine activity
- */
-#define DEEP_IN_THE_PAST_SECONDS -10
-
 #ifdef __cplusplus
 /**
  * Provide a 62-bit counter from a 32-bit counter source that wraps around.

unit_tests/tests/trigger/test_cam_vvt_input.cpp

@@ -54,7 +54,7 @@ TEST(trigger, testNoStartUpWarnings) {
 		eth.fireFall(150);
 	}
 	EXPECT_EQ( 1,  unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testNoStartUpWarnings CUSTOM_SYNC_COUNT_MISMATCH expected";
-	EXPECT_EQ(CUSTOM_SYNC_ERROR, unitTestWarningCodeState.recentWarnings.get(0));
+	EXPECT_EQ(CUSTOM_SYNC_ERROR, unitTestWarningCodeState.recentWarnings.get(0).Code);
 }
 
 TEST(trigger, testNoisyInput) {
@@ -74,8 +74,8 @@ TEST(trigger, testNoisyInput) {
 	ASSERT_EQ(NOISY_RPM,  Sensor::getOrZero(SensorType::Rpm)) << "testNoisyInput RPM should be noisy";
 
 	ASSERT_EQ( 2,  unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testNoisyInput";
-	ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, unitTestWarningCodeState.recentWarnings.get(0)) << "@0";
-	ASSERT_EQ(OBD_Crankshaft_Position_Sensor_A_Circuit_Malfunction, unitTestWarningCodeState.recentWarnings.get(1)) << "@0";
+	ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, unitTestWarningCodeState.recentWarnings.get(0).Code) << "@0";
+	ASSERT_EQ(OBD_Crankshaft_Position_Sensor_A_Circuit_Malfunction, unitTestWarningCodeState.recentWarnings.get(1).Code) << "@0";
 }
 
 TEST(trigger, testCamInput) {
@@ -105,7 +105,7 @@ TEST(trigger, testCamInput) {
 
 	// asserting that lack of camshaft signal would be detecting
 	ASSERT_EQ(1,  unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testCamInput #2";
-	ASSERT_EQ(OBD_Camshaft_Position_Sensor_Circuit_Range_Performance, unitTestWarningCodeState.recentWarnings.get(0)) << "@0";
+	ASSERT_EQ(OBD_Camshaft_Position_Sensor_Circuit_Range_Performance, unitTestWarningCodeState.recentWarnings.get(0).Code) << "@0";
 	unitTestWarningCodeState.recentWarnings.clear();
 
 	for (int i = 0; i < 600;i++) {

unit_tests/tests/trigger/test_issue_898.cpp

@@ -19,6 +19,6 @@ TEST(issues, issue898) {
 	ASSERT_EQ(TRUE, engine->triggerCentral.triggerShape.shapeDefinitionError) << "MRE_MIATA_NA6 shapeDefinitionError";
 
 	ASSERT_EQ( 2,  eth.recentWarnings()->getCount()) << "warningCounter#testFuelSchedulerBug299smallAndMedium";
-	ASSERT_EQ(CUSTOM_ERR_BOTH_FRONTS_REQUIRED, eth.recentWarnings()->get(0));
-	ASSERT_EQ(CUSTOM_ERR_TRIGGER_SYNC, eth.recentWarnings()->get(1));
+	ASSERT_EQ(CUSTOM_ERR_BOTH_FRONTS_REQUIRED, eth.recentWarnings()->get(0).Code);
+	ASSERT_EQ(CUSTOM_ERR_TRIGGER_SYNC, eth.recentWarnings()->get(1).Code);
 }

unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp

@@ -34,7 +34,7 @@ TEST(realCrankingVQ40, normalCranking) {
 
 	// TODO: why warnings?
 	ASSERT_EQ(3, eth.recentWarnings()->getCount());
-	ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(0));
-	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(1));	// this is from a coil being protected by overdwell protection
-	ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(2));
+	ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(0).Code);
+	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(1).Code);	// this is from a coil being protected by overdwell protection
+	ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(2).Code);
 }

unit_tests/tests/trigger/test_real_nb2_cranking.cpp

@@ -29,9 +29,9 @@ TEST(realCrankingNB2, normalCranking) {
 	ASSERT_EQ(942, round(Sensor::getOrZero(SensorType::Rpm)));
 
 	ASSERT_EQ(3, eth.recentWarnings()->getCount());
-	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0));
-	ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1));
-	ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(2));
+	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code);
+	ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1).Code);
+	ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(2).Code);
 }
 
 TEST(realCrankingNB2, crankingMissingInjector) {
@@ -51,7 +51,7 @@ TEST(realCrankingNB2, crankingMissingInjector) {
 	ASSERT_EQ(668, round(Sensor::getOrZero(SensorType::Rpm)));
 
 	ASSERT_EQ(3, eth.recentWarnings()->getCount());
-	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0));
-	ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1));
-	ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(2));
+	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code);
+	ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1).Code);
+	ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(2).Code);
 }

unit_tests/tests/trigger/test_trigger_decoder.cpp

@@ -867,7 +867,7 @@ void doTestFuelSchedulerBug299smallAndMedium(int startUpDelayMs) {
 
 	ASSERT_EQ( 0,  unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testFuelSchedulerBug299smallAndMedium";
 /*
-	ASSERT_EQ(CUSTOM_OBD_SKIPPED_FUEL, unitTestWarningCodeState.recentWarnings.get(0));
+	ASSERT_EQ(CUSTOM_OBD_SKIPPED_FUEL, unitTestWarningCodeState.recentWarnings.get(0).Code);
 */
 }
 
@@ -1048,7 +1048,7 @@ TEST(big, testFuelSchedulerBug299smallAndLarge) {
 	eth.executeActions();
 	ASSERT_EQ( 0,  unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testFuelSchedulerBug299smallAndLarge";
 	/*
-	ASSERT_EQ(CUSTOM_OBD_SKIPPED_FUEL, unitTestWarningCodeState.recentWarnings.get(0));
+	ASSERT_EQ(CUSTOM_OBD_SKIPPED_FUEL, unitTestWarningCodeState.recentWarnings.get(0).Code);
 */
 }
 
@@ -1154,8 +1154,8 @@ TEST(big, testSparkReverseOrderBug319) {
 	eth.executeActions();
 	ASSERT_EQ( 0,  enginePins.coils[3].outOfOrder) << "out-of-order #8";
 	ASSERT_EQ( 2,  unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#SparkReverseOrderBug319";
-	ASSERT_EQ(CUSTOM_DWELL_TOO_LONG, unitTestWarningCodeState.recentWarnings.get(0)) << "warning @0";
-	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, unitTestWarningCodeState.recentWarnings.get(1));
+	ASSERT_EQ(CUSTOM_DWELL_TOO_LONG, unitTestWarningCodeState.recentWarnings.get(0).Code) << "warning @0";
+	ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, unitTestWarningCodeState.recentWarnings.get(1).Code);
 }
 
 TEST(big, testMissedSpark299) {