From b11c5cd8c8f8ab90ad5443a8c1ba721734073bca Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Sat, 23 May 2020 07:46:28 -0700 Subject: [PATCH] switch some firmware warnings to errors (#1465) * warnings as errors * throw instead of exit * test massaging Co-authored-by: Matthew Kennedy --- .../controllers/algo/engine_configuration.cpp | 2 +- firmware/controllers/core/error_handling.cpp | 2 +- .../controllers/engine_cycle/aux_valves.cpp | 2 +- firmware/controllers/flash_main.cpp | 2 +- firmware/controllers/gauges/tachometer.cpp | 4 +-- firmware/controllers/math/engine_math.cpp | 21 +++++++-------- .../trigger/decoders/trigger_universal.cpp | 2 +- .../controllers/trigger/trigger_decoder.cpp | 2 +- firmware/hw_layer/adc_inputs.cpp | 2 +- unit_tests/engine_test_helper.cpp | 27 +++++++++++-------- unit_tests/engine_test_helper.h | 1 + .../tests/trigger/test_trigger_decoder.cpp | 4 ++- 12 files changed, 39 insertions(+), 32 deletions(-) diff --git a/firmware/controllers/algo/engine_configuration.cpp b/firmware/controllers/algo/engine_configuration.cpp index d939d8743d..78364ee2c2 100644 --- a/firmware/controllers/algo/engine_configuration.cpp +++ b/firmware/controllers/algo/engine_configuration.cpp @@ -1398,7 +1398,7 @@ void resetConfigurationExt(Logging * logger, configuration_callback_t boardCallb break; #endif // EFI_INCLUDE_ENGINE_PRESETS default: - warning(CUSTOM_UNEXPECTED_ENGINE_TYPE, "Unexpected engine type: %d", engineType); + firmwareError(CUSTOM_UNEXPECTED_ENGINE_TYPE, "Unexpected engine type: %d", engineType); } applyNonPersistentConfiguration(logger PASS_ENGINE_PARAMETER_SUFFIX); diff --git a/firmware/controllers/core/error_handling.cpp b/firmware/controllers/core/error_handling.cpp index c4150f353b..27d96634af 100644 --- a/firmware/controllers/core/error_handling.cpp +++ b/firmware/controllers/core/error_handling.cpp @@ -271,7 +271,7 @@ void firmwareError(obd_code_e code, const char *fmt, ...) { printf("\r\n"); #if EFI_SIMULATOR || EFI_UNIT_TEST - exit(-1); + throw "fatal error"; #endif /* EFI_SIMULATOR */ #endif } diff --git a/firmware/controllers/engine_cycle/aux_valves.cpp b/firmware/controllers/engine_cycle/aux_valves.cpp index 7cd5e53eaf..ec08e2664b 100644 --- a/firmware/controllers/engine_cycle/aux_valves.cpp +++ b/firmware/controllers/engine_cycle/aux_valves.cpp @@ -126,7 +126,7 @@ void initAuxValves(Logging *sharedLogger DECLARE_ENGINE_PARAMETER_SUFFIX) { } if (!Sensor::hasSensor(SensorType::DriverThrottleIntent)) { - warning(CUSTOM_OBD_91, "No TPS for Aux Valves"); + firmwareError(CUSTOM_OBD_91, "No TPS for Aux Valves"); return; } diff --git a/firmware/controllers/flash_main.cpp b/firmware/controllers/flash_main.cpp index bebf0539a4..5fc9397398 100644 --- a/firmware/controllers/flash_main.cpp +++ b/firmware/controllers/flash_main.cpp @@ -135,7 +135,7 @@ persisted_configuration_state_e readConfiguration(Logging * logger) { } if (result == CRC_FAILED) { - warning(CUSTOM_ERR_FLASH_CRC_FAILED, "flash CRC failed"); + firmwareError(CUSTOM_ERR_FLASH_CRC_FAILED, "flash CRC failed"); resetConfigurationExt(logger, DEFAULT_ENGINE_TYPE PASS_ENGINE_PARAMETER_SUFFIX); } else if (result == INCOMPATIBLE_VERSION) { resetConfigurationExt(logger, engineConfiguration->engineType PASS_ENGINE_PARAMETER_SUFFIX); diff --git a/firmware/controllers/gauges/tachometer.cpp b/firmware/controllers/gauges/tachometer.cpp index b7a1daa5e3..cae0a8bfdb 100644 --- a/firmware/controllers/gauges/tachometer.cpp +++ b/firmware/controllers/gauges/tachometer.cpp @@ -49,8 +49,8 @@ static void tachSignalCallback(trigger_event_e ckpSignalType, // How many tach pulse periods do we have? int periods = CONFIG(tachPulsePerRev); - if(periods == 0){ - warning(CUSTOM_ERR_6709,"Check Tachometer Pulse per Rev!"); + if (periods == 0 || periods > 10){ + firmwareError(CUSTOM_ERR_6709, "Invalid tachometer pulse per rev: %d", periods); return; } diff --git a/firmware/controllers/math/engine_math.cpp b/firmware/controllers/math/engine_math.cpp index 4a36e73000..2c4de4a187 100644 --- a/firmware/controllers/math/engine_math.cpp +++ b/firmware/controllers/math/engine_math.cpp @@ -60,7 +60,7 @@ float getEngineLoadT(DECLARE_ENGINE_PARAMETER_SIGNATURE) { switch (engineConfiguration->fuelAlgorithm) { case LM_PLAIN_MAF: if (!hasMafSensor(PASS_ENGINE_PARAMETER_SIGNATURE)) { - warning(CUSTOM_MAF_NEEDED, "MAF sensor needed for current fuel algorithm"); + firmwareError(CUSTOM_MAF_NEEDED, "MAF sensor needed for current fuel algorithm"); return NAN; } return getMafVoltage(PASS_ENGINE_PARAMETER_SIGNATURE); @@ -68,11 +68,10 @@ float getEngineLoadT(DECLARE_ENGINE_PARAMETER_SIGNATURE) { return getMap(PASS_ENGINE_PARAMETER_SIGNATURE); case LM_ALPHA_N: return Sensor::get(SensorType::Tps1).value_or(0); - case LM_REAL_MAF: { + case LM_REAL_MAF: return getRealMaf(PASS_ENGINE_PARAMETER_SIGNATURE); - } default: - warning(CUSTOM_UNKNOWN_ALGORITHM, "Unexpected engine load parameter: %d", engineConfiguration->fuelAlgorithm); + firmwareError(CUSTOM_UNKNOWN_ALGORITHM, "Unexpected engine load parameter: %d", engineConfiguration->fuelAlgorithm); return -1; } } @@ -156,7 +155,7 @@ bool FuelSchedule::addFuelEventsForCylinder(int i DECLARE_ENGINE_PARAMETER_SUFF // does not look exactly right, not too consistent with IM_SEQUENTIAL injectorIndex = i % (engineConfiguration->specs.cylindersCount / 2); } else { - warning(CUSTOM_OBD_UNEXPECTED_INJECTION_MODE, "Unexpected injection mode %d", mode); + firmwareError(CUSTOM_OBD_UNEXPECTED_INJECTION_MODE, "Unexpected injection mode %d", mode); injectorIndex = 0; } @@ -166,7 +165,7 @@ bool FuelSchedule::addFuelEventsForCylinder(int i DECLARE_ENGINE_PARAMETER_SUFF int cylindersCount = CONFIG(specs.cylindersCount); if (cylindersCount < 1) { - warning(CUSTOM_OBD_ZERO_CYLINDER_COUNT, "temp cylindersCount %d", cylindersCount); + firmwareError(CUSTOM_OBD_ZERO_CYLINDER_COUNT, "Invalid cylinder count: %d", cylindersCount); return false; } @@ -362,7 +361,7 @@ static int getFiringOrderLength(DECLARE_ENGINE_PARAMETER_SIGNATURE) { return 16; default: - warning(CUSTOM_OBD_UNKNOWN_FIRING_ORDER, "getCylinderId not supported for %d", CONFIG(specs.firingOrder)); + firmwareError(CUSTOM_OBD_UNKNOWN_FIRING_ORDER, "Invalid firing order: %d", CONFIG(specs.firingOrder)); } return 1; } @@ -381,13 +380,13 @@ int getCylinderId(int index DECLARE_ENGINE_PARAMETER_SUFFIX) { return 1; } if (engineConfiguration->specs.cylindersCount != firingOrderLength) { - warning(CUSTOM_OBD_WRONG_FIRING_ORDER, "Wrong firing order %d/%d", engineConfiguration->specs.cylindersCount, firingOrderLength); + firmwareError(CUSTOM_OBD_WRONG_FIRING_ORDER, "Wrong cyl count for firing order, expected %d cylinders", firingOrderLength); return 1; } if (index < 0 || index >= firingOrderLength) { // todo: open question when does this happen? reproducible with functional tests? - warning(CUSTOM_ERR_6686, "index %d", index); + firmwareError(CUSTOM_ERR_6686, "index %d", index); return 1; } @@ -458,7 +457,7 @@ int getCylinderId(int index DECLARE_ENGINE_PARAMETER_SUFFIX) { return order_1_14_9_4_7_12_15_6_13_8_3_16_11_2_5_10[index]; default: - warning(CUSTOM_OBD_UNKNOWN_FIRING_ORDER, "getCylinderId not supported for %d", CONFIG(specs.firingOrder)); + firmwareError(CUSTOM_OBD_UNKNOWN_FIRING_ORDER, "Invalid firing order: %d", CONFIG(specs.firingOrder)); } return 1; } @@ -483,7 +482,7 @@ static int getIgnitionPinForIndex(int cylinderIndex DECLARE_ENGINE_PARAMETER_SUF return cylinderIndex % 2; default: - warning(CUSTOM_OBD_IGNITION_MODE, "unsupported ignitionMode %d in getIgnitionPinForIndex()", engineConfiguration->ignitionMode); + firmwareError(CUSTOM_OBD_IGNITION_MODE, "Invalid ignition mode getIgnitionPinForIndex(): %d", engineConfiguration->ignitionMode); return 0; } } diff --git a/firmware/controllers/trigger/decoders/trigger_universal.cpp b/firmware/controllers/trigger/decoders/trigger_universal.cpp index ac294ff383..17eae3d139 100644 --- a/firmware/controllers/trigger/decoders/trigger_universal.cpp +++ b/firmware/controllers/trigger/decoders/trigger_universal.cpp @@ -31,7 +31,7 @@ void addSkippedToothTriggerEvents(trigger_wheel_e wheel, TriggerWaveform *s, int void initializeSkippedToothTriggerWaveformExt(TriggerWaveform *s, int totalTeethCount, int skippedCount, operation_mode_e operationMode) { if (totalTeethCount <= 0) { - warning(CUSTOM_OBD_TRIGGER_WAVEFORM, "totalTeethCount is zero or less: %d", totalTeethCount); + firmwareError(CUSTOM_OBD_TRIGGER_WAVEFORM, "Invalid total tooth count for missing tooth decoder: %d", totalTeethCount); s->setShapeDefinitionError(true); return; } diff --git a/firmware/controllers/trigger/trigger_decoder.cpp b/firmware/controllers/trigger/trigger_decoder.cpp index 39ad0f7842..c5ab037088 100644 --- a/firmware/controllers/trigger/trigger_decoder.cpp +++ b/firmware/controllers/trigger/trigger_decoder.cpp @@ -133,7 +133,7 @@ void calculateTriggerSynchPoint(TriggerWaveform *shape, TriggerState *state DECL engine->engineCycleEventCount = length; efiAssertVoid(CUSTOM_SHAPE_LEN_ZERO, length > 0, "shapeLength=0"); if (length >= PWM_PHASE_MAX_COUNT) { - warning(CUSTOM_ERR_TRIGGER_WAVEFORM_TOO_LONG, "Count above %d", length); + firmwareError(CUSTOM_ERR_TRIGGER_WAVEFORM_TOO_LONG, "Trigger length above maximum: %d", length); shape->setShapeDefinitionError(true); return; } diff --git a/firmware/hw_layer/adc_inputs.cpp b/firmware/hw_layer/adc_inputs.cpp index 795746dbc8..a9d4404799 100644 --- a/firmware/hw_layer/adc_inputs.cpp +++ b/firmware/hw_layer/adc_inputs.cpp @@ -259,7 +259,7 @@ int getInternalAdcValue(const char *msg, adc_channel_e hwChannel) { return value; } if (adcHwChannelEnabled[hwChannel] != ADC_SLOW) { - warning(CUSTOM_OBD_WRONG_ADC_MODE, "ADC is off [%s] index=%d", msg, hwChannel); + firmwareError(CUSTOM_OBD_WRONG_ADC_MODE, "ADC is off [%s] index=%d", msg, hwChannel); } return slowAdc.getAdcValueByHwChannel(hwChannel); diff --git a/unit_tests/engine_test_helper.cpp b/unit_tests/engine_test_helper.cpp index eb2e56f986..6e42e69f83 100644 --- a/unit_tests/engine_test_helper.cpp +++ b/unit_tests/engine_test_helper.cpp @@ -25,7 +25,22 @@ EngineTestHelperBase::EngineTestHelperBase() { timeNowUs = 0; } -EngineTestHelper::EngineTestHelper(engine_type_e engineType, configuration_callback_t boardCallback) { +EngineTestHelper::EngineTestHelper(engine_type_e engineType, configuration_callback_t boardCallback) + : EngineTestHelper(engineType, boardCallback, {}) { +} + +EngineTestHelper::EngineTestHelper(engine_type_e engineType, const std::unordered_map& sensorValues) + : EngineTestHelper(engineType, &emptyCallbackWithConfiguration, sensorValues) { +} + +EngineTestHelper::EngineTestHelper(engine_type_e engineType, configuration_callback_t boardCallback, const std::unordered_map& sensorValues) { + Sensor::setMockValue(SensorType::Clt, 70); + Sensor::setMockValue(SensorType::Iat, 30); + + for (const auto [s, v] : sensorValues) { + Sensor::setMockValue(s, v); + } + unitTestWarningCodeState.clear(); @@ -60,19 +75,9 @@ EngineTestHelper::EngineTestHelper(engine_type_e engineType, configuration_callb engineConfiguration->mafAdcChannel = EFI_ADC_10; engine->engineState.mockAdcState.setMockVoltage(EFI_ADC_10, 0 PASS_ENGINE_PARAMETER_SUFFIX); - Sensor::setMockValue(SensorType::Clt, 70); - Sensor::setMockValue(SensorType::Iat, 30); - // this is needed to have valid CLT and IAT. //todo: reuse initPeriodicEvents(PASS_ENGINE_PARAMETER_SIGNATURE) method engine->periodicSlowCallback(PASS_ENGINE_PARAMETER_SIGNATURE); - -} - -EngineTestHelper::EngineTestHelper(engine_type_e engineType, const std::unordered_map& sensorValues) : EngineTestHelper(engineType, &emptyCallbackWithConfiguration) { - for (const auto [s, v] : sensorValues) { - Sensor::setMockValue(s, v); - } } EngineTestHelper::~EngineTestHelper() { diff --git a/unit_tests/engine_test_helper.h b/unit_tests/engine_test_helper.h index 47e75f8dba..d2a27291a1 100644 --- a/unit_tests/engine_test_helper.h +++ b/unit_tests/engine_test_helper.h @@ -31,6 +31,7 @@ class EngineTestHelper : public EngineTestHelperBase { public: EngineTestHelper(engine_type_e engineType, const std::unordered_map& sensorValues); EngineTestHelper(engine_type_e engineType, configuration_callback_t boardCallback); + EngineTestHelper(engine_type_e engineType, configuration_callback_t boardCallback, const std::unordered_map& sensorValues); ~EngineTestHelper(); void applyTriggerWaveform(); diff --git a/unit_tests/tests/trigger/test_trigger_decoder.cpp b/unit_tests/tests/trigger/test_trigger_decoder.cpp index 52f59c7115..da2ac59637 100644 --- a/unit_tests/tests/trigger/test_trigger_decoder.cpp +++ b/unit_tests/tests/trigger/test_trigger_decoder.cpp @@ -229,7 +229,9 @@ TEST(misc, testFordAspire) { static void testTriggerDecoder2(const char *msg, engine_type_e type, int synchPointIndex, float channel1duty, float channel2duty) { printf("====================================================================================== testTriggerDecoder2 msg=%s\r\n", msg); - WITH_ENGINE_TEST_HELPER(type); + // Some configs use aux valves, which requires this sensor + std::unordered_map sensorVals = {{SensorType::DriverThrottleIntent, 0}}; + WITH_ENGINE_TEST_HELPER_SENS(type, sensorVals); TriggerWaveform *t = &ENGINE(triggerCentral.triggerShape);