Debuggability in the new sensor world (#1238)

* rename to avoid conflict

* fix efilib

* add sensor printing

* makefile

* that check was already there

* const

* const

* fix tests

* formatting

Co-authored-by: Matthew Kennedy <makenne@microsoft.com>

firmware/controllers/engine_controller.cpp

@@ -123,7 +123,7 @@ void initDataStructures(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
 
 static void mostCommonInitEngineController(Logging *sharedLogger DECLARE_ENGINE_PARAMETER_SUFFIX) {
 #if !EFI_UNIT_TEST
-	initSensors();
+	initNewSensors(sharedLogger);
 #endif /* EFI_UNIT_TEST */
 
 	initSensors(sharedLogger PASS_ENGINE_PARAMETER_SUFFIX);
@@ -368,11 +368,9 @@ static void printAnalogChannelInfoExt(const char *name, adc_channel_e hwChannel,
 }
 
 static void printAnalogChannelInfo(const char *name, adc_channel_e hwChannel) {
-	if (hwChannel != EFI_ADC_NONE) {
 #if HAL_USE_ADC
-		printAnalogChannelInfoExt(name, hwChannel, getVoltage("print", hwChannel PASS_ENGINE_PARAMETER_SUFFIX), engineConfiguration->analogInputDividerCoefficient);
+	printAnalogChannelInfoExt(name, hwChannel, getVoltage("print", hwChannel PASS_ENGINE_PARAMETER_SUFFIX), engineConfiguration->analogInputDividerCoefficient);
 #endif /* HAL_USE_ADC */
-	}
 }
 
 static void printAnalogInfo(void) {

firmware/controllers/sensors/function_pointer_sensor.h

@@ -29,6 +29,8 @@ public:
 		return {valid, result};
 	}
 
+	void showInfo(Logging* logger, const char* sensorName) const override {}
+
 private:
 	float (*m_func)();
 };

firmware/controllers/sensors/functional_sensor.h

@@ -38,6 +38,8 @@ public:
 		return m_rawValue;
 	}
 
+	void showInfo(Logging* logger, const char* sensorName) const override;
+
 private:
 	// Conversion function for this sensor
 	SensorConverter* m_function = nullptr;

firmware/controllers/sensors/proxy_sensor.h

@@ -21,6 +21,8 @@ public:
 		m_proxiedSensor = proxiedSensor;
 	}
 
+	void showInfo(Logging* logger, const char* sensorName) const override;
+
 private:
 	SensorResult get() const {
 		return Sensor::get(m_proxiedSensor);

firmware/controllers/sensors/sensor.cpp

@@ -1,4 +1,6 @@
 #include "sensor.h"
+#include "efilib.h"
+#include "loggingcentral.h"
 
 // This struct represents one sensor in the registry.
 // It stores whether the sensor should use a mock value,
@@ -12,6 +14,30 @@ struct SensorRegistryEntry {
 
 static SensorRegistryEntry s_sensorRegistry[static_cast<size_t>(SensorType::PlaceholderLast)] = {};
 
+static const char* s_sensorNames[] = {
+	"Invalid",
+	"CLT",
+	"IAT",
+
+	"Oil Pressure",
+
+	"TPS 1",
+	"TPS 1 Primary",
+	"TPS 1 Secondary",
+
+	"TPS 2",
+	"TPS 2 Primary",
+	"TPS 2 Secondary",
+
+	"Acc Pedal",
+	"Acc Pedal Primary",
+	"Acc Pedal Secondary",
+
+	"Driver Acc Intent"
+};
+
+static_assert(efi::size(s_sensorNames) == efi::size(s_sensorNames));
+
 bool Sensor::Register() {
 	// Get a ref to where we should be
 	auto &entry = s_sensorRegistry[getIndex()];
@@ -28,10 +54,8 @@ bool Sensor::Register() {
 }
 
 /*static*/ void Sensor::resetRegistry() {
-	constexpr size_t len = sizeof(s_sensorRegistry) / sizeof(s_sensorRegistry[0]);
-
 	// Clear all entries
-	for (size_t i = 0; i < len; i++) {
+	for (size_t i = 0; i < efi::size(s_sensorRegistry); i++) {
 		auto &entry = s_sensorRegistry[i];
 
 		entry.sensor = nullptr;
@@ -123,12 +147,34 @@ bool Sensor::Register() {
 }
 
 /*static*/ void Sensor::resetAllMocks() {
-	constexpr size_t len = sizeof(s_sensorRegistry) / sizeof(s_sensorRegistry[0]);
-
 	// Reset all mocks
-	for (size_t i = 0; i < len; i++) {
+	for (size_t i = 0; i < efi::size(s_sensorRegistry); i++) {
 		auto &entry = s_sensorRegistry[i];
 
 		entry.useMock = false;
 	}
 }
+
+/*static*/ const char* Sensor::getSensorName(SensorType type) {
+	return s_sensorNames[static_cast<size_t>(type)];
+}
+
+// Print information about all sensors
+/*static*/ void Sensor::showAllSensorInfo(Logging* logger) {
+	for (size_t i = 1; i < efi::size(s_sensorRegistry); i++) {
+		auto& entry = s_sensorRegistry[i];
+		const char* name = s_sensorNames[i];
+
+		if (entry.useMock) {
+			scheduleMsg(logger, "Sensor \"%s\" mocked with value %.2f", name, entry.mockValue);
+		} else {
+			const auto sensor = entry.sensor;
+
+			if (sensor) {
+				sensor->showInfo(logger, name);
+			} else {
+				scheduleMsg(logger, "Sensor \"%s\" is not configured.", name);
+			}
+		}
+	}
+}

firmware/controllers/sensors/sensor.h

@@ -68,6 +68,7 @@ struct SensorResult {
 
 // Fwd declare - nobody outside of Sensor.cpp needs to see inside this type
 struct SensorRegistryEntry;
+class Logging;
 
 class Sensor {
 public:
@@ -78,6 +79,12 @@ public:
 	// done internally!
 	[[nodiscard]] bool Register();
 
+	// Print information about this sensor
+	virtual void showInfo(Logging* logger, const char* sensorName) const = 0;
+
+	// Print information about all sensors
+	static void showAllSensorInfo(Logging* logger);
+
 	// Remove all sensors from the sensor registry - tread carefully if you use this outside of a unit test
 	static void resetRegistry();
 
@@ -121,6 +128,8 @@ protected:
 	explicit Sensor(SensorType type)
 		: m_type(type) {}
 
+	static const char* getSensorName(SensorType type);
+
 private:
 	// Retrieve the current reading from the sensor.
 	//

firmware/controllers/sensors/sensor_info_printing.cpp

@@ -0,0 +1,13 @@
+#include "proxy_sensor.h"
+#include "functional_sensor.h"
+#include "efilib.h"
+#include "loggingcentral.h"
+
+void ProxySensor::showInfo(Logging* logger, const char* sensorName) const {
+	scheduleMsg(logger, "Sensor \"%s\" proxied from sensor \"%s\"", sensorName, getSensorName(m_proxiedSensor));
+}
+
+void FunctionalSensor::showInfo(Logging* logger, const char* sensorName) const {
+	const auto [valid, value] = get();
+	scheduleMsg(logger, "Sensor \"%s\": Raw value: %.2f Valid: %d Converted value %.2f", sensorName, m_rawValue, valid, value);
+}

firmware/controllers/sensors/sensors.mk

@@ -11,6 +11,7 @@ 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/sensor_info_printing.cpp \
 	$(PROJECT_DIR)/controllers/sensors/functional_sensor.cpp \
 	$(PROJECT_DIR)/controllers/sensors/converters/linear_func.cpp \
 	$(PROJECT_DIR)/controllers/sensors/converters/resistance_func.cpp \

firmware/init/init.h

@@ -4,8 +4,10 @@
 
 #pragma once
 
+class Logging;
+
 // Call this once at startup to initialize, configure, and subscribe sensors
-void initSensors();
+void initNewSensors(Logging* logger);
 
 // Call this whenever the configuration may have changed, so any sensors
 // can be reconfigured with the new settings.

firmware/init/sensor/init_sensors.cpp

@@ -6,18 +6,18 @@
 #include "init.h"
 #include "sensor.h"
 
-static void initSensorCli();
+static void initSensorCli(Logging* logger);
 
 // Sensor init/config
 void initTps();
 void initOilPressure();
 
-void initSensors() {
+void initNewSensors(Logging* logger) {
 	initTps();
 	initOilPressure();
 
 	// Init CLI functionality for sensors (mocking)
-	initSensorCli();
+	initSensorCli(logger);
 }
 
 // Sensor reconfiguration
@@ -29,8 +29,19 @@ void reconfigureSensors() {
 	reconfigureOilPressure();
 }
 
+static Logging* s_logger;
+
 // Mocking/testing helpers
-static void initSensorCli() {
+static void initSensorCli(Logging* logger) {
+	s_logger = logger;
+
 	addConsoleActionIF("set_sensor_mock", Sensor::setMockValue);
 	addConsoleAction("reset_sensor_mocks", Sensor::resetAllMocks);
+	addConsoleAction("show_sensors", []() { Sensor::showAllSensorInfo(s_logger); });
+	addConsoleActionI("show_sensor", 
+		[](int idx) {
+			if (auto s = Sensor::getSensorOfType(static_cast<SensorType>(idx))) {
+				s->showAllSensorInfo(s_logger);
+			}
+		});
 }

firmware/util/efilib.h

@@ -45,19 +45,6 @@ int indexOf(const char *string, char ch);
 float atoff(const char *string);
 int atoi(const char *string);
 
-#if defined(__cplusplus) && defined(__OPTIMIZE__)
-#include <type_traits>
-// "g++ -O2" version, adds more strict type check and yet no "strict-aliasing" warnings!
-#define cisnan(f) ({ \
-	static_assert(sizeof(f) == sizeof(int32_t)); \
-	union cisnanu_t { std::remove_reference_t<decltype(f)> __f; int32_t __i; } __cisnan_u = { f }; \
-	__cisnan_u.__i == 0x7FC00000; \
-})
-#else
-// "g++ -O0" or other C++/C compilers
-#define cisnan(f) (*(((int*) (&f))) == 0x7FC00000)
-#endif /* __cplusplus && __OPTIMIZE__ */
-
 #define UNUSED(x) (void)(x)
   
 int absI(int32_t value);
@@ -124,4 +111,17 @@ constexpr void copyArrayPartial(TElement (&dest)[NDest], const TElement (&src)[N
 
 #endif /* __cplusplus */
 
+#if defined(__cplusplus) && defined(__OPTIMIZE__)
+#include <type_traits>
+// "g++ -O2" version, adds more strict type check and yet no "strict-aliasing" warnings!
+#define cisnan(f) ({ \
+	static_assert(sizeof(f) == sizeof(int32_t)); \
+	union cisnanu_t { std::remove_reference_t<decltype(f)> __f; int32_t __i; } __cisnan_u = { f }; \
+	__cisnan_u.__i == 0x7FC00000; \
+})
+#else
+// "g++ -O0" or other C++/C compilers
+#define cisnan(f) (*(((int*) (&f))) == 0x7FC00000)
+#endif /* __cplusplus && __OPTIMIZE__ */
+
 #endif /* EFILIB_H_ */

firmware/util/loggingcentral.h

@@ -7,6 +7,8 @@
 #ifndef UTIL_LOGGINGCENTRAL_H_
 #define UTIL_LOGGINGCENTRAL_H_
 
+class Logging;
+
 void initLoggingCentral(void);
 char * swapOutputBuffers(int *actualOutputBufferSize);
 void scheduleMsg(Logging *logging, const char *fmt, ...);

unit_tests/tests/sensor/mock/mock_sensor.h

@@ -18,4 +18,6 @@ struct MockSensor final : public StoredValueSensor
 	{
 		StoredValueSensor::invalidate();
 	}
+
+	void showInfo(Logging* logger, const char* name) const override {}
 };