diff --git a/firmware/console/status_loop.cpp b/firmware/console/status_loop.cpp index a0721c79d0..7e389ec637 100644 --- a/firmware/console/status_loop.cpp +++ b/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; diff --git a/firmware/controllers/algo/engine2.cpp b/firmware/controllers/algo/engine2.cpp index 43eda22baf..3176907ca2 100644 --- a/firmware/controllers/algo/engine2.cpp +++ b/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) { diff --git a/firmware/controllers/algo/engine_parts.h b/firmware/controllers/algo/engine_parts.h index 134d4ef586..e3ea855fdc 100644 --- a/firmware/controllers/algo/engine_parts.h +++ b/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 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 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; }; diff --git a/firmware/controllers/core/error_handling.cpp b/firmware/controllers/core/error_handling.cpp index 77af22288f..5bb451ddc7 100644 --- a/firmware/controllers/core/error_handling.cpp +++ b/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); diff --git a/firmware/util/containers/static_vector.h b/firmware/util/containers/static_vector.h new file mode 100644 index 0000000000..ca3fc88bce --- /dev/null +++ b/firmware/util/containers/static_vector.h @@ -0,0 +1,46 @@ +#pragma once + +template +struct static_vector { + + void clear() { + m_size = 0; + } + + template + T* find(const TSearch& search) const { + for (size_t i = 0; i < m_size; i++) { + if (m_storage[i] == search) { + return const_cast(&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]; +}; diff --git a/firmware/util/efitime.h b/firmware/util/efitime.h index d450a653bb..4fb2cb80c2 100644 --- a/firmware/util/efitime.h +++ b/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. diff --git a/unit_tests/tests/trigger/test_cam_vvt_input.cpp b/unit_tests/tests/trigger/test_cam_vvt_input.cpp index 30c06bac66..17f9e65d31 100644 --- a/unit_tests/tests/trigger/test_cam_vvt_input.cpp +++ b/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++) { diff --git a/unit_tests/tests/trigger/test_issue_898.cpp b/unit_tests/tests/trigger/test_issue_898.cpp index 348078366d..ea7310e8c2 100644 --- a/unit_tests/tests/trigger/test_issue_898.cpp +++ b/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); } diff --git a/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp b/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp index ead37219d6..71c6c7f15c 100644 --- a/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp +++ b/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); } diff --git a/unit_tests/tests/trigger/test_real_nb2_cranking.cpp b/unit_tests/tests/trigger/test_real_nb2_cranking.cpp index 6e21eeaccc..e9c9e2c537 100644 --- a/unit_tests/tests/trigger/test_real_nb2_cranking.cpp +++ b/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); } diff --git a/unit_tests/tests/trigger/test_trigger_decoder.cpp b/unit_tests/tests/trigger/test_trigger_decoder.cpp index 2b402096d4..36da4ef039 100644 --- a/unit_tests/tests/trigger/test_trigger_decoder.cpp +++ b/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) {