encapsulate entry logic in to entry (#2018)

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

firmware/controllers/sensors/sensor.cpp

@@ -3,51 +3,6 @@
 #include "efilib.h"
 #include "loggingcentral.h"
 
-// This struct represents one sensor in the registry.
-// It stores whether the sensor should use a mock value,
-// the value to use, and if not a pointer to the sensor that
-// can provide a real value.
-class SensorRegistryEntry {
-public:
-	Sensor *getSensor() {
-		return sensor;
-	}
-
-	void setSensor(Sensor * sensor) {
-		this->sensor = sensor;
-	}
-
-	bool isMock() {
-		return useMock;
-	}
-
-	void setMockValue(float value) {
-		mockValue = value;
-		useMock = true;
-	}
-
-	float getMockValue() {
-		return mockValue;
-	}
-
-	void resetMock() {
-		useMock = false;
-		mockValue = 0.0f;
-	}
-
-	void reset() {
-		sensor = nullptr;
-		resetMock();
-	}
-
-private:
-	bool useMock = false;
-	float mockValue;
-	Sensor *sensor = nullptr;
-};
-
-static SensorRegistryEntry s_sensorRegistry[static_cast<size_t>(SensorType::PlaceholderLast)] = {};
-
 static const char* s_sensorNames[] = {
 	"Invalid",
 	"CLT",
@@ -85,26 +40,106 @@ static const char* s_sensorNames[] = {
 	"Idle Valve Position",
 };
 
-static_assert(efi::size(s_sensorNames) == efi::size(s_sensorRegistry));
+// This struct represents one sensor in the registry.
+// It stores whether the sensor should use a mock value,
+// the value to use, and if not a pointer to the sensor that
+// can provide a real value.
+class SensorRegistryEntry {
+public:
+	const Sensor* getSensor() {
+		return m_sensor;
+	}
 
-bool Sensor::Register() {
+	void setMockValue(float value) {
-	// Get a ref to where we should be
+		m_mockValue = value;
-	auto &entry = s_sensorRegistry[getIndex()];
+		m_useMock = true;
+	}
 
+	void resetMock() {
+		m_useMock = false;
+		m_mockValue = 0.0f;
+	}
+
+	void reset() {
+		m_sensor = nullptr;
+		resetMock();
+	}
+
+	bool Register(Sensor* sensor) {
 		// If there's somebody already here - a consumer tried to double-register a sensor
-	if (entry.getSensor()) {
+		if (m_sensor) {
 			// This sensor has already been registered. Don't re-register it.
 	#if ! EFI_UNIT_TEST
-		firmwareError(CUSTOM_OBD_26, "Duplicate registration for %s sensor", s_sensorNames[getIndex()]);
+			firmwareError(CUSTOM_OBD_26, "Duplicate registration for %s sensor", sensor->getSensorName());
 	#endif
 			return false;
 		} else {
-		// put ourselves in the registry
+			// Put the sensor in the registry
-		entry.setSensor(this);
+			m_sensor = sensor;
 			return true;
 		}
 	}
 
+	SensorResult get() const {
+		// Check if mock
+		if (m_useMock) {
+			return m_mockValue;
+		}
+
+		// Get the sensor out of the entry
+		const Sensor *s = m_sensor;
+		if (s) {
+			// If we found the sensor, ask it for a result.
+			return s->get();
+		}
+
+		// We've exhausted all valid ways to return something - sensor not found.
+		return unexpected;
+	}
+
+	void showInfo(Logging* logger, const char* sensorName) const {
+		if (m_useMock) {
+			scheduleMsg(logger, "Sensor \"%s\" mocked with value %.2f", sensorName, m_mockValue);
+		} else {
+			const auto sensor = m_sensor;
+
+			if (sensor) {
+				sensor->showInfo(logger, sensorName);
+			} else {
+				scheduleMsg(logger, "Sensor \"%s\" is not configured.", sensorName);
+			}
+		}
+	}
+
+	bool hasSensor() const {
+		return m_useMock || m_sensor;
+	}
+
+	float getRaw() const {
+		const auto sensor = m_sensor;
+
+		if (m_sensor) {
+			return m_sensor->getRaw();
+		}
+
+		// We've exhausted all valid ways to return something - sensor not found.
+		return 0;
+	}
+
+private:
+	bool m_useMock = false;
+	float m_mockValue;
+	Sensor* m_sensor = nullptr;
+};
+
+static SensorRegistryEntry s_sensorRegistry[static_cast<size_t>(SensorType::PlaceholderLast)] = {};
+
+static_assert(efi::size(s_sensorNames) == efi::size(s_sensorRegistry));
+
+bool Sensor::Register() {
+	return s_sensorRegistry[getIndex()].Register(this);
+}
+
 /*static*/ void Sensor::resetRegistry() {
 	// Clear all entries
 	for (size_t i = 0; i < efi::size(s_sensorRegistry); i++) {
@@ -137,47 +172,19 @@ bool Sensor::Register() {
 		return unexpected;
 	}
 
-	// Next check for mock
+	return entry->get();
-	if (entry->isMock()) {
-		return entry->getMockValue();
-	}
-
-	// Get the sensor out of the entry
-	const Sensor *s = entry->getSensor();
-	if (s) {
-		// If we found the sensor, ask it for a result.
-		return s->get();
-	}
-
-	// We've exhausted all valid ways to return something - sensor not found.
-	return unexpected;
 }
 
 /*static*/ float Sensor::getRaw(SensorType type) {
 	const auto entry = getEntryForType(type);
 
-	// Check if this is a valid sensor entry
+	return entry ? entry->getRaw() : 0;
-	if (!entry) {
-		return 0;
-	}
-
-	const auto s = entry->getSensor();
-	if (s) {
-		return s->getRaw();
-	}
-
-	// We've exhausted all valid ways to return something - sensor not found.
-	return 0;
 }
 
 /*static*/ bool Sensor::hasSensor(SensorType type) {
 	const auto entry = getEntryForType(type);
 
-	if (!entry) {
+	return entry ? entry->hasSensor() : false;
-		return false;
-	}
-
-	return entry->isMock() || entry->getSensor();
 }
 
 /*static*/ void Sensor::setMockValue(SensorType type, float value) {
@@ -208,9 +215,7 @@ bool Sensor::Register() {
 /*static*/ void Sensor::resetAllMocks() {
 	// Reset all mocks
 	for (size_t i = 0; i < efi::size(s_sensorRegistry); i++) {
-		auto &entry = s_sensorRegistry[i];
+		s_sensorRegistry[i].resetMock();
-
-		entry.resetMock();
 	}
 }
 
@@ -224,16 +229,15 @@ bool Sensor::Register() {
 		auto& entry = s_sensorRegistry[i];
 		const char* name = s_sensorNames[i];
 
-		if (entry.isMock()) {
+		entry.showInfo(logger, name);
-			scheduleMsg(logger, "Sensor \"%s\" mocked with value %.2f", name, entry.getMockValue());
+	}
-		} else {
+}
-			const auto sensor = entry.getSensor();
 
-			if (sensor) {
+// Print information about a particular sensor
-				sensor->showInfo(logger, name);
+/*static*/ void Sensor::showInfo(Logging* logger, SensorType type) {
-			} else {
+	auto entry = getEntryForType(type);
-				scheduleMsg(logger, "Sensor \"%s\" is not configured.", name);
+
-			}
+	if (entry) {
-		}
+		entry->showInfo(logger, getSensorName(type));
 	}
 }

firmware/controllers/sensors/sensor.h

@@ -71,6 +71,9 @@ public:
 	// Print information about all sensors
 	static void showAllSensorInfo(Logging* logger);
 
+	// Print information about a particular sensor
+	static void showInfo(Logging* logger, SensorType type);
+
 	// Remove all sensors from the sensor registry - tread carefully if you use this outside of a unit test
 	static void resetRegistry();
 
@@ -127,6 +130,13 @@ public:
 	// this should be field lookup and simple math.
 	virtual SensorResult get() const = 0;
 
+	/*
+	 * Get an unconverted value from the sensor, if available.
+	 */
+	virtual float getRaw() const {
+		return 0;
+	}
+
 protected:
 	// Protected constructor - only subclasses call this
 	explicit Sensor(SensorType type)
@@ -135,14 +145,6 @@ protected:
 	static const char* getSensorName(SensorType type);
 
 private:
-
-	/*
-	 * Get an unconverted value from the sensor, if available.
-	 */
-	virtual float getRaw() const {
-		return 0;
-	}
-
 	const SensorType m_type;
 
 	// Get this sensor's index in the list

firmware/init/sensor/init_sensors.cpp

@@ -39,8 +39,6 @@ static void initSensorCli(Logging* logger) {
 	addConsoleAction("show_sensors", []() { Sensor::showAllSensorInfo(s_logger); });
 	addConsoleActionI("show_sensor", 
 		[](int idx) {
-			if (auto s = Sensor::getSensorOfType(static_cast<SensorType>(idx))) {
+			Sensor::showInfo(s_logger, static_cast<SensorType>(idx));
-				s->showAllSensorInfo(s_logger);
-			}
 		});
 }