From 70c2689f1e40ae3f3efc11c6c26bf0d0916bd81a Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Mon, 30 May 2022 19:39:57 -0700 Subject: [PATCH] remove trigger duty cycle calculation (#4213) * This field was ignored. * move pad out * gone * make trigger configuration a little clearer * even simpler! * format * test fix * remove duty cycle thing --- .../trigger/decoders/trigger_structure.cpp | 1 - .../trigger/decoders/trigger_structure.h | 6 --- .../controllers/trigger/trigger_central.cpp | 3 -- .../controllers/trigger/trigger_decoder.cpp | 49 +------------------ .../controllers/trigger/trigger_decoder.h | 21 -------- .../controllers/trigger/trigger_simulator.cpp | 19 ++----- .../controllers/trigger/trigger_simulator.h | 5 +- .../tests/trigger/test_all_triggers.cpp | 1 - .../tests/trigger/test_cam_vvt_input.cpp | 4 -- .../tests/trigger/test_trigger_decoder.cpp | 19 +++---- .../tests/trigger/test_trigger_decoder_2.cpp | 2 +- 11 files changed, 17 insertions(+), 113 deletions(-) diff --git a/firmware/controllers/trigger/decoders/trigger_structure.cpp b/firmware/controllers/trigger/decoders/trigger_structure.cpp index 52bb1b6141..7f0cd4a0de 100644 --- a/firmware/controllers/trigger/decoders/trigger_structure.cpp +++ b/firmware/controllers/trigger/decoders/trigger_structure.cpp @@ -59,7 +59,6 @@ void TriggerWaveform::initialize(operation_mode_e operationMode) { isSecondWheelCam = false; needSecondTriggerInput = false; shapeWithoutTdc = false; - memset(expectedDutyCycle, 0, sizeof(expectedDutyCycle)); setTriggerSynchronizationGap(2); for (int gapIndex = 1; gapIndex < GAP_TRACKING_LENGTH ; gapIndex++) { diff --git a/firmware/controllers/trigger/decoders/trigger_structure.h b/firmware/controllers/trigger/decoders/trigger_structure.h index 10770a7de0..09472b9e42 100644 --- a/firmware/controllers/trigger/decoders/trigger_structure.h +++ b/firmware/controllers/trigger/decoders/trigger_structure.h @@ -121,12 +121,6 @@ public: */ int version = 0; - /** - * duty cycle for each individual trigger channel - */ - float expectedDutyCycle[PWM_PHASE_MAX_WAVE_PER_PWM]; - - /** * Depending on trigger shape, we use betweeb one and three previous gap ranges to detect synchronizaiton. * diff --git a/firmware/controllers/trigger/trigger_central.cpp b/firmware/controllers/trigger/trigger_central.cpp index be56ce3bd7..174c3f326d 100644 --- a/firmware/controllers/trigger/trigger_central.cpp +++ b/firmware/controllers/trigger/trigger_central.cpp @@ -299,7 +299,6 @@ void hwHandleVvtCamSignal(trigger_value_e front, efitick_t nowNt, int index) { "vvt", tc->vvtShape[camIndex], nullptr, - nullptr, engine->vvtTriggerConfiguration[camIndex], front == TV_RISE ? SHAFT_PRIMARY_RISING : SHAFT_PRIMARY_FALLING, nowNt); // yes we log data from all VVT channels into same fields for now @@ -677,7 +676,6 @@ void TriggerCentral::handleShaftSignal(trigger_event_e signal, efitick_t timesta auto decodeResult = triggerState.decodeTriggerEvent( "trigger", triggerShape, - nullptr, engine, engine->primaryTriggerConfiguration, signal, timestamp); @@ -799,7 +797,6 @@ void triggerInfo(void) { efiPrintf("trigger type=%d/need2ndChannel=%s", engineConfiguration->trigger.type, boolToString(TRIGGER_WAVEFORM(needSecondTriggerInput))); - efiPrintf("expected duty #0=%.2f/#1=%.2f", TRIGGER_WAVEFORM(expectedDutyCycle[0]), TRIGGER_WAVEFORM(expectedDutyCycle[1])); efiPrintf("synchronizationNeeded=%s/isError=%s/total errors=%d ord_err=%d/total revolutions=%d/self=%s", boolToString(ts->isSynchronizationNeeded), diff --git a/firmware/controllers/trigger/trigger_decoder.cpp b/firmware/controllers/trigger/trigger_decoder.cpp index de5840f9b3..23c35ab212 100644 --- a/firmware/controllers/trigger/trigger_decoder.cpp +++ b/firmware/controllers/trigger/trigger_decoder.cpp @@ -74,7 +74,6 @@ void TriggerDecoderBase::resetTriggerState() { startOfCycleNt = 0; resetCurrentCycleState(); - memset(expectedTotalTime, 0, sizeof(expectedTotalTime)); totalEventCountBase = 0; isFirstEvent = true; @@ -86,11 +85,6 @@ void TriggerDecoderBase::setTriggerErrorState() { void TriggerDecoderBase::resetCurrentCycleState() { memset(currentCycle.eventCount, 0, sizeof(currentCycle.eventCount)); - memset(currentCycle.timeOfPreviousEventNt, 0, sizeof(currentCycle.timeOfPreviousEventNt)); -#if EFI_UNIT_TEST - memcpy(currentCycle.totalTimeNtCopy, currentCycle.totalTimeNt, sizeof(currentCycle.totalTimeNt)); -#endif - memset(currentCycle.totalTimeNt, 0, sizeof(currentCycle.totalTimeNt)); currentCycle.current_index = 0; } @@ -349,15 +343,6 @@ static trigger_value_e eventType[6] = { TV_FALL, TV_RISE, TV_FALL, TV_RISE, TV_F #define nextTriggerEvent() \ { \ - uint32_t prevTime = currentCycle.timeOfPreviousEventNt[triggerWheel]; \ - if (prevTime != 0) { \ - /* even event - apply the value*/ \ - currentCycle.totalTimeNt[triggerWheel] += (nowNt - prevTime); \ - currentCycle.timeOfPreviousEventNt[triggerWheel] = 0; \ - } else { \ - /* odd event - start accumulation */ \ - currentCycle.timeOfPreviousEventNt[triggerWheel] = nowNt; \ - } \ if (triggerConfiguration.UseOnlyRisingEdgeForTrigger) {currentCycle.current_index++;} \ currentCycle.current_index++; \ PRINT_INC_INDEX; \ @@ -435,16 +420,9 @@ bool TriggerDecoderBase::validateEventCounters(const TriggerWaveform& triggerSha } void TriggerDecoderBase::onShaftSynchronization( - const TriggerStateCallback triggerCycleCallback, bool wasSynchronized, const efitick_t nowNt, const TriggerWaveform& triggerShape) { - - - if (triggerCycleCallback) { - triggerCycleCallback(this); - } - startOfCycleNt = nowNt; resetCurrentCycleState(); @@ -477,7 +455,6 @@ void TriggerDecoderBase::onShaftSynchronization( expected TriggerDecoderBase::decodeTriggerEvent( const char *msg, const TriggerWaveform& triggerShape, - const TriggerStateCallback triggerCycleCallback, TriggerStateListener* triggerStateListener, const TriggerConfiguration& triggerConfiguration, const trigger_event_e signal, @@ -582,14 +559,6 @@ expected TriggerDecoderBase::decodeTriggerEvent( int rpm = Sensor::getOrZero(SensorType::Rpm); floatms_t engineCycleDuration = getEngineCycleDuration(rpm); - if (!engineConfiguration->useOnlyRisingEdgeForTrigger) { - int time = currentCycle.totalTimeNt[0]; - efiPrintf("%s duty %f %d", - name, - time / engineCycleDuration, - time - ); - } for (int i = 0;i TriggerDecoderBase::decodeTriggerEvent( nextTriggerEvent() ; - onShaftSynchronization(triggerCycleCallback, wasSynchronized, nowNt, triggerShape); + onShaftSynchronization(wasSynchronized, nowNt, triggerShape); } else { /* if (!isSynchronizationPoint) */ nextTriggerEvent() ; @@ -791,13 +760,6 @@ bool TriggerDecoderBase::isSyncPoint(const TriggerWaveform& triggerShape, trigge return true; } -static void onFindIndexCallback(TriggerDecoderBase *state) { - for (int i = 0; i < PWM_PHASE_MAX_WAVE_PER_PWM; i++) { - // todo: that's not the best place for this intermediate data storage, fix it! - state->expectedTotalTime[i] = state->currentCycle.totalTimeNt[i]; - } -} - /** * Trigger shape is defined in a way which is convenient for trigger shape definition * On the other hand, trigger decoder indexing begins from synchronization event. @@ -836,14 +798,7 @@ uint32_t TriggerDecoderBase::findTriggerZeroEventIndex( } #endif /* EFI_UNIT_TEST */ - /** - * Now that we have just located the synch point, we can simulate the whole cycle - * in order to calculate expected duty cycle - * - * todo: add a comment why are we doing '2 * shape->getSize()' here? - */ - - helper.assertSyncPositionAndSetDutyCycle(onFindIndexCallback, triggerConfiguration, + helper.assertSyncPosition(triggerConfiguration, syncIndex, *this, shape); return syncIndex % shape.getSize(); diff --git a/firmware/controllers/trigger/trigger_decoder.h b/firmware/controllers/trigger/trigger_decoder.h index 673bcea82f..67bd954890 100644 --- a/firmware/controllers/trigger/trigger_decoder.h +++ b/firmware/controllers/trigger/trigger_decoder.h @@ -39,8 +39,6 @@ protected: virtual trigger_config_s getType() const = 0; }; -typedef void (*TriggerStateCallback)(TriggerDecoderBase*); - typedef struct { /** * index within trigger revolution, from 0 to trigger event count @@ -52,22 +50,7 @@ typedef struct { * see TriggerWaveform */ size_t eventCount[PWM_PHASE_MAX_WAVE_PER_PWM]; - /** - * This array is used to calculate duty cycle of each trigger channel. - * Current implementation is a bit funny - it does not really consider if an event - * is a rise or a fall, it works based on the event order within synchronization cycle. - * - * 32 bit value is good enough here, overflows will happen but they would work just fine. - */ - uint32_t timeOfPreviousEventNt[PWM_PHASE_MAX_WAVE_PER_PWM]; - /** - * Here we accumulate the amount of time this signal was ON within current trigger cycle - */ - uint32_t totalTimeNt[PWM_PHASE_MAX_WAVE_PER_PWM]; -#if EFI_UNIT_TEST - uint32_t totalTimeNtCopy[PWM_PHASE_MAX_WAVE_PER_PWM]; -#endif // EFI_UNIT_TEST } current_cycle_state_s; struct TriggerDecodeResult { @@ -95,14 +78,12 @@ public: expected decodeTriggerEvent( const char *msg, const TriggerWaveform& triggerShape, - const TriggerStateCallback triggerCycleCallback, TriggerStateListener* triggerStateListener, const TriggerConfiguration& triggerConfiguration, const trigger_event_e signal, const efitime_t nowUs); void onShaftSynchronization( - const TriggerStateCallback triggerCycleCallback, bool wasSynchronized, const efitick_t nowNt, const TriggerWaveform& triggerShape); @@ -129,8 +110,6 @@ public: current_cycle_state_s currentCycle; const char *name = nullptr; - int expectedTotalTime[PWM_PHASE_MAX_WAVE_PER_PWM]; - /** * how many times since ECU reboot we had unexpected number of teeth in trigger cycle */ diff --git a/firmware/controllers/trigger/trigger_simulator.cpp b/firmware/controllers/trigger/trigger_simulator.cpp index 7f1d76cc31..2485b31c24 100644 --- a/firmware/controllers/trigger/trigger_simulator.cpp +++ b/firmware/controllers/trigger/trigger_simulator.cpp @@ -37,7 +37,6 @@ int getSimulatedEventTime(const TriggerWaveform& shape, int i) { } void TriggerStimulatorHelper::feedSimulatedEvent( - const TriggerStateCallback triggerCycleCallback, const TriggerConfiguration& triggerConfiguration, TriggerDecoderBase& state, const TriggerWaveform& shape, @@ -83,7 +82,6 @@ void TriggerStimulatorHelper::feedSimulatedEvent( state.decodeTriggerEvent( "sim", shape, - triggerCycleCallback, /* override */ nullptr, triggerConfiguration, event, time); @@ -93,8 +91,7 @@ void TriggerStimulatorHelper::feedSimulatedEvent( } -void TriggerStimulatorHelper::assertSyncPositionAndSetDutyCycle( - const TriggerStateCallback triggerCycleCallback, +void TriggerStimulatorHelper::assertSyncPosition( const TriggerConfiguration& triggerConfiguration, const uint32_t syncIndex, TriggerDecoderBase& state, @@ -108,10 +105,9 @@ void TriggerStimulatorHelper::assertSyncPositionAndSetDutyCycle( * let's feed two more cycles to validate shape definition */ for (uint32_t i = syncIndex + 1; i <= syncIndex + TEST_REVOLUTIONS * shape.getSize(); i++) { - feedSimulatedEvent(triggerCycleCallback, - triggerConfiguration, - state, shape, i); + feedSimulatedEvent(triggerConfiguration, state, shape, i); } + int revolutionCounter = state.getTotalRevolutionCounter(); if (revolutionCounter != TEST_REVOLUTIONS) { warning(CUSTOM_OBD_TRIGGER_WAVEFORM, "sync failed/wrong gap parameters trigger=%s revolutionCounter=%d", @@ -128,11 +124,6 @@ void TriggerStimulatorHelper::assertSyncPositionAndSetDutyCycle( revolutionCounter); } #endif /* EFI_UNIT_TEST */ - - - for (int i = 0; i < PWM_PHASE_MAX_WAVE_PER_PWM; i++) { - shape.expectedDutyCycle[i] = 1.0 * state.expectedTotalTime[i] / SIMULATION_CYCLE_PERIOD; - } } /** @@ -143,9 +134,7 @@ uint32_t TriggerStimulatorHelper::findTriggerSyncPoint( const TriggerConfiguration& triggerConfiguration, TriggerDecoderBase& state) { for (int i = 0; i < 4 * PWM_PHASE_MAX_COUNT; i++) { - feedSimulatedEvent(nullptr, - triggerConfiguration, - state, shape, i); + feedSimulatedEvent(triggerConfiguration, state, shape, i); if (state.getShaftSynchronized()) { return i; diff --git a/firmware/controllers/trigger/trigger_simulator.h b/firmware/controllers/trigger/trigger_simulator.h index 8469e2be0b..f7c0bce13a 100644 --- a/firmware/controllers/trigger/trigger_simulator.h +++ b/firmware/controllers/trigger/trigger_simulator.h @@ -22,8 +22,7 @@ public: const TriggerConfiguration& triggerConfiguration, TriggerDecoderBase& state); - void assertSyncPositionAndSetDutyCycle( - const TriggerStateCallback triggerCycleCallback, + void assertSyncPosition( const TriggerConfiguration& triggerConfiguration, const uint32_t index, TriggerDecoderBase& state, @@ -31,7 +30,7 @@ public: ); // send next event so that we can see how state reacts - void feedSimulatedEvent(const TriggerStateCallback triggerCycleCallback, + void feedSimulatedEvent( const TriggerConfiguration& triggerConfiguration, TriggerDecoderBase& state, const TriggerWaveform& shape, diff --git a/unit_tests/tests/trigger/test_all_triggers.cpp b/unit_tests/tests/trigger/test_all_triggers.cpp index 2ce7a20c57..815d3c7fb0 100644 --- a/unit_tests/tests/trigger/test_all_triggers.cpp +++ b/unit_tests/tests/trigger/test_all_triggers.cpp @@ -73,7 +73,6 @@ TEST_P(AllTriggersFixture, TestTrigger) { fprintf(fp, "%s=%s\n", TRIGGER_HAS_SECOND_CHANNEL, shape->needSecondTriggerInput ? "true" : "false"); fprintf(fp, "%s=%s\n", TRIGGER_IS_SECOND_WHEEL_CAM, shape->isSecondWheelCam ? "true" : "false"); - fprintf(fp, "# duty %.2f %.2f\n", shape->expectedDutyCycle[0], shape->expectedDutyCycle[1]); for (size_t i = 0; i < shape->getLength(); i++) { int triggerDefinitionCoordinate = (shape->getTriggerWaveformSynchPointIndex() + i) % shape->getSize(); diff --git a/unit_tests/tests/trigger/test_cam_vvt_input.cpp b/unit_tests/tests/trigger/test_cam_vvt_input.cpp index c63c5540d4..19f7be9bd8 100644 --- a/unit_tests/tests/trigger/test_cam_vvt_input.cpp +++ b/unit_tests/tests/trigger/test_cam_vvt_input.cpp @@ -177,9 +177,5 @@ TEST(trigger, testNB2CamInput) { // actually position based on VVT! ASSERT_EQ(totalRevolutionCountBeforeVvtSync + 3, engine->triggerCentral.triggerState.getTotalRevolutionCounter()); - float dutyCycleNt = engine->triggerCentral.vvtState[0][0].currentCycle.totalTimeNtCopy[0]; - EXPECT_FLOAT_EQ(27'000'000, dutyCycleNt); - EXPECT_FLOAT_EQ(0.056944445f, engine->triggerCentral.vvtShape[0].expectedDutyCycle[0]); - EXPECT_EQ(40, waveChart.getSize()); } diff --git a/unit_tests/tests/trigger/test_trigger_decoder.cpp b/unit_tests/tests/trigger/test_trigger_decoder.cpp index 200747a016..786ca27b80 100644 --- a/unit_tests/tests/trigger/test_trigger_decoder.cpp +++ b/unit_tests/tests/trigger/test_trigger_decoder.cpp @@ -119,28 +119,28 @@ TEST(trigger, testSomethingWeird) { ASSERT_FALSE(sta->shaft_is_synchronized) << "shaft_is_synchronized"; int r = 10; - sta->decodeTriggerEvent("t", engine->triggerCentral.triggerShape, nullptr, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_FALLING, r); + sta->decodeTriggerEvent("t", engine->triggerCentral.triggerShape, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_FALLING, r); ASSERT_FALSE(sta->shaft_is_synchronized) << "shaft_is_synchronized"; // still no synchronization - sta->decodeTriggerEvent("t", engine->triggerCentral.triggerShape, nullptr, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_RISING, ++r); + sta->decodeTriggerEvent("t", engine->triggerCentral.triggerShape, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_RISING, ++r); ASSERT_TRUE(sta->shaft_is_synchronized); // first signal rise synchronize ASSERT_EQ(0, sta->getCurrentIndex()); - sta->decodeTriggerEvent("t", engine->triggerCentral.triggerShape, nullptr, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_FALLING, r++); + sta->decodeTriggerEvent("t", engine->triggerCentral.triggerShape, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_FALLING, r++); ASSERT_EQ(1, sta->getCurrentIndex()); for (int i = 2; i < 10;) { - sta->decodeTriggerEvent("t", engine->triggerCentral.triggerShape, nullptr, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_RISING, r++); + sta->decodeTriggerEvent("t", engine->triggerCentral.triggerShape, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_RISING, r++); assertEqualsM("even", i++, sta->getCurrentIndex()); - sta->decodeTriggerEvent("t", engine->triggerCentral.triggerShape, nullptr, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_FALLING, r++); + sta->decodeTriggerEvent("t", engine->triggerCentral.triggerShape, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_FALLING, r++); assertEqualsM("odd", i++, sta->getCurrentIndex()); } - sta->decodeTriggerEvent("test", engine->triggerCentral.triggerShape, nullptr, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_RISING, r++); + sta->decodeTriggerEvent("test", engine->triggerCentral.triggerShape, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_RISING, r++); ASSERT_EQ(10, sta->getCurrentIndex()); - sta->decodeTriggerEvent("test", engine->triggerCentral.triggerShape, nullptr, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_FALLING, r++); + sta->decodeTriggerEvent("test", engine->triggerCentral.triggerShape, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_FALLING, r++); ASSERT_EQ(11, sta->getCurrentIndex()); - sta->decodeTriggerEvent("test", engine->triggerCentral.triggerShape, nullptr, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_RISING, r++); + sta->decodeTriggerEvent("test", engine->triggerCentral.triggerShape, /* override */ nullptr, triggerConfiguration, SHAFT_PRIMARY_RISING, r++); ASSERT_EQ(0, sta->getCurrentIndex()); // new revolution } @@ -236,9 +236,6 @@ static void testTriggerDecoder2(const char *msg, engine_type_e type, int synchPo ASSERT_FALSE(t->shapeDefinitionError) << "isError"; assertEqualsM("synchPointIndex", synchPointIndex, t->getTriggerWaveformSynchPointIndex()); - - ASSERT_NEAR(channel1duty, t->expectedDutyCycle[0], 0.0001) << msg << " channel1duty"; - ASSERT_NEAR(channel2duty, t->expectedDutyCycle[1], 0.0001) << msg << " channel2duty"; } static void testTriggerDecoder3(const char *msg, engine_type_e type, int synchPointIndex, float channel1duty, float channel2duty, float expectedGap) { diff --git a/unit_tests/tests/trigger/test_trigger_decoder_2.cpp b/unit_tests/tests/trigger/test_trigger_decoder_2.cpp index 9265e3ba6f..1a90652998 100644 --- a/unit_tests/tests/trigger/test_trigger_decoder_2.cpp +++ b/unit_tests/tests/trigger/test_trigger_decoder_2.cpp @@ -39,7 +39,7 @@ static auto makeTriggerShape(operation_mode_e mode, const TriggerConfiguration& return shape; } -#define doTooth(dut, shape, cfg, t) dut.decodeTriggerEvent("", shape, nullptr, nullptr, cfg, SHAFT_PRIMARY_RISING, t) +#define doTooth(dut, shape, cfg, t) dut.decodeTriggerEvent("", shape, nullptr, cfg, SHAFT_PRIMARY_RISING, t) TEST(TriggerDecoder, FindsFirstSyncPoint) { MockTriggerConfiguration cfg(true, {TT_TOOTHED_WHEEL, 4, 1});