Possible bug when writing to flash using the EEPROM library (F7)

[hasenbanck]

Version used: Current HEAD + PR
Affected variant: RemRam v1

Currently I try to write some configuration values with the help of the EEPROM library to the flash.

Given this simple sketch:

#include <EEPROM.h>

void setup() {
	Serial.begin(115200);
	uint8_t v = EEPROM.read(0);
	if (v == 123) {
		Serial.println("Saved");
	} else {
		Serial.println("Wasn't saved");
		EEPROM.write(0, 123);
	}
}

void loop() {
	delay(15);
}

The flash won't safe the variable content. I tracked down the problem to the set_data_to_flash() function in stm32_eeprom.c.

HAL_FLASHEx_Erase(&EraseInitStruct, &SectorError) will return an HAL_ERROR and when debugging the error with HAL_FLASH_GetError(), I will receive a FLASH_ERROR_WRP error (FLASH Write protected error). SectorError contains a 0x17 (23).

Is there anything special I need to do when wanting to write to the flash when using a F7?

[fpistm]

Hi @hasenbanck
You could use the STM32CubeProgrammer to check Write flash protection.

[hasenbanck]

I will check the state of the write flash protection this evening when I'm back home. I can't remember having set it though. Could it be set by the factory? Maybe a side effect of using a J-Link programmer? Or maybe it just flipped by chance.

[hasenbanck]

I just reset the option bytes and checked the write flash protection. They are not write protected.

One thing is curious: The SectorError points to sector 23, which is the last sector in double bank mode. But since the default (and my) configuration is single bank mode, the last sector should be 11.

Could this be a lead?

Edit: For fun I just edited the FLASH_SECTOR_TOTAL definition from 24 to 12 and changed some broken stuff, which enabled me to save top flash just fine. Either I was simply lucky, or we really select the wrong flash sector number.

[fpistm]

Probably there is some issue(s) in the generic definition for F7.
Here:
https://github.com/stm32duino/Arduino_Core_STM32/blob/master/cores/arduino/stm32/stm32_eeprom.h#L54
or here:
https://github.com/stm32duino/Arduino_Core_STM32/blob/master/cores/arduino/stm32/stm32_eeprom.c#L81

FLASH_SECTOR_TOTAL is defined by our CMSIS. Hope It was not wrong :)
I think those part need some deep review.

[hasenbanck]

I don't think it's a problem in the generic definition for the F7, since the page size and also the memory addresses seem right.

The CMSIS is not wrong about the total sector count, but only when using the double bank mode. Maybe we shouldn't use the FLASH_SECTOR_TOTAL definition in this case, since 24 is definitely wrong in the standard configuration.

[hasenbanck]

Okay, let me structure my thoughts a little bit. As it seems to me right now, the HAL doesn't know how to handle SINGLE / DUAL BANK MODE for the flash. As far as I can see inside the stm32f4xx_hal_flash_ex.h and the CMSIS files of the F76xxx and F77xxx, they used the total value of the flash sectors in dual bank mode.

Information about that mode can be found here.

The stm32f4xx_hal_flash_ex.h and the stm32f7xx_hal_flash_ex.h then use those values to define the available sector, that later the HAL will use in his FLASH module. There we currently haven't defined the sectors for the default 12 sectors in single bank mode.

In my case, the HAL will later try to erase the sector 23 of the flash (last), which of course wouldn't work, since offset and size are different from the sector 11 in single bank mode.

So as a work around, people could activate the two bank mode for the flash on the affected systems, but since you lose easy accessible uniform flash memory, that might not be a good option in the long term.

A better fix would be, to let the HAL become aware of the single / double memory bank configuration of those systems and let the user configure, which mode he wants the HAL to use (with single bank mode as the default, since it's the factory default).

So right now we would need:

• A configuration switch in the stm32f4xx_hal_conf.h and stm32f7xx_hal_conf.h to select either single or dual bank mode.
• Defining the FLASH_SECTOR_TOTAL based on this configuration switch (in the CMSIS files for the STM32F7 and the stm32f4xx_hal_flash_ex.h for the STM32f4)
• Define the sectors in case of the FLASH_SECTOR_TOTAL == 12 in the stm32f4xx_hal_flash_ex.h and stm32f7xx_hal_flash_ex.h

[fpistm]

Ok looking deeply this and found that it should be possible to know if we are in Single or Dual Bank.
At least for F7:

#if defined (FLASH_OPTCR_nDBANK)
/** @defgroup FLASHEx_Option_Bytes_nDBank FLASH Single Bank or Dual Bank
  * @{
  */
#define OB_NDBANK_SINGLE_BANK      ((uint32_t)0x20000000U) /*!< NDBANK bit is set : Single Bank mode */
#define OB_NDBANK_DUAL_BANK        ((uint32_t)0x00000000U) /*!< NDBANK bit is reset : Dual Bank mode */
/**
  * @}
  */
#endif /* FLASH_OPTCR_nDBANK */

So F7 HAL provide (if FLASH_OPTCR_nDBANK defined):

/**
  * @brief  Return the FLASH User Option Byte value.
  * @retval uint32_t FLASH User Option Bytes values: WWDG_SW(Bit4), IWDG_SW(Bit5), nRST_STOP(Bit6), 
  *         nRST_STDBY(Bit7), IWDG_STDBY(Bit30) and IWDG_STOP(Bit31).
  */
static uint32_t FLASH_OB_GetUser(void)
{
  /* Return the User Option Byte */
  return ((uint32_t)(FLASH->OPTCR & 0xF00000F0U));
}

Then it would be possible to know the correct config, something like that.
if Single:
FLASH_DATA_SECTOR --> ((uint32_t)(FLASH_SECTOR_TOTAL/2 - 1))
else /dual/
FLASH_DATA_SECTOR --> ((uint32_t)(FLASH_SECTOR_TOTAL - 1))

For F4 I didn't find any stuff like for F7. I have to check how to handle or if this is planned to add thoses API.

[hasenbanck]

Yeah of course, you could read out the option byte and then set the data sector you want to use based on that information. Didn't thought about doing it that way. That would work and wouldn't require a rework on the HAL/CMSIS level.