From 3bd1ddcc1f8ce6d4a12eb7dc2049df9d4a32ac7e Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Tue, 24 Sep 2019 18:11:41 -0700 Subject: [PATCH] Sensors: composition >> inheritance (#953) * functional sensors * format * tests * sim makefile * eol * format tests --- firmware/Makefile | 1 + .../controllers/sensors/converter_sensor.h | 60 --------------- .../linear_func.cpp} | 6 +- .../linear_func.h} | 10 +-- .../converters/sensor_converter_func.h | 7 ++ .../controllers/sensors/functional_sensor.h | 75 +++++++++++++++++++ firmware/controllers/sensors/sensors.mk | 2 +- firmware/hw_layer/adc_subscription.cpp | 15 ++-- firmware/hw_layer/adc_subscription.h | 6 +- firmware/init/sensor/init_oil_pressure.cpp | 7 +- simulator/Makefile | 1 + unit_tests/Makefile | 1 + ...erter_sensor.cpp => functional_sensor.cpp} | 45 +++++------ .../sensor/{lin_sensor.cpp => lin_func.cpp} | 22 ++---- unit_tests/tests/tests.mk | 4 +- 15 files changed, 143 insertions(+), 119 deletions(-) delete mode 100644 firmware/controllers/sensors/converter_sensor.h rename firmware/controllers/sensors/{linear_sensor.cpp => converters/linear_func.cpp} (59%) rename firmware/controllers/sensors/{linear_sensor.h => converters/linear_func.h} (55%) create mode 100644 firmware/controllers/sensors/converters/sensor_converter_func.h create mode 100644 firmware/controllers/sensors/functional_sensor.h rename unit_tests/tests/sensor/{converter_sensor.cpp => functional_sensor.cpp} (75%) rename unit_tests/tests/sensor/{lin_sensor.cpp => lin_func.cpp} (59%) diff --git a/firmware/Makefile b/firmware/Makefile index 843e684869..59d6aad1c2 100644 --- a/firmware/Makefile +++ b/firmware/Makefile @@ -306,6 +306,7 @@ INCDIR = $(CHIBIOS)/os/license \ development/test \ $(CONTROLLERS_INC) \ controllers/sensors \ + controllers/sensors/converters \ controllers/system \ controllers/algo \ controllers/core \ diff --git a/firmware/controllers/sensors/converter_sensor.h b/firmware/controllers/sensors/converter_sensor.h deleted file mode 100644 index f3e86b5644..0000000000 --- a/firmware/controllers/sensors/converter_sensor.h +++ /dev/null @@ -1,60 +0,0 @@ -/** - * @file converter_sensor.h - * - * @date September 12, 2019 - * @author Matthew Kennedy, (c) 2019 - */ - -#pragma once - -#include "stored_value_sensor.h" - -/** - * @brief Base class for sensors that convert from some raw floating point - * value (ex: voltage, frequency, pulse width) to a sensor reading. - * - * To use this base class, inherit it and implement ConvertFromInputValue(float input). - * Perform any conversion work necessary to convert from the raw value to a sensor - * reading, and return it. Register an instance of the new class with an interface - * that provides and posts raw values so the sensor can update. - */ -class ConvertedSensor : public StoredValueSensor { -public: - void postRawValue(float inputValue) { - // Report the raw value - float *rawReportLocation = m_rawReportingLocation; - if (rawReportLocation) { - *rawReportLocation = inputValue; - } - - auto r = convertFromInputValue(inputValue); - - // This has to happen so that we set the valid bit after - // the value is stored, to prevent the data race of reading - // an old invalid value - if (r.Valid) { - setValidValue(r.Value); - } else { - invalidate(); - } - } - - void setRawReportingLocation(float *rawReportingLocation) { - m_rawReportingLocation = rawReportingLocation; - } - -protected: - explicit ConvertedSensor(SensorType type) - : StoredValueSensor(type) {} - - /** - * @brief Convert from the "raw" input value to a sensor reading (or invalid). - * - * For example, this function might convert from a voltage to the pressure - * represented by that voltage. - */ - virtual SensorResult convertFromInputValue(float inputValue) = 0; - -private: - float *m_rawReportingLocation = nullptr; -}; diff --git a/firmware/controllers/sensors/linear_sensor.cpp b/firmware/controllers/sensors/converters/linear_func.cpp similarity index 59% rename from firmware/controllers/sensors/linear_sensor.cpp rename to firmware/controllers/sensors/converters/linear_func.cpp index d219eb38d3..8664805642 100644 --- a/firmware/controllers/sensors/linear_sensor.cpp +++ b/firmware/controllers/sensors/converters/linear_func.cpp @@ -1,8 +1,8 @@ -#include "linear_sensor.h" +#include "linear_func.h" #include "interpolation.h" -void LinearSensor::configure(float in1, float out1, float in2, float out2, float minOutput, float maxOutput) { +void LinearFunc::configure(float in1, float out1, float in2, float out2, float minOutput, float maxOutput) { m_minOutput = minOutput; m_maxOutput = maxOutput; @@ -10,7 +10,7 @@ void LinearSensor::configure(float in1, float out1, float in2, float out2, float m_b = out1 - m_a * in1; } -SensorResult LinearSensor::convertFromInputValue(float inputValue) { +SensorResult LinearFunc::convert(float inputValue) const { float result = m_a * inputValue + m_b; // Bounds check diff --git a/firmware/controllers/sensors/linear_sensor.h b/firmware/controllers/sensors/converters/linear_func.h similarity index 55% rename from firmware/controllers/sensors/linear_sensor.h rename to firmware/controllers/sensors/converters/linear_func.h index 373c746df2..3cb3a44161 100644 --- a/firmware/controllers/sensors/linear_sensor.h +++ b/firmware/controllers/sensors/converters/linear_func.h @@ -1,16 +1,14 @@ #pragma once -#include "converter_sensor.h" +#include "sensor_converter_func.h" -class LinearSensor final : public ConvertedSensor { +class LinearFunc final : public SensorConverter { public: - explicit LinearSensor(SensorType type) - : ConvertedSensor(type) {} + LinearFunc() = default; void configure(float in1, float out1, float in2, float out2, float minOutput, float maxOutput); -protected: - SensorResult convertFromInputValue(float inputValue) override; + SensorResult convert(float inputValue) const override; private: // Linear equation parameters for equation of form diff --git a/firmware/controllers/sensors/converters/sensor_converter_func.h b/firmware/controllers/sensors/converters/sensor_converter_func.h new file mode 100644 index 0000000000..ee13adb9aa --- /dev/null +++ b/firmware/controllers/sensors/converters/sensor_converter_func.h @@ -0,0 +1,7 @@ +#pragma once + +#include "sensor.h" + +struct SensorConverter { + virtual SensorResult convert(float raw) const = 0; +}; diff --git a/firmware/controllers/sensors/functional_sensor.h b/firmware/controllers/sensors/functional_sensor.h new file mode 100644 index 0000000000..0a292f8860 --- /dev/null +++ b/firmware/controllers/sensors/functional_sensor.h @@ -0,0 +1,75 @@ +/** + * @file converter_sensor.h + * + * @date September 12, 2019 + * @author Matthew Kennedy, (c) 2019 + */ + +#pragma once + +#include "converters/sensor_converter_func.h" +#include "stored_value_sensor.h" + +#include + +struct FunctionalSensorBase : public StoredValueSensor { + FunctionalSensorBase(SensorType type) + : StoredValueSensor(type) {} + + virtual void postRawValue(float value) = 0; +}; + +/** + * @brief Class for sensors that convert from some raw floating point + * value (ex: voltage, frequency, pulse width) to a sensor reading. + * + * To use this class, implement the conversion operation for your sensor + * as a class that inherits from SensorConverter, and implement convert + * to convert a raw reading from the sensor to a usable value (and valid bit). + * + * Register an instance of the new class with an interface + * that provides and posts raw values so the sensor can update. + */ +template +class FunctionalSensor final : public FunctionalSensorBase { + static_assert(std::is_base_of_v, "TFunc must inherit from SensorConverter"); + static_assert(std::is_default_constructible_v, "TFunc must be default constructible"); + +public: + explicit FunctionalSensor(SensorType type) + : FunctionalSensorBase(type) {} + + void postRawValue(float inputValue) { + // Report the raw value + float *rawReportLocation = m_rawReportingLocation; + if (rawReportLocation) { + *rawReportLocation = inputValue; + } + + auto r = m_function.convert(inputValue); + + // This has to happen so that we set the valid bit after + // the value is stored, to prevent the data race of reading + // an old invalid value + if (r.Valid) { + setValidValue(r.Value); + } else { + invalidate(); + } + } + + void setRawReportingLocation(float *rawReportingLocation) { + m_rawReportingLocation = rawReportingLocation; + } + + // Allow access to the underlying function + TFunc &f() { + return m_function; + } + +private: + float *m_rawReportingLocation = nullptr; + + // Conversion function for this sensor + TFunc m_function; +}; diff --git a/firmware/controllers/sensors/sensors.mk b/firmware/controllers/sensors/sensors.mk index b2ae71cdba..926c95e094 100644 --- a/firmware/controllers/sensors/sensors.mk +++ b/firmware/controllers/sensors/sensors.mk @@ -11,4 +11,4 @@ CONTROLLERS_SENSORS_SRC_CPP = $(PROJECT_DIR)/controllers/sensors/thermistors.cp $(PROJECT_DIR)/controllers/sensors/maf2map.cpp \ $(PROJECT_DIR)/controllers/sensors/hip9011_lookup.cpp \ $(PROJECT_DIR)/controllers/sensors/sensor.cpp \ - $(PROJECT_DIR)/controllers/sensors/linear_sensor.cpp + $(PROJECT_DIR)/controllers/sensors/converters/linear_func.cpp diff --git a/firmware/hw_layer/adc_subscription.cpp b/firmware/hw_layer/adc_subscription.cpp index f156003ce1..076490d79b 100644 --- a/firmware/hw_layer/adc_subscription.cpp +++ b/firmware/hw_layer/adc_subscription.cpp @@ -1,4 +1,5 @@ #include "adc_subscription.h" + #include "adc_inputs.h" #include "engine.h" @@ -9,7 +10,7 @@ EXTERN_ENGINE; #if !EFI_UNIT_TEST struct AdcSubscriptionEntry { - ConvertedSensor* Sensor; + FunctionalSensorBase *Sensor; float VoltsPerAdcVolt; adc_channel_e Channel; }; @@ -17,12 +18,14 @@ struct AdcSubscriptionEntry { static size_t s_nextEntry = 0; static AdcSubscriptionEntry s_entries[8]; -void AdcSubscription::SubscribeSensor(ConvertedSensor& sensor, adc_channel_e channel, float voltsPerAdcVolt /*= 0.0f*/) { +void AdcSubscription::SubscribeSensor(FunctionalSensorBase &sensor, + adc_channel_e channel, + float voltsPerAdcVolt /*= 0.0f*/) { // Don't subscribe null channels if (channel == EFI_ADC_NONE) { return; } - + // bounds check if (s_nextEntry >= std::size(s_entries)) { return; @@ -34,7 +37,7 @@ void AdcSubscription::SubscribeSensor(ConvertedSensor& sensor, adc_channel_e cha } // Populate the entry - auto& entry = s_entries[s_nextEntry]; + auto &entry = s_entries[s_nextEntry]; entry.Sensor = &sensor; entry.VoltsPerAdcVolt = voltsPerAdcVolt; entry.Channel = channel; @@ -44,7 +47,7 @@ void AdcSubscription::SubscribeSensor(ConvertedSensor& sensor, adc_channel_e cha void AdcSubscription::UpdateSubscribers() { for (size_t i = 0; i < s_nextEntry; i++) { - auto& entry = s_entries[i]; + auto &entry = s_entries[i]; float mcuVolts = getVoltage("sensor", entry.Channel); float sensorVolts = mcuVolts * entry.VoltsPerAdcVolt; @@ -53,4 +56,4 @@ void AdcSubscription::UpdateSubscribers() { } } -#endif// !EFI_UNIT_TEST +#endif // !EFI_UNIT_TEST diff --git a/firmware/hw_layer/adc_subscription.h b/firmware/hw_layer/adc_subscription.h index 1da1f45be8..1cb41d3258 100644 --- a/firmware/hw_layer/adc_subscription.h +++ b/firmware/hw_layer/adc_subscription.h @@ -2,11 +2,13 @@ * @file adc_subscription.h */ +#pragma once + +#include "functional_sensor.h" #include "global.h" -#include "converter_sensor.h" class AdcSubscription { public: - static void SubscribeSensor(ConvertedSensor& sensor, adc_channel_e channel, float voltsPerAdcVolt = 0.0f); + static void SubscribeSensor(FunctionalSensorBase &sensor, adc_channel_e channel, float voltsPerAdcVolt = 0.0f); static void UpdateSubscribers(); }; diff --git a/firmware/init/sensor/init_oil_pressure.cpp b/firmware/init/sensor/init_oil_pressure.cpp index 7d15aeafea..8e06614f19 100644 --- a/firmware/init/sensor/init_oil_pressure.cpp +++ b/firmware/init/sensor/init_oil_pressure.cpp @@ -2,14 +2,15 @@ #include "engine.h" #include "error_handling.h" #include "global.h" -#include "linear_sensor.h" +#include "functional_sensor.h" +#include "linear_func.h" #include "tunerstudio_configuration.h" EXTERN_ENGINE; extern TunerStudioOutputChannels tsOutputChannels; -LinearSensor oilpSensor(SensorType::OilPressure); +FunctionalSensor oilpSensor(SensorType::OilPressure); void initOilPressure() { // Only register if we have a sensor @@ -28,7 +29,7 @@ void initOilPressure() { float greaterOutput = val1 > val2 ? val1 : val2; // Allow slightly negative output (-5kpa) so as to not fail the sensor when engine is off - oilpSensor.configure(sensorCfg->v1, val1, sensorCfg->v2, val2, /*minOutput*/ -5, greaterOutput); + oilpSensor.f().configure(sensorCfg->v1, val1, sensorCfg->v2, val2, /*minOutput*/ -5, greaterOutput); // Tell it to report to its output channel oilpSensor.setReportingLocation(&tsOutputChannels.oilPressure); diff --git a/simulator/Makefile b/simulator/Makefile index f076be2257..52c0ab88d3 100644 --- a/simulator/Makefile +++ b/simulator/Makefile @@ -208,6 +208,7 @@ INCDIR = . \ $(PROJECT_DIR)/controllers/core \ $(PROJECT_DIR)/controllers/math \ $(PROJECT_DIR)/controllers/sensors \ + $(PROJECT_DIR)/controllers/sensors/converters \ $(PROJECT_DIR)/controllers/system \ $(PROJECT_DIR)/controllers/trigger \ $(PROJECT_DIR)/controllers/trigger/decoders \ diff --git a/unit_tests/Makefile b/unit_tests/Makefile index d163991924..d21db4246d 100644 --- a/unit_tests/Makefile +++ b/unit_tests/Makefile @@ -170,6 +170,7 @@ INCDIR = . \ $(CONTROLLERS_INC) \ $(PROJECT_DIR)/console \ $(PROJECT_DIR)/controllers/sensors \ + $(PROJECT_DIR)/controllers/sensors/converters \ $(PROJECT_DIR)/controllers/algo \ $(PROJECT_DIR)/controllers/core \ $(PROJECT_DIR)/controllers/math \ diff --git a/unit_tests/tests/sensor/converter_sensor.cpp b/unit_tests/tests/sensor/functional_sensor.cpp similarity index 75% rename from unit_tests/tests/sensor/converter_sensor.cpp rename to unit_tests/tests/sensor/functional_sensor.cpp index 0434ae8331..e373ca949b 100644 --- a/unit_tests/tests/sensor/converter_sensor.cpp +++ b/unit_tests/tests/sensor/functional_sensor.cpp @@ -1,25 +1,9 @@ -#include "converter_sensor.h" +#include "functional_sensor.h" #include -class SensorConverted : public ::testing::Test { -protected: - void SetUp() override { - Sensor::resetRegistry(); - } - - void TearDown() override { - Sensor::resetRegistry(); - } -}; - -class DoublerConverterSensor final : public ConvertedSensor { -public: - DoublerConverterSensor() - : ConvertedSensor(SensorType::Clt) {} - -protected: - SensorResult convertFromInputValue(float input) { +struct DoublerFunc final : public SensorConverter { + SensorResult convert(float input) const { bool valid = input > 0; float value = input * 2; @@ -27,8 +11,23 @@ protected: } }; +class SensorConverted : public ::testing::Test { +protected: + SensorConverted() + : dut(SensorType::Clt) {} + + void SetUp() override { + Sensor::resetRegistry(); + } + + void TearDown() override { + Sensor::resetRegistry(); + } + + FunctionalSensor dut; +}; + TEST_F(SensorConverted, TestValid) { - DoublerConverterSensor dut; ASSERT_TRUE(dut.Register()); // Should be invalid - not set yet @@ -48,7 +47,6 @@ TEST_F(SensorConverted, TestValid) { } TEST_F(SensorConverted, TestInvalid) { - DoublerConverterSensor dut; ASSERT_TRUE(dut.Register()); // Should be invalid - not set yet @@ -66,3 +64,8 @@ TEST_F(SensorConverted, TestInvalid) { EXPECT_FLOAT_EQ(s.Value, 0); } } + +TEST_F(SensorConverted, TestGet) { + // we're only checking that this compiles + DoublerFunc &f = dut.f(); +} diff --git a/unit_tests/tests/sensor/lin_sensor.cpp b/unit_tests/tests/sensor/lin_func.cpp similarity index 59% rename from unit_tests/tests/sensor/lin_sensor.cpp rename to unit_tests/tests/sensor/lin_func.cpp index 10eb38d2eb..4dff471553 100644 --- a/unit_tests/tests/sensor/lin_sensor.cpp +++ b/unit_tests/tests/sensor/lin_func.cpp @@ -1,14 +1,10 @@ -#include "linear_sensor.h" - +#include "linear_func.h" #include "unit_test_framework.h" -class SensorLinear : public ::testing::Test { +class LinearFuncTest : public ::testing::Test { protected: // Maps (1, 4) -> (100, -100) - LinearSensor dut; - - SensorLinear() - : dut(SensorType::Clt) {} + LinearFunc dut; void SetUp() override { dut.configure(1, 100, 4, -100, -110, 110); @@ -17,26 +13,22 @@ protected: #define test_point(in, out) \ { \ - dut.postRawValue(in); \ - auto result = dut.get(); \ + auto result = dut.convert(in); \ \ EXPECT_TRUE(result.Valid); \ ASSERT_NEAR(result.Value, (out), EPS4D) << "Not " << out << " for " << in; \ } #define test_point_invalid(in) \ - { \ - dut.postRawValue(in); \ - EXPECT_FALSE(dut.get().Valid); \ - } + { EXPECT_FALSE(dut.convert(in).Valid); } -TEST_F(SensorLinear, TestInRange) { +TEST_F(LinearFuncTest, TestInRange) { test_point(2.5, 0); test_point(1, 100); test_point(4, -100); } -TEST_F(SensorLinear, TestOutOfRange) { +TEST_F(LinearFuncTest, TestOutOfRange) { test_point(1, 100); test_point_invalid(0.5); diff --git a/unit_tests/tests/tests.mk b/unit_tests/tests/tests.mk index b911bd1f66..141ba3e198 100644 --- a/unit_tests/tests/tests.mk +++ b/unit_tests/tests/tests.mk @@ -29,8 +29,8 @@ TESTS_SRC_CPP = \ tests/test_gpiochip.cpp \ \ tests/sensor/basic_sensor.cpp \ - tests/sensor/converter_sensor.cpp \ + tests/sensor/functional_sensor.cpp \ tests/sensor/function_pointer_sensor.cpp \ tests/sensor/mock_sensor.cpp \ tests/sensor/sensor_reader.cpp \ - tests/sensor/lin_sensor.cpp + tests/sensor/lin_func.cpp