From b3261f25f38d3ea3680fa31802709881f0efe243 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Tue, 14 Jul 2020 23:54:41 -0700 Subject: [PATCH 1/4] add reset --- firmware/controllers/algo/event_registry.h | 2 ++ firmware/controllers/engine_cycle/rpm_calculator.cpp | 1 + firmware/controllers/math/engine_math.cpp | 6 ++++++ 3 files changed, 9 insertions(+) diff --git a/firmware/controllers/algo/event_registry.h b/firmware/controllers/algo/event_registry.h index 0284b7d9c6..b59f954acd 100644 --- a/firmware/controllers/algo/event_registry.h +++ b/firmware/controllers/algo/event_registry.h @@ -57,6 +57,8 @@ public: void addFuelEvents(DECLARE_ENGINE_PARAMETER_SIGNATURE); bool addFuelEventsForCylinder(int cylinderIndex DECLARE_ENGINE_PARAMETER_SUFFIX); + void resetOverlapping(); + /** * injection events, per cylinder */ diff --git a/firmware/controllers/engine_cycle/rpm_calculator.cpp b/firmware/controllers/engine_cycle/rpm_calculator.cpp index 29e7553f52..374f9c2718 100644 --- a/firmware/controllers/engine_cycle/rpm_calculator.cpp +++ b/firmware/controllers/engine_cycle/rpm_calculator.cpp @@ -166,6 +166,7 @@ void RpmCalculator::setRpmValue(float value DECLARE_ENGINE_PARAMETER_SUFFIX) { // This presumably fixes injection mode change for cranking-to-running transition. // 'isSimultanious' flag should be updated for events if injection modes differ for cranking and running. if (state != oldState) { + engine->injectionEvents.resetOverlapping(); engine->injectionEvents.addFuelEvents(PASS_ENGINE_PARAMETER_SIGNATURE); } #endif diff --git a/firmware/controllers/math/engine_math.cpp b/firmware/controllers/math/engine_math.cpp index eadbe4f606..6c8a3b5648 100644 --- a/firmware/controllers/math/engine_math.cpp +++ b/firmware/controllers/math/engine_math.cpp @@ -117,6 +117,12 @@ void FuelSchedule::clear() { isReady = false; } +void FuelSchedule::resetOverlapping() { + for (size_t i = 0; i < efi::size(enginePins.injectors); i++) { + enginePins.injectors[i].reset(); + } +} + /** * @returns false in case of error, true if success */ From b7336e953ed7a2cde324077fcdb52f44f06b4d78 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Thu, 16 Jul 2020 23:55:41 -0700 Subject: [PATCH 2/4] comments & improve logic --- firmware/controllers/engine_cycle/rpm_calculator.cpp | 8 +++++++- firmware/controllers/system/efi_gpio.cpp | 7 ++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/firmware/controllers/engine_cycle/rpm_calculator.cpp b/firmware/controllers/engine_cycle/rpm_calculator.cpp index 374f9c2718..a1283626f4 100644 --- a/firmware/controllers/engine_cycle/rpm_calculator.cpp +++ b/firmware/controllers/engine_cycle/rpm_calculator.cpp @@ -165,8 +165,14 @@ void RpmCalculator::setRpmValue(float value DECLARE_ENGINE_PARAMETER_SUFFIX) { #if EFI_ENGINE_CONTROL // This presumably fixes injection mode change for cranking-to-running transition. // 'isSimultanious' flag should be updated for events if injection modes differ for cranking and running. - if (state != oldState) { + if (state != oldState && CONFIG(crankingInjectionMode) != CONFIG(injectionMode)) { + // Reset the state of all injectors: when we change fueling modes, we could + // immediately reschedule an injection that's currently underway. That will cause + // the injector's overlappingCounter to get out of sync with reality. As the fix, + // every injector's state is forcibly reset just before we could cause that to happen. engine->injectionEvents.resetOverlapping(); + + // reschedule all injection events now that we've reset them engine->injectionEvents.addFuelEvents(PASS_ENGINE_PARAMETER_SIGNATURE); } #endif diff --git a/firmware/controllers/system/efi_gpio.cpp b/firmware/controllers/system/efi_gpio.cpp index 0a9a459f3c..50004bf618 100644 --- a/firmware/controllers/system/efi_gpio.cpp +++ b/firmware/controllers/system/efi_gpio.cpp @@ -277,7 +277,12 @@ bool NamedOutputPin::stop() { } void InjectorOutputPin::reset() { - overlappingCounter = 0; + // If this injector was open, close it and reset state + if (overlappingCounter != 0) { + overlappingCounter = 0; + setValue(0); + } + // todo: this could be refactored by calling some super-reset method currentLogicValue = INITIAL_PIN_STATE; } From 77294ae542c07026524834123c015d87494aeb27 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Sat, 18 Jul 2020 23:03:12 -0700 Subject: [PATCH 3/4] encapsulate overlap logic --- .../engine_cycle/main_trigger_callback.cpp | 50 +++++++++---------- firmware/controllers/system/efi_gpio.h | 8 ++- 2 files changed, 30 insertions(+), 28 deletions(-) diff --git a/firmware/controllers/engine_cycle/main_trigger_callback.cpp b/firmware/controllers/engine_cycle/main_trigger_callback.cpp index 7421083e80..c581fe24f1 100644 --- a/firmware/controllers/engine_cycle/main_trigger_callback.cpp +++ b/firmware/controllers/engine_cycle/main_trigger_callback.cpp @@ -68,13 +68,13 @@ static Logging *logger; void startSimultaniousInjection(Engine *engine) { for (int i = 0; i < engine->engineConfigurationPtr->specs.cylindersCount; i++) { - enginePins.injectors[i].setHigh(); + enginePins.injectors[i].open(); } } static void endSimultaniousInjectionOnlyTogglePins(Engine *engine) { for (int i = 0; i < engine->engineConfigurationPtr->specs.cylindersCount; i++) { - enginePins.injectors[i].setLow(); + enginePins.injectors[i].close(); } } @@ -89,29 +89,29 @@ void endSimultaniousInjection(InjectionEvent *event) { engine->injectionEvents.addFuelEventsForCylinder(event->ownIndex PASS_ENGINE_PARAMETER_SUFFIX); } -static inline void turnInjectionPinHigh(InjectorOutputPin *output) { - output->overlappingCounter++; +void InjectorOutputPin::open() { + overlappingCounter++; #if FUEL_MATH_EXTREME_LOGGING - printf("turnInjectionPinHigh %s %d %d\r\n", output->name, output->overlappingCounter, (int)getTimeNowUs()); + printf("turnInjectionPinHigh %s %d %d\r\n", name, overlappingCounter, (int)getTimeNowUs()); #endif /* FUEL_MATH_EXTREME_LOGGING */ - if (output->overlappingCounter > 1) { + if (overlappingCounter > 1) { // /** // * #299 // * this is another kind of overlap which happens in case of a small duty cycle after a large duty cycle // */ #if FUEL_MATH_EXTREME_LOGGING - printf("overlapping, no need to touch pin %s %d\r\n", output->name, (int)getTimeNowUs()); + printf("overlapping, no need to touch pin %s %d\r\n", name, (int)getTimeNowUs()); #endif /* FUEL_MATH_EXTREME_LOGGING */ } else { #if FUEL_MATH_EXTREME_LOGGING - const char * w = output->currentLogicValue == true ? "err" : ""; + const char * w = currentLogicValue == true ? "err" : ""; // scheduleMsg(&sharedLogger, "^ %spin=%s eventIndex %d %d", w, output->name, // getRevolutionCounter(), getTimeNowUs()); #endif /* FUEL_MATH_EXTREME_LOGGING */ - output->setHigh(); + setHigh(); } } @@ -126,33 +126,29 @@ void turnInjectionPinHigh(InjectionEvent *event) { InjectorOutputPin *output = event->outputs[i]; if (output) { - turnInjectionPinHigh(output); + output->open(); } } } -static inline void turnInjectionPinLow(InjectorOutputPin *output) { +void InjectorOutputPin::close() { #if FUEL_MATH_EXTREME_LOGGING - printf("turnInjectionPinLow %s %d %d\r\n", output->name, output->overlappingCounter, (int)getTimeNowUs()); + printf("turnInjectionPinLow %s %d %d\r\n", name, overlappingCounter, (int)getTimeNowUs()); #endif /* FUEL_MATH_EXTREME_LOGGING */ - + overlappingCounter--; + if (overlappingCounter > 0) { #if FUEL_MATH_EXTREME_LOGGING - const char * w = output->currentLogicValue == false ? "err" : ""; - -// scheduleMsg(&sharedLogger, "- %spin=%s eventIndex %d %d", w, output->name, -// getRevolutionCounter(), getTimeNowUs()); + printf("was overlapping, no need to touch pin %s %d\r\n", name, (int)getTimeNowUs()); #endif /* FUEL_MATH_EXTREME_LOGGING */ + } else { + setLow(); + } - output->overlappingCounter--; - if (output->overlappingCounter > 0) { -#if FUEL_MATH_EXTREME_LOGGING - printf("was overlapping, no need to touch pin %s %d\r\n", output->name, (int)getTimeNowUs()); -#endif /* FUEL_MATH_EXTREME_LOGGING */ - } else { - output->setLow(); - } - + // Don't allow negative overlap count + if (overlappingCounter < 0) { + overlappingCounter = 0; + } } void turnInjectionPinLow(InjectionEvent *event) { @@ -166,7 +162,7 @@ void turnInjectionPinLow(InjectionEvent *event) { for (int i = 0;ioutputs[i]; if (output != NULL) { - turnInjectionPinLow(output); + output->close(); } } #if EFI_UNIT_TEST diff --git a/firmware/controllers/system/efi_gpio.h b/firmware/controllers/system/efi_gpio.h index 3113c91e80..2662dfe710 100644 --- a/firmware/controllers/system/efi_gpio.h +++ b/firmware/controllers/system/efi_gpio.h @@ -103,12 +103,18 @@ public: const char *shortName = NULL; }; -class InjectorOutputPin : public NamedOutputPin { +class InjectorOutputPin final : public NamedOutputPin { public: InjectorOutputPin(); void reset(); + + void open(); + void close(); + // todo: re-implement this injectorIndex via address manipulation to reduce memory usage? int8_t injectorIndex; + +private: int8_t overlappingCounter; }; From b66b8f8542f4f6a5c0945c20c7f5e944e4d1e0ff Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Mon, 20 Jul 2020 18:47:08 -0700 Subject: [PATCH 4/4] test with fix --- .../ignition_injection/injection_mode_transition.cpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/unit_tests/tests/ignition_injection/injection_mode_transition.cpp b/unit_tests/tests/ignition_injection/injection_mode_transition.cpp index befd2578a9..bc8aa5756f 100644 --- a/unit_tests/tests/ignition_injection/injection_mode_transition.cpp +++ b/unit_tests/tests/ignition_injection/injection_mode_transition.cpp @@ -84,15 +84,10 @@ TEST(fuelControl, transitionIssue1592) { } // Check that no injectors are stuck open - // Only injector 1 should currently be open + // Injectors 1/3 should be open EXPECT_EQ(enginePins.injectors[0].getOverlappingCounter(), 1); EXPECT_EQ(enginePins.injectors[1].getOverlappingCounter(), 0); - - // !!!!!!!!! BUG !!!!!!!!!!!!!!! - // Injector #3 gets stuck open! - EXPECT_EQ(enginePins.injectors[2].getOverlappingCounter(), 2); - // !!!!!!!!! BUG !!!!!!!!!!!!!!! - + EXPECT_EQ(enginePins.injectors[2].getOverlappingCounter(), 1); EXPECT_EQ(enginePins.injectors[3].getOverlappingCounter(), 0); eth.writeEvents("fuel_schedule_transition_issue_1592.logicdata");