read configuration from flash exactly once (#2481)

* early hw init

* s

* read configuration exactly once

* cleanup

* housekeeping

* test friendly

* ugh bad merge

* that is a noop

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

firmware/config/boards/hellen/cypress/efifeatures.h

@@ -391,8 +391,6 @@
 #define EFI_PRINT_ERRORS_AS_WARNINGS TRUE
 //#define EFI_PRINT_MESSAGES_TO_TERMINAL TRUE
 
-//#define EFI_ACTIVE_CONFIGURATION_IN_FLASH (FLASH_ADDR + offsetof(persistent_config_container_s, persistentConfiguration.engineConfiguration))
-
 //#define PWM_PHASE_MAX_COUNT 122
 
 //!!!!!!!!!!!!!!!!!!!!!!

firmware/console/binary/tunerstudio.cpp

@@ -256,7 +256,6 @@ static void onlineApplyWorkingCopyBytes(uint32_t offset, int count) {
 	// open question what's the best strategy to balance coding efforts, performance matters and tune crc functionality
 	// open question what is the runtime cost of wiping 2K of bytes on each IO communication, could be that 2K of byte memset
 	// is negligable comparing with the IO costs?
-	//		wipeStrings(PASS_ENGINE_PARAMETER_SIGNATURE);
 }
 
 static const void * getStructAddr(int structId) {

firmware/controllers/algo/engine_configuration.cpp

@@ -30,6 +30,7 @@
 #include "speed_density.h"
 #include "advance_map.h"
 #include "sensor.h"
+#include "flash_main.h"
 
 #include "hip9011_lookup.h"
 #include "hip9011_logic.h"
@@ -151,7 +152,7 @@ engine_configuration_s & activeConfiguration = activeConfigurationLocalStorage;
 
 extern engine_configuration_s *engineConfiguration;
 
-void rememberCurrentConfiguration(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
+static void rememberCurrentConfiguration(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
 #if ! EFI_ACTIVE_CONFIGURATION_IN_FLASH
 	memcpy(&activeConfiguration, engineConfiguration, sizeof(engine_configuration_s));
 #else
@@ -164,13 +165,11 @@ extern LoggingWithStorage sharedLogger;
 static void wipeString(char *string, int size) {
 	// we have to reset bytes after \0 symbol in order to calculate correct tune CRC from MSQ file
 	for (int i = strlen(string) + 1; i < size; i++) {
-		// todo: open question if it's worth replacing for loop with a memset. would a memset be much faster?
-		// do we care about performance here?
 		string[i] = 0;
 	}
 }
 
-void wipeStrings(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
+static void wipeStrings(DECLARE_ENGINE_PARAMETER_SIGNATURE) {
 	wipeString(engineConfiguration->engineMake, sizeof(vehicle_info_t));
 	wipeString(engineConfiguration->engineCode, sizeof(vehicle_info_t));
 	wipeString(engineConfiguration->vehicleName, sizeof(vehicle_info_t));
@@ -1128,6 +1127,43 @@ void setDefaultFrankensoConfiguration(DECLARE_CONFIG_PARAMETER_SIGNATURE) {
 	engineConfiguration->is_enabled_spi_3 = true;
 }
 
+#ifdef CONFIG_RESET_SWITCH_PORT
+// this pin is not configurable at runtime so that we have a reliable way to reset configuration
+#define SHOULD_IGNORE_FLASH() (palReadPad(CONFIG_RESET_SWITCH_PORT, CONFIG_RESET_SWITCH_PIN) == 0)
+#else
+#define SHOULD_IGNORE_FLASH() (false)
+#endif // CONFIG_RESET_SWITCH_PORT
+
+// by default, do not ignore config from flash! use it!
+#ifndef IGNORE_FLASH_CONFIGURATION
+#define IGNORE_FLASH_CONFIGURATION false
+#endif
+
+void loadConfiguration(Logging* logger DECLARE_ENGINE_PARAMETER_SUFFIX) {
+#ifdef CONFIG_RESET_SWITCH_PORT
+	// initialize the reset pin if necessary
+	palSetPadMode(CONFIG_RESET_SWITCH_PORT, CONFIG_RESET_SWITCH_PIN, PAL_MODE_INPUT_PULLUP);
+#endif /* CONFIG_RESET_SWITCH_PORT */
+
+#if EFI_INTERNAL_FLASH
+	if (SHOULD_IGNORE_FLASH() || IGNORE_FLASH_CONFIGURATION) {
+		engineConfiguration->engineType = DEFAULT_ENGINE_TYPE;
+		resetConfigurationExt(logger, engineConfiguration->engineType PASS_ENGINE_PARAMETER_SUFFIX);
+		writeToFlashNow();
+	} else {
+		// this call reads configuration from flash memory or sets default configuration
+		// if flash state does not look right.
+		readFromFlash();
+	}
+#else // not EFI_INTERNAL_FLASH
+	// This board doesn't load configuration, initialize the default
+	engineConfiguration->engineType = DEFAULT_ENGINE_TYPE;
+	resetConfigurationExt(logger, engineConfiguration->engineType PASS_ENGINE_PARAMETER_SUFFIX);
+#endif /* EFI_INTERNAL_FLASH */
+
+	rememberCurrentConfiguration(PASS_ENGINE_PARAMETER_SIGNATURE);
+}
+
 void resetConfigurationExt(Logging * logger, configuration_callback_t boardCallback, engine_type_e engineType DECLARE_ENGINE_PARAMETER_SUFFIX) {
 	enginePins.reset(); // that's mostly important for functional tests
 	/**

firmware/controllers/algo/engine_configuration.h

@@ -44,8 +44,6 @@ void setDefaultBasePins(DECLARE_CONFIG_PARAMETER_SIGNATURE);
 void setDefaultSdCardParameters(DECLARE_CONFIG_PARAMETER_SIGNATURE);
 
 void onBurnRequest(DECLARE_ENGINE_PARAMETER_SIGNATURE);
-void wipeStrings(DECLARE_ENGINE_PARAMETER_SIGNATURE);
-void rememberCurrentConfiguration(DECLARE_ENGINE_PARAMETER_SIGNATURE);
 void incrementGlobalConfigurationVersion(DECLARE_ENGINE_PARAMETER_SIGNATURE);
 
 void commonFrankensoAnalogInputs(engine_configuration_s *engineConfiguration);
@@ -60,6 +58,7 @@ typedef void (*configuration_callback_t)(engine_configuration_s*);
 
 #ifdef __cplusplus
 // because of 'Logging' class parameter these functions are visible only to C++ code but C code
+void loadConfiguration(Logging* logger DECLARE_ENGINE_PARAMETER_SUFFIX);
 void resetConfigurationExt(Logging * logger, configuration_callback_t boardCallback, engine_type_e engineType DECLARE_ENGINE_PARAMETER_SUFFIX);
 void resetConfigurationExt(Logging * logger, engine_type_e engineType DECLARE_ENGINE_PARAMETER_SUFFIX);
 #endif /* __cplusplus */

firmware/controllers/flash_main.cpp

@@ -106,7 +106,13 @@ static void doResetConfiguration(void) {
 	resetConfigurationExt(logger, engineConfiguration->engineType PASS_ENGINE_PARAMETER_SUFFIX);
 }
 
-persisted_configuration_state_e flashState;
+typedef enum {
+	PC_OK = 0,
+	CRC_FAILED = 1,
+	INCOMPATIBLE_VERSION = 2,
+	RESET_REQUESTED = 3,
+	PC_ERROR = 4
+} persisted_configuration_state_e;
 
 static persisted_configuration_state_e doReadConfiguration(flashaddr_t address, Logging * logger) {
 	printMsg(logger, "readFromFlash %x", address);
@@ -125,7 +131,7 @@ static persisted_configuration_state_e doReadConfiguration(flashaddr_t address,
  * this method could and should be executed before we have any
  * connectivity so no console output here
  */
-persisted_configuration_state_e readConfiguration(Logging * logger) {
+static persisted_configuration_state_e readConfiguration(Logging* logger) {
 	efiAssert(CUSTOM_ERR_ASSERT, getCurrentRemainingStack() > EXPECTED_REMAINING_STACK, "read f", PC_ERROR);
 	persisted_configuration_state_e result = doReadConfiguration(getFlashAddrFirstCopy(), logger);
 	if (result != PC_OK) {
@@ -152,15 +158,15 @@ persisted_configuration_state_e readConfiguration(Logging * logger) {
 	return result;
 }
 
-void readFromFlash(void) {
+void readFromFlash() {
 	persisted_configuration_state_e result = readConfiguration(logger);
 
 	if (result == CRC_FAILED) {
 		printMsg(logger, "Need to reset flash to default due to CRC");
 	} else if (result == INCOMPATIBLE_VERSION) {
-		printMsg(logger, "Resetting but saving engine type [%d]", engineConfiguration->engineType);
+		printMsg(logger, "Resetting due to version mismatch but preserving engine type [%d]", engineConfiguration->engineType);
 	} else {
-		printMsg(logger, "Got valid configuration from flash!");
+		printMsg(logger, "Read valid configuration from flash!");
 	}
 }
 

firmware/controllers/flash_main.h

@@ -10,16 +10,7 @@
 
 #include "engine.h"
 
-typedef enum {
-	PC_OK = 0,
-	CRC_FAILED = 1,
-	INCOMPATIBLE_VERSION = 2,
-	RESET_REQUESTED = 3,
-	PC_ERROR = 4
-} persisted_configuration_state_e;
-
-persisted_configuration_state_e readConfiguration(Logging * logger);
-void readFromFlash(void);
+void readFromFlash();
 void initFlash(Logging *sharedLogger);
 
 /**

firmware/hw_layer/hardware.cpp

@@ -506,36 +506,6 @@ void initHardwareNoConfig(Logging *l) {
 }
 
 void initHardware() {
-#if EFI_INTERNAL_FLASH
-#ifdef CONFIG_RESET_SWITCH_PORT
-// this pin is not configurable at runtime so that we have a reliable way to reset configuration
-#define SHOULD_IGNORE_FLASH() (palReadPad(CONFIG_RESET_SWITCH_PORT, CONFIG_RESET_SWITCH_PIN) == 0)
-#else
-#define SHOULD_IGNORE_FLASH() (false)
-#endif // CONFIG_RESET_SWITCH_PORT
-
-#ifdef CONFIG_RESET_SWITCH_PORT
-	palSetPadMode(CONFIG_RESET_SWITCH_PORT, CONFIG_RESET_SWITCH_PIN, PAL_MODE_INPUT_PULLUP);
-#endif /* CONFIG_RESET_SWITCH_PORT */
-
-	/**
-	 * this call reads configuration from flash memory or sets default configuration
-	 * if flash state does not look right.
-	 *
-	 * interesting fact that we have another read from flash before we get here
-	 */
-	if (SHOULD_IGNORE_FLASH()) {
-		engineConfiguration->engineType = DEFAULT_ENGINE_TYPE;
-		resetConfigurationExt(sharedLogger, engineConfiguration->engineType PASS_ENGINE_PARAMETER_SUFFIX);
-		writeToFlashNow();
-	} else {
-		readFromFlash();
-	}
-#else
-	engineConfiguration->engineType = DEFAULT_ENGINE_TYPE;
-	resetConfigurationExt(sharedLogger, engineConfiguration->engineType PASS_ENGINE_PARAMETER_SUFFIX);
-#endif /* EFI_INTERNAL_FLASH */
-
 #if EFI_HD44780_LCD
 	lcd_HD44780_init(sharedLogger);
 	if (hasFirmwareError())

firmware/rusefi.cpp

@@ -120,7 +120,6 @@
 #include "eficonsole.h"
 #include "status_loop.h"
 #include "pin_repository.h"
-#include "flash_main.h"
 #include "custom_engine.h"
 #include "engine_math.h"
 #include "mpu_util.h"
@@ -194,22 +193,7 @@ void runRusEfi(void) {
 
 	initHardwareNoConfig(&sharedLogger);
 
-#if EFI_INTERNAL_FLASH
- #if IGNORE_FLASH_CONFIGURATION
-	resetConfigurationExt(&sharedLogger, DEFAULT_ENGINE_TYPE PASS_ENGINE_PARAMETER_SUFFIX);
- #else
-	/**
-	 * First thing is reading configuration from flash memory.
-	 * In order to have complete flexibility configuration has to go before anything else.
-	 */
-	readConfiguration(&sharedLogger);
- #endif // IGNORE_FLASH_CONFIGURATION
-#endif /* EFI_INTERNAL_FLASH */
-
-#if ! EFI_ACTIVE_CONFIGURATION_IN_FLASH
-	// TODO: need to fix this place!!! should be a version of PASS_ENGINE_PARAMETER_SIGNATURE somehow
-	prepareVoidConfiguration(&activeConfiguration);
-#endif /* EFI_ACTIVE_CONFIGURATION_IN_FLASH */
+	loadConfiguration(&sharedLogger PASS_ENGINE_PARAMETER_SUFFIX);
 
 #if EFI_FILE_LOGGING
 	initMmcCard();
@@ -247,7 +231,6 @@ void runRusEfi(void) {
 		 * todo: should we initialize some? most? controllers before hardware?
 		 */
 		initEngineContoller(&sharedLogger PASS_ENGINE_PARAMETER_SIGNATURE);
-		rememberCurrentConfiguration();
 
 	#if EFI_PERF_METRICS
 		initTimePerfActions(&sharedLogger);