better flash logic and warning message (#3975)

* better flash logic and warning message

* comment

* s

* ssssss

* simplify logic

* s

firmware/controllers/flash_main.cpp

@@ -71,8 +71,8 @@ const MFSConfig mfsd_nor_config = {
  * should be in a different sector of flash since complete flash sectors are erased on write.
  */
 
-crc_t flashStateCrc(persistent_config_container_s *state) {
+static crc_t flashStateCrc(const persistent_config_container_s& state) {
-	return calc_crc((const crc_t*) &state->persistentConfiguration, sizeof(persistent_config_s));
+	return calc_crc((const crc_t*) &state.persistentConfiguration, sizeof(persistent_config_s));
 }
 
 #if EFI_FLASH_WRITE_THREAD
@@ -164,7 +164,7 @@ void writeToFlashNow(void) {
 	// Set up the container
 	persistentState.size = sizeof(persistentState);
 	persistentState.version = FLASH_DATA_VERSION;
-	persistentState.value = flashStateCrc(&persistentState);
+	persistentState.value = flashStateCrc(persistentState);
 
 #if EFI_STORAGE_EXT_SNOR == TRUE
 	mfs_error_t err;
@@ -200,43 +200,44 @@ void writeToFlashNow(void) {
 	needToWriteConfiguration = false;
 }
 
-static bool isValidCrc(persistent_config_container_s *state) {
-	crc_t result = flashStateCrc(state);
-	int isValidCrc_b = result == state->value;
-	return isValidCrc_b;
-}
-
 static void doResetConfiguration() {
 	resetConfigurationExt(engineConfiguration->engineType);
 }
 
-typedef enum {
+enum class FlashState {
-	PC_OK = 0,
+	Ok,
-	CRC_FAILED = 1,
+	CrcFailed,
-	INCOMPATIBLE_VERSION = 2,
+	IncompatibleVersion,
-	RESET_REQUESTED = 3,
+	// all is well, but we're on a fresh chip with blank memory
-	PC_ERROR = 4
+	BlankChip,
-} persisted_configuration_state_e;
+};
 
 /**
  * Read single copy of rusEFI configuration from flash
  */
-static persisted_configuration_state_e readOneConfigurationCopy(flashaddr_t address) {
+static FlashState readOneConfigurationCopy(flashaddr_t address) {
 	efiPrintf("readFromFlash %x", address);
 
 	// error already reported, return
 	if (!address) {
-		return CRC_FAILED;
+		return FlashState::BlankChip;
 	}
 
 	intFlashRead(address, (char *) &persistentState, sizeof(persistentState));
 
-	if (!isValidCrc(&persistentState)) {
+	auto flashCrc = flashStateCrc(persistentState);
-		return CRC_FAILED;
+
+	if (flashCrc != persistentState.value) {
+		// If the stored crc is all 1s, that probably means the flash is actually blank, not that the crc failed.
+		if (persistentState.value == ((crc_t)-1)) {
+			return FlashState::BlankChip;
+		} else {
+			return FlashState::CrcFailed;
+		}
 	} else if (persistentState.version != FLASH_DATA_VERSION || persistentState.size != sizeof(persistentState)) {
-		return INCOMPATIBLE_VERSION;
+		return FlashState::IncompatibleVersion;
 	} else {
-		return PC_OK;
+		return FlashState::Ok;
 	}
 }
 
@@ -246,39 +247,41 @@ static persisted_configuration_state_e readOneConfigurationCopy(flashaddr_t addr
  *
  * in this method we read first copy of configuration in flash. if that first copy has CRC or other issues we read second copy.
  */
-static persisted_configuration_state_e readConfiguration() {
+static FlashState readConfiguration() {
-	persisted_configuration_state_e result = CRC_FAILED;
-
-	efiAssert(CUSTOM_ERR_ASSERT, getCurrentRemainingStack() > EXPECTED_REMAINING_STACK, "read f", PC_ERROR);
-
 #if EFI_STORAGE_EXT_SNOR == TRUE
-	mfs_error_t err;
 	size_t settings_size = sizeof(persistentState);
-	err = mfsReadRecord(&mfsd, EFI_MFS_SETTINGS_RECORD_ID,
+	mfs_error_t err = mfsReadRecord(&mfsd, EFI_MFS_SETTINGS_RECORD_ID,
 						&settings_size, (uint8_t *)&persistentState);
 
-	if ((err == MFS_NO_ERROR) && (sizeof(persistentState) == settings_size))
+	// TODO: check err result better?
-		result = PC_OK;
+	if (err == MFS_NO_ERROR) {
+		return FlashState::Ok;
+	} else {
+		// TODO: is this correct?
+		return FlashState::BlankChip;
+	}
 #endif
 
 #if EFI_STORAGE_INT_FLASH == TRUE
 	auto firstCopyAddr = getFlashAddrFirstCopy();
 	auto secondyCopyAddr = getFlashAddrSecondCopy();
 
-	result = readOneConfigurationCopy(firstCopyAddr);
+	FlashState firstCopy = readOneConfigurationCopy(firstCopyAddr);
 
-	if (result != PC_OK) {
+	if (firstCopy == FlashState::Ok) {
-		efiPrintf("Reading second configuration copy");
+		// First copy looks OK, don't even need to check second copy.
-		result = readOneConfigurationCopy(secondyCopyAddr);
+		return firstCopy;
 	}
+
+	efiPrintf("Reading second configuration copy");
+	return readOneConfigurationCopy(secondyCopyAddr);
 #endif
 
-	return result;
+	// In case of neither of those cases, return that things went OK?
+	return FlashState::Ok;
 }
 
 void readFromFlash() {
-	persisted_configuration_state_e result = PC_OK;
-
 #if HW_CHECK_MODE
 	/*
 	 * getFlashAddr does device validation, we want validation to be invoked even while we are
@@ -289,35 +292,36 @@ void readFromFlash() {
 	getFlashAddrSecondCopy();
 
 	resetConfigurationExt(DEFAULT_ENGINE_TYPE);
+
+	FlashState result = FlashState::Ok;
 #else
-	result = readConfiguration();
+	FlashState result = readConfiguration();
 #endif
 
-	if (result == CRC_FAILED) {
+	switch (result) {
-	    // we are here on first boot on brand new chip
+		case FlashState::CrcFailed:
-		warning(CUSTOM_ERR_FLASH_CRC_FAILED, "flash CRC failed");
+			warning(CUSTOM_ERR_FLASH_CRC_FAILED, "flash CRC failed");
-		resetConfigurationExt(DEFAULT_ENGINE_TYPE);
+			efiPrintf("Need to reset flash to default due to CRC mismatch");
-	} else if (result == INCOMPATIBLE_VERSION) {
+			// falls through
-		resetConfigurationExt(engineConfiguration->engineType);
+		case FlashState::BlankChip:
-	} else {
+			resetConfigurationExt(DEFAULT_ENGINE_TYPE);
-		/**
+			break;
-		 * At this point we know that CRC and version number is what we expect. Safe to assume it's a valid configuration.
+		case FlashState::IncompatibleVersion:
-		 */
+			// Preserve engine type from old config
-		applyNonPersistentConfiguration();
+			efiPrintf("Resetting due to version mismatch but preserving engine type [%d]", engineConfiguration->engineType);
+			resetConfigurationExt(engineConfiguration->engineType);
+			break;
+		case FlashState::Ok:
+			// At this point we know that CRC and version number is what we expect. Safe to assume it's a valid configuration.
+			applyNonPersistentConfiguration();
+			efiPrintf("Read valid configuration from flash!");
+			break;
 	}
 
 	// we can only change the state after the CRC check
 	engineConfiguration->byFirmwareVersion = getRusEfiVersion();
 	memset(persistentState.persistentConfiguration.warning_message , 0, ERROR_BUFFER_SIZE);
 	validateConfiguration();
-
-	if (result == CRC_FAILED) {
-		efiPrintf("Need to reset flash to default due to CRC");
-	} else if (result == INCOMPATIBLE_VERSION) {
-		efiPrintf("Resetting due to version mismatch but preserving engine type [%d]", engineConfiguration->engineType);
-	} else {
-		efiPrintf("Read valid configuration from flash!");
-	}
 }
 
 static void rewriteConfig() {