From 923d2c2e44f986d3c36f2db08a0338552cdd7c43 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Thu, 28 Jul 2022 00:04:28 -0700 Subject: [PATCH] unexpected contains information about why it failed (#4393) * unexpected can contain information * info printing * sensors return appropriate error codes * remove reliance on undefined behavior * s --- .../controllers/actuators/fan_control.cpp | 8 +++--- firmware/controllers/algo/advance_map.cpp | 12 ++++---- firmware/controllers/algo/fuel/dfco.cpp | 14 +++++----- firmware/controllers/algo/fuel_math.cpp | 18 ++++++------ .../controllers/engine_cycle/aux_valves.cpp | 8 +++--- .../engine_cycle/high_pressure_fuel_pump.cpp | 9 ++++-- .../sensors/converters/flex_sensor.h | 16 +++++++---- .../sensors/converters/linear_func.cpp | 12 +++++--- .../sensors/converters/resistance_func.cpp | 4 +-- .../sensors/converters/thermistor_func.cpp | 13 ++++++--- .../controllers/sensors/functional_sensor.cpp | 4 +-- .../sensors/redundant_ford_tps.cpp | 6 ++-- .../controllers/sensors/redundant_sensor.cpp | 4 +-- firmware/controllers/sensors/sensor.cpp | 6 ++-- .../sensors/sensor_info_printing.cpp | 16 +++++------ .../controllers/sensors/stored_value_sensor.h | 27 +++++++++--------- firmware/util/expected.h | 28 +++++++++++++++++-- unit_tests/tests/sensor/func_sensor.cpp | 1 - 18 files changed, 122 insertions(+), 84 deletions(-) diff --git a/firmware/controllers/actuators/fan_control.cpp b/firmware/controllers/actuators/fan_control.cpp index be85ee2942..eecfc25c46 100644 --- a/firmware/controllers/actuators/fan_control.cpp +++ b/firmware/controllers/actuators/fan_control.cpp @@ -5,16 +5,16 @@ #include "bench_test.h" bool FanController::getState(bool acActive, bool lastState) { - auto [cltValid, clt] = Sensor::get(SensorType::Clt); + auto clt = Sensor::get(SensorType::Clt); cranking = engine->rpmCalculator.isCranking(); notRunning = !engine->rpmCalculator.isRunning(); disabledWhileEngineStopped = notRunning && disableWhenStopped(); - brokenClt = !cltValid; + brokenClt = !clt; enabledForAc = enableWithAc() && acActive; - hot = clt > getFanOnTemp(); - cold = clt < getFanOffTemp(); + hot = clt.Value > getFanOnTemp(); + cold = clt.Value < getFanOffTemp(); if (cranking) { // Inhibit while cranking diff --git a/firmware/controllers/algo/advance_map.cpp b/firmware/controllers/algo/advance_map.cpp index 8bd8919f99..a8bb4922d9 100644 --- a/firmware/controllers/algo/advance_map.cpp +++ b/firmware/controllers/algo/advance_map.cpp @@ -55,10 +55,10 @@ static angle_t getRunningAdvance(int rpm, float engineLoad) { engine->module()->isIdlingOrTaper()) { float idleAdvance = interpolate2d(rpm, config->idleAdvanceBins, config->idleAdvance); - auto [valid, tps] = Sensor::get(SensorType::DriverThrottleIntent); - if (valid) { + auto tps = Sensor::get(SensorType::DriverThrottleIntent); + if (tps) { // interpolate between idle table and normal (running) table using TPS threshold - advanceAngle = interpolateClamped(0.0f, idleAdvance, engineConfiguration->idlePidDeactivationTpsThreshold, advanceAngle, tps); + advanceAngle = interpolateClamped(0.0f, idleAdvance, engineConfiguration->idlePidDeactivationTpsThreshold, advanceAngle, tps.Value); } } @@ -80,14 +80,14 @@ static angle_t getRunningAdvance(int rpm, float engineLoad) { } angle_t getAdvanceCorrections(int rpm) { - const auto [iatValid, iat] = Sensor::get(SensorType::Iat); + auto iat = Sensor::get(SensorType::Iat); - if (!iatValid) { + if (!iat) { engine->engineState.timingIatCorrection = 0; } else { engine->engineState.timingIatCorrection = interpolate3d( config->ignitionIatCorrTable, - config->ignitionIatCorrLoadBins, iat, + config->ignitionIatCorrLoadBins, iat.Value, config->ignitionIatCorrRpmBins, rpm ); } diff --git a/firmware/controllers/algo/fuel/dfco.cpp b/firmware/controllers/algo/fuel/dfco.cpp index 8b9bcbd4db..8b4183e507 100644 --- a/firmware/controllers/algo/fuel/dfco.cpp +++ b/firmware/controllers/algo/fuel/dfco.cpp @@ -5,21 +5,21 @@ bool DfcoController::getState() const { return false; } - const auto [tpsValid, tpsPos] = Sensor::get(SensorType::DriverThrottleIntent); - const auto [cltValid, clt] = Sensor::get(SensorType::Clt); - const auto [mapValid, map] = Sensor::get(SensorType::Map); + const auto tps = Sensor::get(SensorType::DriverThrottleIntent); + const auto clt = Sensor::get(SensorType::Clt); + const auto map = Sensor::get(SensorType::Map); // If some sensor is broken, inhibit DFCO - if (!tpsValid || !cltValid || !mapValid) { + if (!tps || !clt || !map) { return false; } float rpm = Sensor::getOrZero(SensorType::Rpm); float vss = Sensor::getOrZero(SensorType::VehicleSpeed); - bool mapActivate = map < engineConfiguration->coastingFuelCutMap; - bool tpsActivate = tpsPos < engineConfiguration->coastingFuelCutTps; - bool cltActivate = clt > engineConfiguration->coastingFuelCutClt; + bool mapActivate = map.Value < engineConfiguration->coastingFuelCutMap; + bool tpsActivate = tps.Value < engineConfiguration->coastingFuelCutTps; + bool cltActivate = clt.Value > engineConfiguration->coastingFuelCutClt; // True if throttle, MAP, and CLT are all acceptable for DFCO to occur bool dfcoAllowed = mapActivate && tpsActivate && cltActivate; diff --git a/firmware/controllers/algo/fuel_math.cpp b/firmware/controllers/algo/fuel_math.cpp index ddaa3687d2..7031bd1286 100644 --- a/firmware/controllers/algo/fuel_math.cpp +++ b/firmware/controllers/algo/fuel_math.cpp @@ -344,30 +344,30 @@ void initFuelMap() { * @brief Engine warm-up fuel correction. */ float getCltFuelCorrection() { - const auto [valid, clt] = Sensor::get(SensorType::Clt); + const auto clt = Sensor::get(SensorType::Clt); - if (!valid) + if (!clt) return 1; // this error should be already reported somewhere else, let's just handle it - return interpolate2d(clt, config->cltFuelCorrBins, config->cltFuelCorr); + return interpolate2d(clt.Value, config->cltFuelCorrBins, config->cltFuelCorr); } angle_t getCltTimingCorrection() { - const auto [valid, clt] = Sensor::get(SensorType::Clt); + const auto clt = Sensor::get(SensorType::Clt); - if (!valid) + if (!clt) return 0; // this error should be already reported somewhere else, let's just handle it - return interpolate2d(clt, config->cltTimingBins, config->cltTimingExtra); + return interpolate2d(clt.Value, config->cltTimingBins, config->cltTimingExtra); } float getIatFuelCorrection() { - const auto [valid, iat] = Sensor::get(SensorType::Iat); + const auto iat = Sensor::get(SensorType::Iat); - if (!valid) + if (!iat) return 1; // this error should be already reported somewhere else, let's just handle it - return interpolate2d(iat, config->iatFuelCorrBins, config->iatFuelCorr); + return interpolate2d(iat.Value, config->iatFuelCorrBins, config->iatFuelCorr); } float getBaroCorrection() { diff --git a/firmware/controllers/engine_cycle/aux_valves.cpp b/firmware/controllers/engine_cycle/aux_valves.cpp index feec6ae885..198e9c8adc 100644 --- a/firmware/controllers/engine_cycle/aux_valves.cpp +++ b/firmware/controllers/engine_cycle/aux_valves.cpp @@ -81,17 +81,17 @@ void recalculateAuxValveTiming() { return; } - auto [valid, tps] = Sensor::get(SensorType::DriverThrottleIntent); - if (!valid) { + auto tps = Sensor::get(SensorType::DriverThrottleIntent); + if (!tps) { // error should be already reported by now return; } - engine->engineState.auxValveStart = interpolate2d(tps, + engine->engineState.auxValveStart = interpolate2d(tps.Value, config->scriptCurve1Bins, config->scriptCurve1); - engine->engineState.auxValveEnd = interpolate2d(tps, + engine->engineState.auxValveEnd = interpolate2d(tps.Value, config->scriptCurve2Bins, config->scriptCurve2); diff --git a/firmware/controllers/engine_cycle/high_pressure_fuel_pump.cpp b/firmware/controllers/engine_cycle/high_pressure_fuel_pump.cpp index d8d9e0ed53..f63692a9df 100644 --- a/firmware/controllers/engine_cycle/high_pressure_fuel_pump.cpp +++ b/firmware/controllers/engine_cycle/high_pressure_fuel_pump.cpp @@ -81,11 +81,16 @@ float HpfpQuantity::calcPI(int rpm, float calc_fuel_percent) { m_pressureTarget_kPa - (engineConfiguration->hpfpTargetDecay * (FAST_CALLBACK_PERIOD_MS / 1000.)), interpolate3d(engineConfiguration->hpfpTarget, - engineConfiguration->hpfpTargetLoadBins, Sensor::get(SensorType::Map).Value, // TODO: allow other load axis, like we claim to + engineConfiguration->hpfpTargetLoadBins, Sensor::get(SensorType::Map).value_or(0), // TODO: allow other load axis, like we claim to engineConfiguration->hpfpTargetRpmBins, rpm)); + auto fuelPressure = Sensor::get(SensorType::FuelPressureHigh); + if (!fuelPressure) { + return 0; + } + float pressureError_kPa = - m_pressureTarget_kPa - Sensor::get(SensorType::FuelPressureHigh).Value; + m_pressureTarget_kPa - fuelPressure.Value; float p_control_percent = pressureError_kPa * engineConfiguration->hpfpPidP; float i_factor_divisor = diff --git a/firmware/controllers/sensors/converters/flex_sensor.h b/firmware/controllers/sensors/converters/flex_sensor.h index a23655fd24..7842906a3e 100644 --- a/firmware/controllers/sensors/converters/flex_sensor.h +++ b/firmware/controllers/sensors/converters/flex_sensor.h @@ -12,13 +12,17 @@ public: SensorResult convert(float frequency) const override { // Sensor should only report 50-150hz, significantly outside that range indicates a problem // it changes to 200hz+ to indicate methanol "contamination" - if (frequency > 45 && frequency < 155) { - float flexPct = clampF(0, frequency - 50, 100); - - return m_filter.filter(flexPct); - } else { - return unexpected; + if (frequency < 45) { + return UnexpectedCode::Low; } + + if (frequency > 155) { + return UnexpectedCode::High; + } + + float flexPct = clampF(0, frequency - 50, 100); + + return m_filter.filter(flexPct); } private: diff --git a/firmware/controllers/sensors/converters/linear_func.cpp b/firmware/controllers/sensors/converters/linear_func.cpp index afff57f5a0..5e42d11ad7 100644 --- a/firmware/controllers/sensors/converters/linear_func.cpp +++ b/firmware/controllers/sensors/converters/linear_func.cpp @@ -16,11 +16,15 @@ void LinearFunc::configure(float in1, float out1, float in2, float out2, float m SensorResult LinearFunc::convert(float inputValue) const { float result = m_a * inputValue + m_b; - // Bounds check - bool isValid = result <= m_maxOutput && result >= m_minOutput; + // Bounds checks + // Flipped error codes in case of m_a < 0 so that they indicate whether the input + // voltage is high/low, instead of the output high/low + if (result > m_maxOutput) { + return m_a > 0 ? UnexpectedCode::High : UnexpectedCode::Low; + } - if (!isValid) { - return unexpected; + if (result < m_minOutput) { + return m_a > 0 ? UnexpectedCode::Low : UnexpectedCode::High; } return result; diff --git a/firmware/controllers/sensors/converters/resistance_func.cpp b/firmware/controllers/sensors/converters/resistance_func.cpp index 8a26921a20..e7d2e16e2b 100644 --- a/firmware/controllers/sensors/converters/resistance_func.cpp +++ b/firmware/controllers/sensors/converters/resistance_func.cpp @@ -12,12 +12,12 @@ void ResistanceFunc::configure(float supplyVoltage, float pullupResistor) { SensorResult ResistanceFunc::convert(float raw) const { // If the voltage is very low, the sensor is a dead short. if (raw < 0.05f) { - return unexpected; + return UnexpectedCode::Low; } // If the voltage is very high (98% VCC), the sensor is open circuit. if (raw > (m_supplyVoltage * 0.98f)) { - return unexpected; + return UnexpectedCode::High; } // Voltage is in a sensible range - convert diff --git a/firmware/controllers/sensors/converters/thermistor_func.cpp b/firmware/controllers/sensors/converters/thermistor_func.cpp index d62f99321a..558b9d6bd5 100644 --- a/firmware/controllers/sensors/converters/thermistor_func.cpp +++ b/firmware/controllers/sensors/converters/thermistor_func.cpp @@ -12,7 +12,7 @@ SensorResult ThermistorFunc::convert(float ohms) const { // This resistance should have already been validated - only // thing we can check is that it's non-negative if (ohms <= 0) { - return unexpected; + return UnexpectedCode::Low; } float lnR = logf(ohms); @@ -26,9 +26,14 @@ SensorResult ThermistorFunc::convert(float ohms) const { float celsius = convertKelvinToCelcius(kelvin); // bounds check result - please don't try to run rusEfi when colder than -50C - // high end limit is required as this could be an oil temp sensor - if (celsius < -50 || celsius > 250) { - return unexpected; + // high end limit is required as this could be an oil temp sensor on an + // air cooled engine + if (celsius < -50) { + return UnexpectedCode::Low; + } + + if (celsius > 250) { + return UnexpectedCode::High; } return celsius; diff --git a/firmware/controllers/sensors/functional_sensor.cpp b/firmware/controllers/sensors/functional_sensor.cpp index 3215e294f9..01338a9d51 100644 --- a/firmware/controllers/sensors/functional_sensor.cpp +++ b/firmware/controllers/sensors/functional_sensor.cpp @@ -7,7 +7,7 @@ void FunctionalSensor::postRawValue(float inputValue, efitick_t timestamp) { // If no function is set, this sensor isn't valid. if (!m_function) { - invalidate(); + invalidate(UnexpectedCode::Configuration); return; } @@ -21,6 +21,6 @@ void FunctionalSensor::postRawValue(float inputValue, efitick_t timestamp) { if (r.Valid) { setValidValue(r.Value, timestamp); } else { - invalidate(); + invalidate(r.Code); } } diff --git a/firmware/controllers/sensors/redundant_ford_tps.cpp b/firmware/controllers/sensors/redundant_ford_tps.cpp index 9861753c55..f357735825 100644 --- a/firmware/controllers/sensors/redundant_ford_tps.cpp +++ b/firmware/controllers/sensors/redundant_ford_tps.cpp @@ -23,7 +23,7 @@ SensorResult RedundantFordTps::get() const { // If either result is invalid, return invalid. if (!tps1 || !tps2) { - return unexpected; + return UnexpectedCode::Inconsistent; } // The "actual" position resolved by the second throttle - this tops out at m_secondaryMaximum instead of 100% @@ -37,7 +37,7 @@ SensorResult RedundantFordTps::get() const { // Check that the resolved positions are close float delta = absF(tps1.Value - tps2Actual); if (delta > m_maxDifference) { - return unexpected; + return UnexpectedCode::Inconsistent; } return (tps1.Value + tps2Actual) / 2; @@ -49,5 +49,5 @@ SensorResult RedundantFordTps::get() const { } // Any other condition indicates an mismatch, and therefore an error - return unexpected; + return UnexpectedCode::Inconsistent; } diff --git a/firmware/controllers/sensors/redundant_sensor.cpp b/firmware/controllers/sensors/redundant_sensor.cpp index 86198e30f1..44dabb6fa9 100644 --- a/firmware/controllers/sensors/redundant_sensor.cpp +++ b/firmware/controllers/sensors/redundant_sensor.cpp @@ -26,13 +26,13 @@ SensorResult RedundantSensor::get() const { // If either result is invalid, return invalid. if (!result1.Valid || !result2.Valid) { - return unexpected; + return UnexpectedCode::Inconsistent; } // If both are valid, check that they're near one another float delta = absF(result1.Value - result2.Value); if (delta > m_maxDifference) { - return unexpected; + return UnexpectedCode::Inconsistent; } // Both sensors are valid, and their readings are close. All is well. diff --git a/firmware/controllers/sensors/sensor.cpp b/firmware/controllers/sensors/sensor.cpp index 305e0eba3b..6e0dd72ccb 100644 --- a/firmware/controllers/sensors/sensor.cpp +++ b/firmware/controllers/sensors/sensor.cpp @@ -55,7 +55,7 @@ public: if (s) { // If this sensor says it doesn't exist, return unexpected if (!s->hasSensor()) { - return unexpected; + return UnexpectedCode::Configuration; } // If we found the sensor, ask it for a result. @@ -63,7 +63,7 @@ public: } // We've exhausted all valid ways to return something - sensor not found. - return unexpected; + return UnexpectedCode::Configuration; } void showInfo(const char* sensorName) const { @@ -155,7 +155,7 @@ void Sensor::unregister() { // Check if this is a valid sensor entry if (!entry) { - return unexpected; + return UnexpectedCode::Configuration; } return entry->get(); diff --git a/firmware/controllers/sensors/sensor_info_printing.cpp b/firmware/controllers/sensors/sensor_info_printing.cpp index dae93dc40c..d6488e8f4b 100644 --- a/firmware/controllers/sensors/sensor_info_printing.cpp +++ b/firmware/controllers/sensors/sensor_info_printing.cpp @@ -15,8 +15,8 @@ void ProxySensor::showInfo(const char* sensorName) const { } void FunctionalSensor::showInfo(const char* sensorName) const { - const auto [valid, value] = get(); - efiPrintf("Sensor \"%s\": Raw value: %.2f Valid: %s Converted value %.2f", sensorName, m_rawValue, boolToString(valid), value); + const auto value = get(); + efiPrintf("Sensor \"%s\": Raw value: %.2f Valid: %s Converted value %.2f", sensorName, m_rawValue, boolToString(value.Valid), value.Value); // now print out the underlying function's info if (auto func = m_function) { @@ -28,8 +28,8 @@ void FunctionalSensor::showInfo(const char* sensorName) const { #include "can_sensor.h" void CanSensorBase::showInfo(const char* sensorName) const { - const auto [valid, value] = get(); - efiPrintf("CAN Sensor \"%s\": valid: %s value: %.2f", sensorName, boolToString(valid), value); + const auto value = get(); + efiPrintf("CAN Sensor \"%s\": valid: %s value: %.2f", sensorName, boolToString(value.Valid), value.Value); } #endif // EFI_CAN_SUPPORT @@ -63,8 +63,8 @@ void Lps25Sensor::showInfo(const char* sensorName) const { void LinearFunc::showInfo(float testRawValue) const { efiPrintf(" Linear function slope: %.2f offset: %.2f min: %.1f max: %.1f", m_a, m_b, m_minOutput, m_maxOutput); - const auto [valid, value] = convert(testRawValue); - efiPrintf(" raw value %.2f converts to %.2f valid: %s", testRawValue, value, boolToString(valid)); + const auto value = convert(testRawValue); + efiPrintf(" raw value %.2f converts to %.2f valid: %s", testRawValue, value.Value, boolToString(value.Valid)); } void ResistanceFunc::showInfo(float testInputValue) const { @@ -73,8 +73,8 @@ void ResistanceFunc::showInfo(float testInputValue) const { } void ThermistorFunc::showInfo(float testInputValue) const { - const auto [valid, value] = convert(testInputValue); - efiPrintf(" %.1f ohms -> valid: %s. %.1f deg C", testInputValue, boolToString(valid), value); + const auto value = convert(testInputValue); + efiPrintf(" %.1f ohms -> valid: %s. %.1f deg C", testInputValue, boolToString(value.Valid), value.Value); } void IdentityFunction::showInfo(float /*testInputValue*/) const { diff --git a/firmware/controllers/sensors/stored_value_sensor.h b/firmware/controllers/sensors/stored_value_sensor.h index 868e52c52c..77648dfe82 100644 --- a/firmware/controllers/sensors/stored_value_sensor.h +++ b/firmware/controllers/sensors/stored_value_sensor.h @@ -30,51 +30,50 @@ class StoredValueSensor : public Sensor { public: SensorResult get() const final override { - bool valid = m_isValid; - float value = m_value; - - if (!valid) { - return unexpected; - } + auto result = m_result; // Timeouts are disabled, return last value if (Sensor::s_inhibitSensorTimeouts) { - return value; + return result; } if (m_timeoutPeriod != 0) { // zero m_timeoutPeriod means value lasts forever if (getTimeNowNt() - m_timeoutPeriod > m_lastUpdate) { - return unexpected; + return UnexpectedCode::Timeout; } } - return value; + return result; } StoredValueSensor(SensorType type, efitick_t timeoutNt) : Sensor(type) + , m_result(unexpected) , m_timeoutPeriod(timeoutNt) { } // Invalidate the stored value. void invalidate() { - m_isValid = false; + m_result = unexpected; + } + + // Invalidate the stored value with an error code + void invalidate(UnexpectedCode why) { + m_result = why; } // A new reading is available: set and validate a new value for the sensor. void setValidValue(float value, efitick_t timestamp) { // Set value before valid - so we don't briefly have the valid bit set on an invalid value - m_value = value; - m_isValid = true; + m_result = value; m_lastUpdate = timestamp; } void showInfo(const char*) const override { } private: - bool m_isValid = false; - float m_value = 0.0f; + SensorResult m_result; const efitick_t m_timeoutPeriod; efitick_t m_lastUpdate = 0; diff --git a/firmware/util/expected.h b/firmware/util/expected.h index b7ca0c940c..ac05d2fd9d 100644 --- a/firmware/util/expected.h +++ b/firmware/util/expected.h @@ -17,13 +17,35 @@ struct unexpected_t {}; +enum class UnexpectedCode : char { + Unknown = 0, + + // Too much time has passed + Timeout, + + // The decoded value was impossibly high/low + High, + Low, + + // An inconsistency was detected using multiple sources of information + Inconsistent, + + // A value is unavailable due to configuration + Configuration, +}; template struct expected { - const bool Valid; - const TValue Value; + bool Valid; + + union { + TValue Value; + UnexpectedCode Code; + }; // Implicit constructor to construct in the invalid state - constexpr expected(const unexpected_t&) : Valid(false), Value{} {} + constexpr expected(const unexpected_t&) : Valid(false), Code{UnexpectedCode::Unknown} {} + + constexpr expected(UnexpectedCode code) : Valid(false), Code{code} {} // Implicit constructor to convert from TValue (for valid values, so an expected behaves like a T) constexpr expected(TValue validValue) diff --git a/unit_tests/tests/sensor/func_sensor.cpp b/unit_tests/tests/sensor/func_sensor.cpp index 973005e960..67808330f6 100644 --- a/unit_tests/tests/sensor/func_sensor.cpp +++ b/unit_tests/tests/sensor/func_sensor.cpp @@ -64,6 +64,5 @@ TEST_F(SensorConverted, TestInvalid) { { auto s = Sensor::get(SensorType::Clt); EXPECT_FALSE(s.Valid); - EXPECT_FLOAT_EQ(s.Value, 0); } }