Adc isAdcChannelValid helper (#2188)

* Guard define argument

* ADC: isAdcChannelValid

Check for both <= EFI_ADC_NONE and >= EFI_ADC_LAST_CHANNEL
Also check for value out of enum range (corrupted settings)

* Fix unit tests

firmware/console/status_loop.cpp

@@ -772,7 +772,7 @@ void updateTunerStudioState(TunerStudioOutputChannels *tsOutputChannels DECLARE_
 		break;
 	case DBG_FSIO_ADC:
 		// todo: implement a proper loop
-		if (engineConfiguration->fsioAdc[0] != EFI_ADC_NONE) {
+		if (isAdcChannelValid(engineConfiguration->fsioAdc[0])) {
 			tsOutputChannels->debugFloatField1 = getVoltage("fsio", engineConfiguration->fsioAdc[0] PASS_ENGINE_PARAMETER_SUFFIX);
 		}
 		break;
@@ -829,13 +829,13 @@ void updateTunerStudioState(TunerStudioOutputChannels *tsOutputChannels DECLARE_
 		break;
 #endif /* EFI_CAN_SUPPORT */
 	case DBG_ANALOG_INPUTS:
-		tsOutputChannels->debugFloatField1 = (engineConfiguration->vbattAdcChannel != EFI_ADC_NONE) ? getVoltageDivided("vbatt", engineConfiguration->vbattAdcChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
+		tsOutputChannels->debugFloatField1 = isAdcChannelValid(engineConfiguration->vbattAdcChannel) ? getVoltageDivided("vbatt", engineConfiguration->vbattAdcChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
 		tsOutputChannels->debugFloatField2 = Sensor::getRaw(SensorType::Tps1);
-		tsOutputChannels->debugFloatField3 = (engineConfiguration->mafAdcChannel != EFI_ADC_NONE) ? getVoltageDivided("maf", engineConfiguration->mafAdcChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
+		tsOutputChannels->debugFloatField3 = isAdcChannelValid(engineConfiguration->mafAdcChannel) ? getVoltageDivided("maf", engineConfiguration->mafAdcChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
-		tsOutputChannels->debugFloatField4 = (engineConfiguration->map.sensor.hwChannel != EFI_ADC_NONE) ? getVoltageDivided("map", engineConfiguration->map.sensor.hwChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
+		tsOutputChannels->debugFloatField4 = isAdcChannelValid(engineConfiguration->map.sensor.hwChannel) ? getVoltageDivided("map", engineConfiguration->map.sensor.hwChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
-		tsOutputChannels->debugFloatField5 = (engineConfiguration->clt.adcChannel != EFI_ADC_NONE) ? getVoltageDivided("clt", engineConfiguration->clt.adcChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
+		tsOutputChannels->debugFloatField5 = isAdcChannelValid(engineConfiguration->clt.adcChannel) ? getVoltageDivided("clt", engineConfiguration->clt.adcChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
-		tsOutputChannels->debugFloatField6 = (engineConfiguration->iat.adcChannel != EFI_ADC_NONE) ? getVoltageDivided("iat", engineConfiguration->iat.adcChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
+		tsOutputChannels->debugFloatField6 = isAdcChannelValid(engineConfiguration->iat.adcChannel) ? getVoltageDivided("iat", engineConfiguration->iat.adcChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
-		tsOutputChannels->debugFloatField7 = (engineConfiguration->afr.hwChannel != EFI_ADC_NONE) ? getVoltageDivided("ego", engineConfiguration->afr.hwChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
+		tsOutputChannels->debugFloatField7 = isAdcChannelValid(engineConfiguration->afr.hwChannel) ? getVoltageDivided("ego", engineConfiguration->afr.hwChannel PASS_ENGINE_PARAMETER_SUFFIX) : 0.0f;
 		break;
 	case DBG_ANALOG_INPUTS2:
 		// TPS 1 pri/sec split

firmware/controllers/algo/engine.cpp

@@ -217,7 +217,7 @@ void Engine::periodicSlowCallback(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
 	slowCallBackWasInvoked = true;
 
 #if ANALOG_HW_CHECK_MODE
-	efiAssertVoid(OBD_PCM_Processor_Fault, CONFIG(clt).adcChannel != EFI_ADC_NONE, "No CLT setting");
+	efiAssertVoid(OBD_PCM_Processor_Fault, isAdcChannelValid(CONFIG(clt).adcChannel), "No CLT setting");
 	efitimesec_t secondsNow = getTimeNowSeconds();
 	if (secondsNow > 2 && secondsNow < 180) {
 		assertCloseTo("RPM", Sensor::get(SensorType::Rpm).Value, HW_CHECK_RPM);
@@ -256,7 +256,7 @@ void Engine::updateSlowSensors(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
 	engineState.updateSlowSensors(PASS_ENGINE_PARAMETER_SIGNATURE);
 
 	// todo: move this logic somewhere to sensors folder?
-	if (CONFIG(fuelLevelSensor) != EFI_ADC_NONE) {
+	if (isAdcChannelValid(CONFIG(fuelLevelSensor))) {
 		float fuelLevelVoltage = getVoltageDivided("fuel", engineConfiguration->fuelLevelSensor PASS_ENGINE_PARAMETER_SUFFIX);
 		sensors.fuelTankLevel = interpolateMsg("fgauge", CONFIG(fuelLevelEmptyTankVoltage), 0,
 				CONFIG(fuelLevelFullTankVoltage), 100,

firmware/controllers/engine_controller.cpp

@@ -280,7 +280,7 @@ void initPeriodicEvents(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
 
 char * getPinNameByAdcChannel(const char *msg, adc_channel_e hwChannel, char *buffer) {
 #if HAL_USE_ADC
-	if (hwChannel == EFI_ADC_NONE) {
+	if (!isAdcChannelValid(hwChannel)) {
 		strcpy(buffer, "NONE");
 	} else {
 		strcpy(buffer, portname(getAdcChannelPort(msg, hwChannel)));
@@ -301,7 +301,7 @@ extern AdcDevice fastAdc;
 static void printAnalogChannelInfoExt(const char *name, adc_channel_e hwChannel, float adcVoltage,
 		float dividerCoeff) {
 #if HAL_USE_ADC
-	if (hwChannel == EFI_ADC_NONE) {
+	if (!isAdcChannelValid(hwChannel)) {
 		scheduleMsg(&logger, "ADC is not assigned for %s", name);
 		return;
 	}
@@ -687,7 +687,7 @@ void initEngineContoller(Logging *sharedLogger DECLARE_ENGINE_PARAMETER_SUFFIX)
 
 	initEgoAveraging(PASS_ENGINE_PARAMETER_SIGNATURE);
 
-	if (engineConfiguration->externalKnockSenseAdc != EFI_ADC_NONE) {
+	if (isAdcChannelValid(engineConfiguration->externalKnockSenseAdc)) {
 		addConsoleAction("knockinfo", getKnockInfo);
 	}
 

firmware/controllers/engine_cycle/main_trigger_callback.cpp

@@ -450,7 +450,7 @@ void mainTriggerCallback(uint32_t trgEventIndex, efitick_t edgeTimestamp DECLARE
 	}
 
 	if (trgEventIndex == (uint32_t)CONFIG(ignMathCalculateAtIndex)) {
-		if (CONFIG(externalKnockSenseAdc) != EFI_ADC_NONE) {
+		if (isAdcChannelValid(CONFIG(externalKnockSenseAdc))) {
 			float externalKnockValue = getVoltageDivided("knock", engineConfiguration->externalKnockSenseAdc PASS_ENGINE_PARAMETER_SUFFIX);
 			engine->knockLogic(externalKnockValue PASS_ENGINE_PARAMETER_SUFFIX);
 		}

firmware/controllers/sensors/ego.cpp

@@ -105,7 +105,7 @@ bool hasAfrSensor(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
 		return cjHasAfrSensor(PASS_ENGINE_PARAMETER_SIGNATURE);
 	}
 #endif /* EFI_CJ125 && HAL_USE_SPI */
-	return engineConfiguration->afr.hwChannel != EFI_ADC_NONE;
+	return isAdcChannelValid(engineConfiguration->afr.hwChannel);
 }
 
 extern float InnovateLC2AFR;
@@ -123,7 +123,7 @@ float getAfr(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
 #endif /* EFI_CJ125 && HAL_USE_SPI */
 	afr_sensor_s * sensor = &CONFIG(afr);
 
-	if (engineConfiguration->afr.hwChannel == EFI_ADC_NONE) {
+	if (!isAdcChannelValid(engineConfiguration->afr.hwChannel)) {
 		return 0;
 	}
 

firmware/controllers/sensors/maf.cpp

@@ -13,7 +13,7 @@ float getMafVoltage(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
 }
 
 bool hasMafSensor(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
-	return engineConfiguration->mafAdcChannel != EFI_ADC_NONE;
+	return isAdcChannelValid(engineConfiguration->mafAdcChannel);
 }
 
 /**

firmware/controllers/sensors/map.cpp

@@ -172,11 +172,11 @@ float getRawMap(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
  * Also if 'useFixedBaroCorrFromMap' option is enabled, and we have the initial pressure value stored and passed validation.
  */
 bool hasBaroSensor(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
-	return engineConfiguration->baroSensor.hwChannel != EFI_ADC_NONE || !cisnan(storedInitialBaroPressure);
+	return isAdcChannelValid(engineConfiguration->baroSensor.hwChannel) || !cisnan(storedInitialBaroPressure);
 }
 
 bool hasMapSensor(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
-	return engineConfiguration->map.sensor.hwChannel != EFI_ADC_NONE;
+	return isAdcChannelValid(engineConfiguration->map.sensor.hwChannel);
 }
 
 float getBaroPressure(DECLARE_ENGINE_PARAMETER_SIGNATURE) {

firmware/controllers/sensors/voltage.cpp

@@ -15,7 +15,7 @@
 EXTERN_ENGINE;
 
 bool hasVBatt(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
-	return engineConfiguration->vbattAdcChannel != EFI_ADC_NONE;
+	return isAdcChannelValid(engineConfiguration->vbattAdcChannel);
 }
 
 float getVBatt(DECLARE_ENGINE_PARAMETER_SIGNATURE) {

firmware/hw_layer/adc/adc_external.h

@@ -12,6 +12,5 @@
 #include "mcp3208.h"
 
 #define getAdcValue(channel) getMcp3208adc(channel)
-#define adcToVoltsDivided(adc) (5.0f / 4095 * adc)
+#define adcToVoltsDivided(adc) (5.0f / 4095 * (adc))
-#define getVoltageDivided(msg, channel) (channel == EFI_ADC_NONE ? 66.66 : adcToVoltsDivided(getAdcValue(msg, channel)))
+#define getVoltageDivided(msg, channel) (isAdcChannelValid(channel) ? adcToVoltsDivided(getAdcValue(msg, channel)) : 66.66)
-

firmware/hw_layer/adc/adc_inputs.cpp

@@ -268,7 +268,7 @@ float getMCUInternalTemperature() {
 }
 
 int getInternalAdcValue(const char *msg, adc_channel_e hwChannel) {
-	if (hwChannel == EFI_ADC_NONE) {
+	if (!isAdcChannelValid(hwChannel)) {
 		warning(CUSTOM_OBD_ANALOG_INPUT_NOT_CONFIGURED, "ADC: %s input is not configured", msg);
 		return -1;
 	}
@@ -410,7 +410,7 @@ static void printFullAdcReport(Logging *logger) {
 
 		adc_channel_e hwIndex = fastAdc.getAdcHardwareIndexByInternalIndex(index);
 
-		if (hwIndex != EFI_ADC_NONE && hwIndex != EFI_ADC_ERROR) {
+		if (isAdcChannelValid(hwIndex)) {
 			ioportid_t port = getAdcChannelPort("print", hwIndex);
 			int pin = getAdcChannelPin(hwIndex);
 
@@ -430,7 +430,7 @@ static void printFullAdcReport(Logging *logger) {
 
 		adc_channel_e hwIndex = slowAdc.getAdcHardwareIndexByInternalIndex(index);
 
-		if (hwIndex != EFI_ADC_NONE && hwIndex != EFI_ADC_ERROR) {
+		if (isAdcChannelValid(hwIndex)) {
 			ioportid_t port = getAdcChannelPort("print", hwIndex);
 			int pin = getAdcChannelPin(hwIndex);
 
@@ -516,7 +516,7 @@ public:
 };
 
 void addChannel(const char *name, adc_channel_e setting, adc_channel_mode_e mode) {
-	if (setting == EFI_ADC_NONE) {
+	if (!isAdcChannelValid(setting)) {
 		return;
 	}
 	if (/*type-limited (int)setting < 0 || */(int)setting>=HW_MAX_ADC_INDEX) {
@@ -532,7 +532,7 @@ void addChannel(const char *name, adc_channel_e setting, adc_channel_mode_e mode
 
 void removeChannel(const char *name, adc_channel_e setting) {
 	(void)name;
-	if (setting == EFI_ADC_NONE) {
+	if (!isAdcChannelValid(setting)) {
 		return;
 	}
 	adcHwChannelEnabled[setting] = ADC_OFF;

firmware/hw_layer/adc/adc_inputs.h

@@ -13,6 +13,21 @@
 
 #define SLOW_ADC_RATE 500
 
+static inline bool isAdcChannelValid(adc_channel_e hwChannel) {
+	if (hwChannel <= EFI_ADC_NONE) {
+		return false;
+	} else if (hwChannel >= EFI_ADC_LAST_CHANNEL) {
+		/* this should not happen!
+		 * if we have enum out of range somewhere in settings
+		 * that means something goes terribly wrong
+		 * TODO: should we say something?
+		 */
+		return false;
+	} else {
+		return true;
+	}
+}
+
 #if HAL_USE_ADC
 
 adc_channel_mode_e getAdcMode(adc_channel_e hwChannel);

firmware/hw_layer/adc/adc_subscription.cpp

@@ -36,7 +36,7 @@ void AdcSubscription::SubscribeSensor(FunctionalSensor &sensor,
 									  float lowpassCutoff,
 									  float voltsPerAdcVolt /*= 0.0f*/) {
 	// Don't subscribe null channels
-	if (channel == EFI_ADC_NONE) {
+	if (!isAdcChannelValid(channel)) {
 		return;
 	}
 

firmware/hw_layer/hardware.cpp

@@ -250,17 +250,15 @@ static void calcFastAdcIndexes(void) {
 #if HAL_USE_ADC
 	fastMapSampleIndex = fastAdc.internalAdcIndexByHardwareIndex[engineConfiguration->map.sensor.hwChannel];
 	hipSampleIndex =
-			engineConfiguration->hipOutputChannel == EFI_ADC_NONE ?
+			isAdcChannelValid(engineConfiguration->hipOutputChannel) ?
-					-1 : fastAdc.internalAdcIndexByHardwareIndex[engineConfiguration->hipOutputChannel];
+					fastAdc.internalAdcIndexByHardwareIndex[engineConfiguration->hipOutputChannel] : -1;
-	if (engineConfiguration->tps1_1AdcChannel != EFI_ADC_NONE) {
+	tpsSampleIndex =
-		tpsSampleIndex = fastAdc.internalAdcIndexByHardwareIndex[engineConfiguration->tps1_1AdcChannel];
+			isAdcChannelValid(engineConfiguration->tps1_1AdcChannel) ?
-	} else {
+					fastAdc.internalAdcIndexByHardwareIndex[engineConfiguration->tps1_1AdcChannel] : TPS_IS_SLOW;
-		tpsSampleIndex = TPS_IS_SLOW;
-	}
 #if HAL_TRIGGER_USE_ADC
 	adc_channel_e triggerChannel = getAdcChannelForTrigger();
-	triggerSampleIndex = (triggerChannel == EFI_ADC_NONE) ?
+	triggerSampleIndex = isAdcChannelValid(triggerChannel) ?
-		-1 : fastAdc.internalAdcIndexByHardwareIndex[triggerChannel];
+		fastAdc.internalAdcIndexByHardwareIndex[triggerChannel] : -1;
 #endif /* HAL_TRIGGER_USE_ADC */
 
 #endif/* HAL_USE_ADC */

firmware/hw_layer/sensors/cj125.cpp

@@ -127,7 +127,7 @@ static void cjWriteRegister(uint8_t regAddr, uint8_t regValue) {
 
 static float getUr() {
 #if ! EFI_UNIT_TEST
-	if (CONFIG(cj125ur) != EFI_ADC_NONE) {
+	if (isAdcChannelValid(CONFIG(cj125ur))) {
 #if EFI_PROD_CODE
 	if (!engineConfiguration->cj125isUrDivided) {
 		// in case of directly connected Ur signal from CJ125 to the ADC pin of MCU
@@ -146,7 +146,7 @@ static float getUr() {
 
 static float getUa() {
 #if ! EFI_UNIT_TEST
-	if (CONFIG(cj125ua) != EFI_ADC_NONE) {
+	if (isAdcChannelValid(CONFIG(cj125ua))) {
 #if EFI_PROD_CODE
 		if (engineConfiguration->cj125isUaDivided) {
 			return getVoltageDivided("cj125ua", CONFIG(cj125ua) PASS_ENGINE_PARAMETER_SUFFIX);
@@ -618,7 +618,7 @@ void initCJ125(Logging *sharedLogger DECLARE_ENGINE_PARAMETER_SUFFIX) {
 		return;
 	}
 
-	if (CONFIG(cj125ur) == EFI_ADC_NONE || CONFIG(cj125ua) == EFI_ADC_NONE) {
+	if (!isAdcChannelValid(CONFIG(cj125ur)) || !isAdcChannelValid(CONFIG(cj125ua))) {
 		scheduleMsg(logger, "cj125 init error! cj125ur and cj125ua are required.");
 		warning(CUSTOM_CJ125_0, "cj ur ua");
 		globalInstance.errorCode = CJ125_ERROR_DISABLED;

firmware/hw_layer/trigger_input_adc.cpp

@@ -299,7 +299,7 @@ adc_channel_e getAdcChannelForTrigger(void) {
 
 void addAdcChannelForTrigger(void) {
 	adc_channel_e ch = getAdcChannelForTrigger();
-	if (ch != EFI_ADC_NONE) {
+	if (isAdcChannelValid(ch)) {
 		addChannel("TRIG", ch, ADC_FAST);
 	}
 }

firmware/init/sensor/init_fluid_pressure.cpp

@@ -1,4 +1,5 @@
 #include "init.h"
+#include "adc_inputs.h"
 #include "adc_subscription.h"
 #include "engine.h"
 #include "error_handling.h"
@@ -39,7 +40,7 @@ static void initFluidPressure(LinearFunc& func, FunctionalSensor& sensor, const
 	auto channel = cfg.hwChannel;
 
 	// Only register if we have a sensor
-	if (channel == EFI_ADC_NONE) {
+	if (!isAdcChannelValid(channel)) {
 		return;
 	}
 

firmware/init/sensor/init_thermistors.cpp

@@ -1,7 +1,8 @@
+#include "global.h"
+#include "adc_inputs.h"
 #include "adc_subscription.h"
 #include "engine.h"
 #include "error_handling.h"
-#include "global.h"
 #include "functional_sensor.h"
 #include "func_chain.h"
 #include "linear_func.h"
@@ -54,7 +55,7 @@ static void configureTempSensor(FunctionalSensor &sensor,
 	auto channel = config.adcChannel;
 
 	// Only register if we have a sensor
-	if (channel == EFI_ADC_NONE) {
+	if (!isAdcChannelValid(channel)) {
 		return;
 	}
 

firmware/init/sensor/init_tps.cpp

@@ -1,7 +1,8 @@
+#include "global.h"
+#include "adc_inputs.h"
 #include "adc_subscription.h"
 #include "engine.h"
 #include "error_handling.h"
-#include "global.h"
 #include "functional_sensor.h"
 #include "redundant_sensor.h"
 #include "proxy_sensor.h"
@@ -41,7 +42,7 @@ FunctionalSensor idlePosSens(SensorType::IdlePosition, MS2NT(10));
 
 static bool configureTps(LinearFunc& func, adc_channel_e channel, float closed, float open, float min, float max, const char* msg) {
 	// Only configure if we have a channel
-	if (channel == EFI_ADC_NONE || channel >= EFI_ADC_LAST_CHANNEL) {
+	if (!isAdcChannelValid(channel)) {
 		return false;
 	}
 
@@ -106,7 +107,7 @@ void initTps(DECLARE_CONFIG_PARAMETER_SIGNATURE) {
 	}
 
 	// Route the pedal or TPS to driverIntent as appropriate
-	if (CONFIG(throttlePedalPositionAdcChannel) != EFI_ADC_NONE) {
+	if (isAdcChannelValid(CONFIG(throttlePedalPositionAdcChannel))) {
 		driverIntent.setProxiedSensor(SensorType::AcceleratorPedal);
 	} else {
 		driverIntent.setProxiedSensor(SensorType::Tps1);

unit_tests/adc_inputs.h

@@ -7,4 +7,19 @@
 
 #pragma once
 
+static inline bool isAdcChannelValid(adc_channel_e hwChannel) {
+	if (hwChannel <= EFI_ADC_NONE) {
+		return false;
+	} else if (hwChannel >= EFI_ADC_LAST_CHANNEL) {
+		/* this should not happen!
+		 * if we have enum out of range somewhere in settings
+		 * that means something goes terribly wrong
+		 * TODO: should we say something?
+		 */
+		return false;
+	} else {
+		return true;
+	}
+}
+
 #include "boards.h"