From fba0906942e8e69bdc4ccafd8636a960e55a4259 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Sun, 4 Sep 2022 06:15:24 -0700 Subject: [PATCH] more granular trigger error messages (#4526) * granular trigger error messages * adjust test expectations * explicitly test behavior --- firmware/controllers/algo/engine.cpp | 39 ++++---------- firmware/controllers/algo/engine.h | 1 - firmware/controllers/algo/obd_error_codes.h | 11 ++-- .../controllers/trigger/trigger_decoder.cpp | 27 +++++++++- .../controllers/trigger/trigger_decoder.h | 9 ++++ .../tests/trigger/test_cam_vvt_input.cpp | 4 +- .../test_real_cranking_nissan_vq40.cpp | 2 +- .../tests/trigger/test_real_nb2_cranking.cpp | 17 +++--- .../tests/trigger/test_trigger_decoder_2.cpp | 54 +++++++++++++++++++ 9 files changed, 118 insertions(+), 46 deletions(-) diff --git a/firmware/controllers/algo/engine.cpp b/firmware/controllers/algo/engine.cpp index a1f4c12d16..688da455cd 100644 --- a/firmware/controllers/algo/engine.cpp +++ b/firmware/controllers/algo/engine.cpp @@ -383,26 +383,6 @@ void Engine::preCalculate() { } #if EFI_SHAFT_POSITION_INPUT -void Engine::OnTriggerStateDecodingError() { - warning(CUSTOM_SYNC_COUNT_MISMATCH, "trigger not happy current %d/%d expected %d/%d", - triggerCentral.triggerState.currentCycle.eventCount[0], - triggerCentral.triggerState.currentCycle.eventCount[1], - TRIGGER_WAVEFORM(getExpectedEventCount(0)), - TRIGGER_WAVEFORM(getExpectedEventCount(1))); - - if (engineConfiguration->verboseTriggerSynchDetails || (triggerCentral.triggerState.someSortOfTriggerError() && !engineConfiguration->silentTriggerError)) { -#if EFI_PROD_CODE - efiPrintf("error: synchronizationPoint @ index %d expected %d/%d got %d/%d", - triggerCentral.triggerState.currentCycle.current_index, - TRIGGER_WAVEFORM(getExpectedEventCount(0)), - TRIGGER_WAVEFORM(getExpectedEventCount(1)), - triggerCentral.triggerState.currentCycle.eventCount[0], - triggerCentral.triggerState.currentCycle.eventCount[1]); -#endif /* EFI_PROD_CODE */ - } - -} - void Engine::OnTriggerStateProperState(efitick_t nowNt) { rpmCalculator.setSpinningUp(nowNt); } @@ -431,18 +411,19 @@ void Engine::OnTriggerSyncronization(bool wasSynchronized, bool isDecodingError) // 'triggerStateListener is not null' means we are running a real engine and now just preparing trigger shape // that's a bit of a hack, a sweet OOP solution would be a real callback or at least 'needDecodingErrorLogic' method? if (isDecodingError) { - OnTriggerStateDecodingError(); +#if EFI_PROD_CODE + if (engineConfiguration->verboseTriggerSynchDetails || (triggerCentral.triggerState.someSortOfTriggerError() && !engineConfiguration->silentTriggerError)) { + efiPrintf("error: synchronizationPoint @ index %d expected %d/%d got %d/%d", + triggerCentral.triggerState.currentCycle.current_index, + TRIGGER_WAVEFORM(getExpectedEventCount(0)), + TRIGGER_WAVEFORM(getExpectedEventCount(1)), + triggerCentral.triggerState.currentCycle.eventCount[0], + triggerCentral.triggerState.currentCycle.eventCount[1]); + } +#endif /* EFI_PROD_CODE */ } engine->triggerErrorDetection.add(isDecodingError); - - if (triggerCentral.isTriggerDecoderError()) { - warning(CUSTOM_OBD_TRG_DECODING, "trigger decoding issue. expected %d/%d got %d/%d", - TRIGGER_WAVEFORM(getExpectedEventCount(0)), - TRIGGER_WAVEFORM(getExpectedEventCount(1)), - triggerCentral.triggerState.currentCycle.eventCount[0], - triggerCentral.triggerState.currentCycle.eventCount[1]); - } } } diff --git a/firmware/controllers/algo/engine.h b/firmware/controllers/algo/engine.h index 8f1b710dc9..a10ef5073f 100644 --- a/firmware/controllers/algo/engine.h +++ b/firmware/controllers/algo/engine.h @@ -239,7 +239,6 @@ public: efitick_t startStopStateLastPushTime = 0; #if EFI_SHAFT_POSITION_INPUT - void OnTriggerStateDecodingError(); void OnTriggerStateProperState(efitick_t nowNt) override; void OnTriggerSyncronization(bool wasSynchronized, bool isDecodingError) override; void OnTriggerSynchronizationLost() override; diff --git a/firmware/controllers/algo/obd_error_codes.h b/firmware/controllers/algo/obd_error_codes.h index 5d3e87edaf..dce1311b99 100644 --- a/firmware/controllers/algo/obd_error_codes.h +++ b/firmware/controllers/algo/obd_error_codes.h @@ -1751,9 +1751,6 @@ typedef enum { CUSTOM_ZERO_DWELL = 6032, CUSTOM_DWELL_TOO_LONG = 6033, CUSTOM_SKIPPING_STROKE = 6034, - CUSTOM_OBD_TRG_DECODING = 6035, - // todo: looks like following two errors always happen together, it's just timing affects which one is published? - CUSTOM_SYNC_ERROR = 6036, CUSTOM_6037 = 6037, /** * This error happens if some pinout configuration changes were applied but ECU was not reset afterwards. @@ -2140,7 +2137,13 @@ typedef enum { CUSTOM_ERR_TRIGGER_SYNC = 9000, CUSTOM_OBD_TRIGGER_WAVEFORM = 9001, - CUSTOM_SYNC_COUNT_MISMATCH = 9002, + + CUSTOM_PRIMARY_TOO_MANY_TEETH = 9002, + CUSTOM_PRIMARY_NOT_ENOUGH_TEETH = 9003, + + CUSTOM_CAM_TOO_MANY_TEETH = 9004, + CUSTOM_CAM_NOT_ENOUGH_TEETH = 9005, + /** * This is not engine miss detection - this is only internal scheduler state validation * Should not happen diff --git a/firmware/controllers/trigger/trigger_decoder.cpp b/firmware/controllers/trigger/trigger_decoder.cpp index 9ae4427345..52526e3d19 100644 --- a/firmware/controllers/trigger/trigger_decoder.cpp +++ b/firmware/controllers/trigger/trigger_decoder.cpp @@ -407,6 +407,30 @@ void PrimaryTriggerDecoder::onTriggerError() { resetHasFullSync(); } +void PrimaryTriggerDecoder::onNotEnoughTeeth(int /*actual*/, int /*expected*/) { + warning(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, "primary trigger error: not enough teeth between sync points: expected %d/%d got %d/%d", + TRIGGER_WAVEFORM(getExpectedEventCount(0)), + TRIGGER_WAVEFORM(getExpectedEventCount(1)), + currentCycle.eventCount[0], + currentCycle.eventCount[1]); +} + +void PrimaryTriggerDecoder::onTooManyTeeth(int /*actual*/, int /*expected*/) { + warning(CUSTOM_PRIMARY_TOO_MANY_TEETH, "primary trigger error: too many teeth between sync points: expected %d/%d got %d/%d", + TRIGGER_WAVEFORM(getExpectedEventCount(0)), + TRIGGER_WAVEFORM(getExpectedEventCount(1)), + currentCycle.eventCount[0], + currentCycle.eventCount[1]); +} + +void VvtTriggerDecoder::onNotEnoughTeeth(int actual, int expected) { + warning(CUSTOM_CAM_NOT_ENOUGH_TEETH, "cam %s trigger error: not enough teeth between sync points: actual %d expected %d", name, actual, expected); +} + +void VvtTriggerDecoder::onTooManyTeeth(int actual, int expected) { + warning(CUSTOM_CAM_TOO_MANY_TEETH, "cam %s trigger error: too many teeth between sync points: %d > %d", name, actual, expected); +} + bool TriggerDecoderBase::validateEventCounters(const TriggerWaveform& triggerShape) const { // We can check if things are fine by comparing the number of events in a cycle with the expected number of event. bool isDecodingError = false; @@ -661,6 +685,7 @@ expected TriggerDecoderBase::decodeTriggerEvent( // so we clear the synchronized flag. if (wasSynchronized && isDecodingError) { setTriggerErrorState(); + onNotEnoughTeeth(currentCycle.current_index, triggerShape.getSize()); // Something wrong, no longer synchronized setShaftSynchronized(false); @@ -693,8 +718,8 @@ expected TriggerDecoderBase::decodeTriggerEvent( // let's not show a warning if we are just starting to spin if (Sensor::getOrZero(SensorType::Rpm) != 0) { - warning(CUSTOM_SYNC_ERROR, "sync error for %s: index #%d above total size %d", name, currentCycle.current_index, triggerShape.getSize()); setTriggerErrorState(); + onTooManyTeeth(currentCycle.current_index, triggerShape.getSize()); } onTriggerError(); diff --git a/firmware/controllers/trigger/trigger_decoder.h b/firmware/controllers/trigger/trigger_decoder.h index 0c31bd8f09..b561da248b 100644 --- a/firmware/controllers/trigger/trigger_decoder.h +++ b/firmware/controllers/trigger/trigger_decoder.h @@ -143,6 +143,9 @@ protected: // - Saw a sync point but the wrong number of events in the cycle virtual void onTriggerError() { } + virtual void onNotEnoughTeeth(int actual, int expected) { } + virtual void onTooManyTeeth(int actual, int expected) { } + private: void resetCurrentCycleState(); bool isSyncPoint(const TriggerWaveform& triggerShape, trigger_type_e triggerType) const; @@ -224,6 +227,9 @@ public: void onTriggerError() override; + void onNotEnoughTeeth(int actual, int expected) override; + void onTooManyTeeth(int actual, int expected) override; + private: float calculateInstantRpm( TriggerWaveform const & triggerShape, TriggerFormDetails *triggerFormDetails, @@ -238,6 +244,9 @@ private: class VvtTriggerDecoder : public TriggerDecoderBase { public: VvtTriggerDecoder(const char* name) : TriggerDecoderBase(name) { } + + void onNotEnoughTeeth(int actual, int expected) override; + void onTooManyTeeth(int actual, int expected) override; }; angle_t getEngineCycle(operation_mode_e operationMode); diff --git a/unit_tests/tests/trigger/test_cam_vvt_input.cpp b/unit_tests/tests/trigger/test_cam_vvt_input.cpp index 17f9e65d31..fbcf066eb1 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).Code); + EXPECT_EQ(CUSTOM_PRIMARY_TOO_MANY_TEETH, unitTestWarningCodeState.recentWarnings.get(0).Code); } TEST(trigger, testNoisyInput) { @@ -74,7 +74,7 @@ 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).Code) << "@0"; + ASSERT_EQ(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, unitTestWarningCodeState.recentWarnings.get(0).Code) << "@0"; ASSERT_EQ(OBD_Crankshaft_Position_Sensor_A_Circuit_Malfunction, unitTestWarningCodeState.recentWarnings.get(1).Code) << "@0"; } 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 1bd0053ae7..4a3dcb3a51 100644 --- a/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp +++ b/unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp @@ -36,5 +36,5 @@ TEST(realCrankingVQ40, normalCranking) { // TODO: why warnings? ASSERT_EQ(2, eth.recentWarnings()->getCount()); ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code); // this is from a coil being protected by overdwell protection - ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1).Code); + ASSERT_EQ(CUSTOM_PRIMARY_TOO_MANY_TEETH, eth.recentWarnings()->get(1).Code); } diff --git a/unit_tests/tests/trigger/test_real_nb2_cranking.cpp b/unit_tests/tests/trigger/test_real_nb2_cranking.cpp index 0a01683326..d28311e017 100644 --- a/unit_tests/tests/trigger/test_real_nb2_cranking.cpp +++ b/unit_tests/tests/trigger/test_real_nb2_cranking.cpp @@ -29,10 +29,10 @@ TEST(realCrankingNB2, normalCranking) { ASSERT_EQ(876, round(Sensor::getOrZero(SensorType::Rpm))); - ASSERT_EQ(3, eth.recentWarnings()->getCount()); - 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); + EXPECT_EQ(3, eth.recentWarnings()->getCount()); + EXPECT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code); + EXPECT_EQ(CUSTOM_CAM_TOO_MANY_TEETH, eth.recentWarnings()->get(1).Code); + EXPECT_EQ(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, eth.recentWarnings()->get(2).Code); } TEST(realCrankingNB2, crankingMissingInjector) { @@ -52,8 +52,9 @@ TEST(realCrankingNB2, crankingMissingInjector) { ASSERT_EQ(316, round(Sensor::getOrZero(SensorType::Rpm))); - ASSERT_EQ(3, eth.recentWarnings()->getCount()); - 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); + EXPECT_EQ(4, eth.recentWarnings()->getCount()); + EXPECT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0).Code); + EXPECT_EQ(CUSTOM_CAM_TOO_MANY_TEETH, eth.recentWarnings()->get(1).Code); + EXPECT_EQ(CUSTOM_PRIMARY_NOT_ENOUGH_TEETH, eth.recentWarnings()->get(2).Code); + EXPECT_EQ(CUSTOM_PRIMARY_TOO_MANY_TEETH, eth.recentWarnings()->get(3).Code); } diff --git a/unit_tests/tests/trigger/test_trigger_decoder_2.cpp b/unit_tests/tests/trigger/test_trigger_decoder_2.cpp index dc31c9699c..a816d2dc35 100644 --- a/unit_tests/tests/trigger/test_trigger_decoder_2.cpp +++ b/unit_tests/tests/trigger/test_trigger_decoder_2.cpp @@ -1,5 +1,6 @@ #include "pch.h" +using ::testing::_; using ::testing::StrictMock; class MockTriggerConfiguration : public TriggerConfiguration { @@ -32,6 +33,8 @@ struct MockTriggerDecoder : public TriggerDecoderBase { MockTriggerDecoder() : TriggerDecoderBase("mock") { } MOCK_METHOD(void, onTriggerError, (), (override)); + MOCK_METHOD(void, onNotEnoughTeeth, (int actual, int expected), (override)); + MOCK_METHOD(void, onTooManyTeeth, (int actual, int expected), (override)); }; static auto makeTriggerShape(operation_mode_e mode, const TriggerConfiguration& config) { @@ -147,6 +150,7 @@ TEST(TriggerDecoder, TooManyTeeth_CausesError) { StrictMock dut; // We expect one call to onTriggerError(). EXPECT_CALL(dut, onTriggerError()); + EXPECT_CALL(dut, onTooManyTeeth(_, _)); // Fire a few boring evenly spaced teeth t += MS2NT(1); @@ -188,6 +192,28 @@ TEST(TriggerDecoder, TooManyTeeth_CausesError) { EXPECT_EQ(6, dut.currentCycle.current_index); EXPECT_EQ(1, dut.totalTriggerErrorCounter); + // Fire some normal revolutions to ensure we recover without additional error types. + for (size_t i = 0; i < 10; i++) { + // Missing tooth, 2x normal length! + t += MS2NT(2); + doTooth(dut, shape, cfg, t); + EXPECT_TRUE(dut.getShaftSynchronized()); + EXPECT_EQ(0, dut.currentCycle.current_index); + + // normal tooth + t += MS2NT(1); + doTooth(dut, shape, cfg, t); + EXPECT_EQ(2, dut.currentCycle.current_index); + + // normal tooth + t += MS2NT(1); + doTooth(dut, shape, cfg, t); + EXPECT_EQ(4, dut.currentCycle.current_index); + } + + // Should now have sync again + EXPECT_TRUE(dut.getShaftSynchronized()); + Sensor::resetAllMocks(); } @@ -202,6 +228,7 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) { StrictMock dut; // We expect one call to onTriggerError(). EXPECT_CALL(dut, onTriggerError()); + EXPECT_CALL(dut, onNotEnoughTeeth(_, _)); // Fire a few boring evenly spaced teeth t += MS2NT(1); @@ -222,6 +249,9 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) { EXPECT_TRUE(dut.getShaftSynchronized()); EXPECT_EQ(0, dut.currentCycle.current_index); + // Fake that we have RPM so that all trigger error detection is enabled + Sensor::setMockValue(SensorType::Rpm, 1000); + t += MS2NT(1); doTooth(dut, shape, cfg, t); EXPECT_TRUE(dut.getShaftSynchronized()); @@ -238,6 +268,30 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) { EXPECT_EQ(0, dut.currentCycle.current_index); EXPECT_EQ(1, dut.totalTriggerErrorCounter); EXPECT_TRUE(dut.someSortOfTriggerError()); + + // Fire some normal revolutions to ensure we recover without additional error types. + for (size_t i = 0; i < 10; i++) { + // normal tooth + t += MS2NT(1); + doTooth(dut, shape, cfg, t); + EXPECT_EQ(2, dut.currentCycle.current_index); + + // normal tooth + t += MS2NT(1); + doTooth(dut, shape, cfg, t); + EXPECT_EQ(4, dut.currentCycle.current_index); + + // Missing tooth, 2x normal length! + t += MS2NT(2); + doTooth(dut, shape, cfg, t); + EXPECT_TRUE(dut.getShaftSynchronized()); + EXPECT_EQ(0, dut.currentCycle.current_index); + } + + // Should now have sync again + EXPECT_TRUE(dut.getShaftSynchronized()); + + Sensor::resetAllMocks(); } TEST(TriggerDecoder, PrimaryDecoderNoDisambiguation) {