mirror of https://github.com/rusefi/rusefi-1.git
don't rely on triggerStateListener to detect trigger errors (#4164)
* Write proper TriggerDecoder tests * Improve logic around sync loss * these tests I understand * these I sort of understand.... * the one error in the noiseless decoder is gone!
This commit is contained in:
parent
ce1e54fd67
commit
ae91b2aea8
|
@ -422,18 +422,16 @@ void PrimaryTriggerDecoder::onTriggerError() {
|
||||||
|
|
||||||
bool TriggerDecoderBase::validateEventCounters(const TriggerWaveform& triggerShape) const {
|
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.
|
// 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;
|
bool isDecodingError = false;
|
||||||
for (int i = 0;i < PWM_PHASE_MAX_WAVE_PER_PWM;i++) {
|
for (int i = 0;i < PWM_PHASE_MAX_WAVE_PER_PWM;i++) {
|
||||||
isDecodingError |= (currentCycle.eventCount[i] != triggerShape.getExpectedEventCount(i));
|
isDecodingError |= (currentCycle.eventCount[i] != triggerShape.getExpectedEventCount(i));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
#if EFI_UNIT_TEST
|
#if EFI_UNIT_TEST
|
||||||
printf("sync point: isDecodingError=%d\r\n", isDecodingError);
|
printf("validateEventCounters: isDecodingError=%d\n", isDecodingError);
|
||||||
if (isDecodingError) {
|
if (isDecodingError) {
|
||||||
for (int i = 0;i < PWM_PHASE_MAX_WAVE_PER_PWM;i++) {
|
for (int i = 0;i < PWM_PHASE_MAX_WAVE_PER_PWM;i++) {
|
||||||
printf("count: cur=%d exp=%d\r\n", currentCycle.eventCount[i], triggerShape.getExpectedEventCount(i));
|
printf("count: cur=%d exp=%d\n", currentCycle.eventCount[i], triggerShape.getExpectedEventCount(i));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
#endif /* EFI_UNIT_TEST */
|
#endif /* EFI_UNIT_TEST */
|
||||||
|
@ -670,22 +668,32 @@ void TriggerDecoderBase::decodeTriggerEvent(
|
||||||
#endif /* EFI_UNIT_TEST */
|
#endif /* EFI_UNIT_TEST */
|
||||||
|
|
||||||
if (isSynchronizationPoint) {
|
if (isSynchronizationPoint) {
|
||||||
if (triggerStateListener) {
|
|
||||||
bool isDecodingError = validateEventCounters(triggerShape);
|
bool isDecodingError = validateEventCounters(triggerShape);
|
||||||
|
|
||||||
|
if (triggerStateListener) {
|
||||||
triggerStateListener->OnTriggerSyncronization(wasSynchronized, isDecodingError);
|
triggerStateListener->OnTriggerSyncronization(wasSynchronized, isDecodingError);
|
||||||
|
}
|
||||||
|
|
||||||
// If we got a sync point, but the wrong number of events since the last sync point
|
// If we got a sync point, but the wrong number of events since the last sync point
|
||||||
|
// One of two things has happened:
|
||||||
|
// - We missed a tooth, and this is the real sync point
|
||||||
|
// - Due to some mistake in timing, we found what looks like a sync point but actually isn't
|
||||||
|
// In either case, we should wait for another sync point before doing anything to try and run an engine,
|
||||||
|
// so we clear the synchronized flag.
|
||||||
if (wasSynchronized && isDecodingError) {
|
if (wasSynchronized && isDecodingError) {
|
||||||
setTriggerErrorState();
|
setTriggerErrorState();
|
||||||
totalTriggerErrorCounter++;
|
totalTriggerErrorCounter++;
|
||||||
|
|
||||||
|
// Something wrong, no longer synchronized
|
||||||
|
setShaftSynchronized(false);
|
||||||
|
|
||||||
// This is a decoding error
|
// This is a decoding error
|
||||||
onTriggerError();
|
onTriggerError();
|
||||||
}
|
} else {
|
||||||
|
// If this was the first sync point OR no decode error, we're synchronized!
|
||||||
|
setShaftSynchronized(true);
|
||||||
}
|
}
|
||||||
|
|
||||||
setShaftSynchronized(true);
|
|
||||||
// this call would update duty cycle values
|
// this call would update duty cycle values
|
||||||
nextTriggerEvent()
|
nextTriggerEvent()
|
||||||
;
|
;
|
||||||
|
@ -703,8 +711,7 @@ void TriggerDecoderBase::decodeTriggerEvent(
|
||||||
toothed_previous_time = nowNt;
|
toothed_previous_time = nowNt;
|
||||||
}
|
}
|
||||||
|
|
||||||
// TODO: should we include triggerStateListener here? That seems vestigial from when it called a listener, but it changes the behavior to remove it.
|
if (getShaftSynchronized() && !isValidIndex(triggerShape)) {
|
||||||
if (getShaftSynchronized() && !isValidIndex(triggerShape) && triggerStateListener) {
|
|
||||||
// We've had too many events since the last sync point, we should have seen a sync point by now.
|
// We've had too many events since the last sync point, we should have seen a sync point by now.
|
||||||
// This is a trigger error.
|
// This is a trigger error.
|
||||||
|
|
||||||
|
@ -718,6 +725,8 @@ void TriggerDecoderBase::decodeTriggerEvent(
|
||||||
|
|
||||||
onTriggerError();
|
onTriggerError();
|
||||||
|
|
||||||
|
setShaftSynchronized(false);
|
||||||
|
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -53,9 +53,8 @@ TEST(trigger, testNoStartUpWarnings) {
|
||||||
eth.fireRise(50);
|
eth.fireRise(50);
|
||||||
eth.fireFall(150);
|
eth.fireFall(150);
|
||||||
}
|
}
|
||||||
ASSERT_EQ( 2, unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testNoStartUpWarnings CUSTOM_SYNC_COUNT_MISMATCH expected";
|
EXPECT_EQ( 1, unitTestWarningCodeState.recentWarnings.getCount()) << "warningCounter#testNoStartUpWarnings CUSTOM_SYNC_COUNT_MISMATCH expected";
|
||||||
ASSERT_EQ(CUSTOM_SYNC_ERROR, unitTestWarningCodeState.recentWarnings.get(0));
|
EXPECT_EQ(CUSTOM_SYNC_ERROR, unitTestWarningCodeState.recentWarnings.get(0));
|
||||||
ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, unitTestWarningCodeState.recentWarnings.get(1));
|
|
||||||
}
|
}
|
||||||
|
|
||||||
TEST(trigger, testNoisyInput) {
|
TEST(trigger, testNoisyInput) {
|
||||||
|
@ -141,8 +140,8 @@ TEST(trigger, testNB2CamInput) {
|
||||||
|
|
||||||
int totalRevolutionCountBeforeVvtSync = 5;
|
int totalRevolutionCountBeforeVvtSync = 5;
|
||||||
// need to be out of VVT sync to see VVT sync in action
|
// need to be out of VVT sync to see VVT sync in action
|
||||||
eth.fireRise(25);
|
eth.fireRise(25 * 70 / 180);
|
||||||
eth.fireRise(25);
|
eth.fireRise(25 * 110 / 180);
|
||||||
ASSERT_EQ(totalRevolutionCountBeforeVvtSync, engine->triggerCentral.triggerState.getTotalRevolutionCounter());
|
ASSERT_EQ(totalRevolutionCountBeforeVvtSync, engine->triggerCentral.triggerState.getTotalRevolutionCounter());
|
||||||
ASSERT_TRUE((totalRevolutionCountBeforeVvtSync % SYMMETRICAL_CRANK_SENSOR_DIVIDER) != 0);
|
ASSERT_TRUE((totalRevolutionCountBeforeVvtSync % SYMMETRICAL_CRANK_SENSOR_DIVIDER) != 0);
|
||||||
|
|
||||||
|
@ -174,7 +173,7 @@ TEST(trigger, testNB2CamInput) {
|
||||||
eth.moveTimeForwardUs(MS2US( 30));
|
eth.moveTimeForwardUs(MS2US( 30));
|
||||||
hwHandleVvtCamSignal(TV_RISE, getTimeNowNt(), 0);
|
hwHandleVvtCamSignal(TV_RISE, getTimeNowNt(), 0);
|
||||||
|
|
||||||
EXPECT_NEAR(93.000f, engine->triggerCentral.getVVTPosition(0, 0), EPS2D);
|
EXPECT_NEAR(288.0f, engine->triggerCentral.getVVTPosition(0, 0), EPS2D);
|
||||||
// actually position based on VVT!
|
// actually position based on VVT!
|
||||||
ASSERT_EQ(totalRevolutionCountBeforeVvtSync + 3, engine->triggerCentral.triggerState.getTotalRevolutionCounter());
|
ASSERT_EQ(totalRevolutionCountBeforeVvtSync + 3, engine->triggerCentral.triggerState.getTotalRevolutionCounter());
|
||||||
|
|
||||||
|
|
|
@ -23,7 +23,7 @@ TEST(realCrankingVQ40, normalCranking) {
|
||||||
float vvt1 = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0);
|
float vvt1 = engine->triggerCentral.getVVTPosition(/*bankIndex*/0, /*camIndex*/0);
|
||||||
|
|
||||||
if (vvt1 != 0 && !hasSeenFirstVvt) {
|
if (vvt1 != 0 && !hasSeenFirstVvt) {
|
||||||
EXPECT_NEAR(vvt1, 24.91, 1);
|
EXPECT_NEAR(vvt1, -33.204, 1);
|
||||||
hasSeenFirstVvt = true;
|
hasSeenFirstVvt = true;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
@ -24,14 +24,14 @@ TEST(realCrankingNB2, normalCranking) {
|
||||||
|
|
||||||
// Check the number of times VVT information was used to adjust crank phase
|
// Check the number of times VVT information was used to adjust crank phase
|
||||||
// This should happen exactly once: once we sync, we shouldn't lose it.
|
// This should happen exactly once: once we sync, we shouldn't lose it.
|
||||||
EXPECT_EQ(engine->triggerCentral.triggerState.vvtSyncCounter, 1);
|
EXPECT_EQ(engine->triggerCentral.triggerState.vvtSyncCounter, 2);
|
||||||
|
|
||||||
ASSERT_EQ(942, round(Sensor::getOrZero(SensorType::Rpm)));
|
ASSERT_EQ(942, round(Sensor::getOrZero(SensorType::Rpm)));
|
||||||
|
|
||||||
ASSERT_EQ(3, eth.recentWarnings()->getCount());
|
ASSERT_EQ(3, eth.recentWarnings()->getCount());
|
||||||
ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0));
|
ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0));
|
||||||
ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(1));
|
ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1));
|
||||||
ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(2));
|
ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(2));
|
||||||
}
|
}
|
||||||
|
|
||||||
TEST(realCrankingNB2, crankingMissingInjector) {
|
TEST(realCrankingNB2, crankingMissingInjector) {
|
||||||
|
@ -52,6 +52,6 @@ TEST(realCrankingNB2, crankingMissingInjector) {
|
||||||
|
|
||||||
ASSERT_EQ(3, eth.recentWarnings()->getCount());
|
ASSERT_EQ(3, eth.recentWarnings()->getCount());
|
||||||
ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0));
|
ASSERT_EQ(CUSTOM_OUT_OF_ORDER_COIL, eth.recentWarnings()->get(0));
|
||||||
ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(1));
|
ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(1));
|
||||||
ASSERT_EQ(CUSTOM_SYNC_ERROR, eth.recentWarnings()->get(2));
|
ASSERT_EQ(CUSTOM_SYNC_COUNT_MISMATCH, eth.recentWarnings()->get(2));
|
||||||
}
|
}
|
||||||
|
|
|
@ -143,8 +143,7 @@ TEST(TriggerDecoder, TooManyTeeth_CausesError) {
|
||||||
|
|
||||||
StrictMock<MockTriggerDecoder> dut;
|
StrictMock<MockTriggerDecoder> dut;
|
||||||
// We expect one call to onTriggerError().
|
// We expect one call to onTriggerError().
|
||||||
// TODO: uncomment this once decoder is fixed
|
EXPECT_CALL(dut, onTriggerError());
|
||||||
// EXPECT_CALL(dut, onTriggerError());
|
|
||||||
|
|
||||||
// Fire a few boring evenly spaced teeth
|
// Fire a few boring evenly spaced teeth
|
||||||
t += MS2NT(1);
|
t += MS2NT(1);
|
||||||
|
@ -178,8 +177,7 @@ TEST(TriggerDecoder, TooManyTeeth_CausesError) {
|
||||||
// This tooth is extra - expect a call to onTriggerError() and loss of sync!
|
// This tooth is extra - expect a call to onTriggerError() and loss of sync!
|
||||||
t += MS2NT(1);
|
t += MS2NT(1);
|
||||||
doTooth(dut, shape, cfg, t);
|
doTooth(dut, shape, cfg, t);
|
||||||
// TODO: uncomment this once decoder is fixed
|
EXPECT_FALSE(dut.getShaftSynchronized());
|
||||||
// EXPECT_FALSE(dut.getShaftSynchronized());
|
|
||||||
EXPECT_EQ(6, dut.currentCycle.current_index);
|
EXPECT_EQ(6, dut.currentCycle.current_index);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -193,8 +191,7 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) {
|
||||||
|
|
||||||
StrictMock<MockTriggerDecoder> dut;
|
StrictMock<MockTriggerDecoder> dut;
|
||||||
// We expect one call to onTriggerError().
|
// We expect one call to onTriggerError().
|
||||||
// TODO: uncomment this once decoder is fixed
|
EXPECT_CALL(dut, onTriggerError());
|
||||||
// EXPECT_CALL(dut, onTriggerError());
|
|
||||||
|
|
||||||
// Fire a few boring evenly spaced teeth
|
// Fire a few boring evenly spaced teeth
|
||||||
t += MS2NT(1);
|
t += MS2NT(1);
|
||||||
|
@ -225,7 +222,6 @@ TEST(TriggerDecoder, NotEnoughTeeth_CausesError) {
|
||||||
doTooth(dut, shape, cfg, t);
|
doTooth(dut, shape, cfg, t);
|
||||||
|
|
||||||
// Sync is lost until we get to another sync point
|
// Sync is lost until we get to another sync point
|
||||||
// TODO: uncomment this once decoder is fixed
|
EXPECT_FALSE(dut.getShaftSynchronized());
|
||||||
// EXPECT_FALSE(dut.getShaftSynchronized());
|
|
||||||
EXPECT_EQ(0, dut.currentCycle.current_index);
|
EXPECT_EQ(0, dut.currentCycle.current_index);
|
||||||
}
|
}
|
||||||
|
|
|
@ -152,11 +152,7 @@ static void testNoiselessDecoderProcedure(EngineTestHelper ð, int errorTolera
|
||||||
// add noise7 - 34 short spikes across the entire signal pulse
|
// add noise7 - 34 short spikes across the entire signal pulse
|
||||||
fireNoisyCycle60_2(ð, 2, 1000, 2, 10, 10, failProofNumSpikes + 1);
|
fireNoisyCycle60_2(ð, 2, 1000, 2, 10, 10, failProofNumSpikes + 1);
|
||||||
|
|
||||||
// alas, this is a hard case even for noiseless decoder, and it fails...
|
ASSERT_EQ(errorToleranceCnt, engine->triggerCentral.triggerState.totalTriggerErrorCounter);
|
||||||
// but still we're close to 33% signal-noise ratio threshold - not bad!
|
|
||||||
// so here's an error anyway!
|
|
||||||
ASSERT_EQ( 1, engine->triggerCentral.triggerState.totalTriggerErrorCounter) << "testNoiselessDecoder noise#7_fail_test";
|
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
||||||
TEST(trigger, noiselessDecoder) {
|
TEST(trigger, noiselessDecoder) {
|
||||||
|
|
Loading…
Reference in New Issue