FOME-Tech
/
fome-fw
mirror of https://github.com/FOME-Tech/fome-fw.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

more granular trigger error messages (#4526) 

* granular trigger error messages

* adjust test expectations

* explicitly test behavior
This commit is contained in:
Matthew Kennedy 2022-09-04 06:15:24 -07:00 committed by GitHub
parent fb12c72dc4
commit fba0906942
9 changed files with 118 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
firmware/controllers/algo/engine.cpp
View File

@ -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]);
}
}
}

1
firmware/controllers/algo/engine.h
View File

@ -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;

11
firmware/controllers/algo/obd_error_codes.h
View File

@ -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

27
firmware/controllers/trigger/trigger_decoder.cpp
View File

@ -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<TriggerDecodeResult> 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<TriggerDecodeResult> 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();

9
firmware/controllers/trigger/trigger_decoder.h
View File

@ -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);

4
unit_tests/tests/trigger/test_cam_vvt_input.cpp
View File

@ -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";
}

2
unit_tests/tests/trigger/test_real_cranking_nissan_vq40.cpp
View File

@ -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);
}

17
unit_tests/tests/trigger/test_real_nb2_cranking.cpp
View File

@ -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);
}

54
unit_tests/tests/trigger/test_trigger_decoder_2.cpp
View File

@ -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<MockTriggerDecoder> 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<MockTriggerDecoder> 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) {