X-CUBE-EEPROM 7.0.0 bug

[pretoriandave]

While implementing EEPROM emulation on an STM32G0 (although I believe this would apply to all supported families) I have discovered what I believe to be a bug in eeprom_emul.c:

Although ConfigureCrc() is called at the beginning of EE_Init to turn on the CRC block clock, it is not similarly called at the beginning of EE_Format and I believe it should be.

The effect of this is that if you call EE_Format first to configure the EEPROM emulation pages and then write some values in, the CRCs are all written to zero. On subsequent power/reset cycles, when usign EE_Init, ConfigureCRC is called, allowing the CRCs to be caculated correct from there on, although subsequent reads using EE_ReadVariablexxbits fail due to bad CRC, since the stored CRCs are all zero.

Adding ConfigureCrc(); at the beginning of EE_Format fixes the issue.

EE_Status EE_Format(EE_Erase_type EraseType)
{
 uint32_t page = 0U;

 ConfigureCrc();	//***BUG FIX***										

 /* Check if the configuration is 128-bits bank or 2*64-bits bank */
 if (FI_CheckBankConfig() != EE_OK)
 {
 return EE_INVALID_BANK_CFG;
 }
etc

 

[mƎALLEm]

Hello @pretoriandave ,

Internal ticket 193188 for follow-up. I'll get back to you as soon as I have a feedback.

[pretoriandave]

@mƎALLEm : A few other things for the team to consider while they are about it, if I may:

1. In the example application supplied with this middleware, there are a couple of iffy lines of code:

if ((ee_status & EE_STATUSMASK_CLEANUP) == EE_STATUSMASK_CLEANUP) {ErasingOnGoing = 0;ee_status|= EE_CleanUp();}

should surely be:

if ((ee_status & EE_STATUSMASK_CLEANUP) == EE_CLEANUP_REQUIRED) {ErasingOnGoing = 0;ee_status|= EE_CleanUp();}

It compiles the same either way, but in the spirit of improving readability, it would be better if it were changed. There are two instances of this.

2. The very next line of code is this:

if ((ee_status & EE_STATUSMASK_ERROR) == EE_STATUSMASK_ERROR) {Error_Handler();}

Not certain on this one, but think perhaps what they meant was:

if ((ee_status & EE_STATUSMASK_ERROR) != 0) {Error_Handler();}

Again, two instances.

3. Lastly, I came at this from having previously used the EEPROM emulator set out in AN3969 for older processors. This has as its example virtual addresses 0x5555, 0x6666 etc. I continued to use these for no particular reason and have spent all of today stracthing my head over why the latest stored vales are not copied across to the other page once the first is full. I now realise that the virtual addresses MUST be 1 to NM_OF_VARIABLES and therefore contiguous) for the PagesTransfer function to work. Now what's stated in AN4894, foot of page 7 about the addresses is not wrong, but it could perhaps stress this requirement more, as I managed to overlook it and others might too.

Hope these are helpful.

[mƎALLEm]

Hello @pretoriandave ,

This is a feedback from the package developer:

Regarding the missing CRC Config in the EE_Format function, it's not really a limitation, as described in the AN, the EE_Init is supposed to be called before any access to the Emulated EEPROM, since it's mandatory after each reset for robustness purposes in case the resets occur during a page transfer for example, so that's why the CRC Config function is not called within the format function.

But the code can be easily tailored by the customer as he seems to have done if that fit his specific use case.

Regarding the next 2 points about clarity in code we will check if an update is needed, but it doesn't imply any "bug" so that's a good news.

Regarding the last point, the virtual addresses description, it is something that has already been taken into account for next release.

Hope it does answer your question.

[pretoriandave]

Many thanks @mƎALLEm , great service as ever. I think there may be an important distinction in here that we should bottom out, as I think it might warrant a tweak to AN4898 and/or the headers in eeprom_emul.c as they seem to be at odds...

I think what I'm gathering from the team's response is that, despite the function name, EE_Format is not how one should first format the EEPROM emulation space. In any case, without my tweak, you can't, since the CRC block is not clocked.

The response above is that calling EE_Init is mandatory after each reset and I presume that includes the very first time the MCU is powered after programming too. In other words, one should use the following to first format the EEPROM space..

EE_Init(EE_FORCED_ERASE)

By contrast, the header of eeprom_emul.c says:

(#) EEPROM emulation initialization functions:
 (++) Format the FLASH pages used by EEPROM emulation using EE_Format().
 This function is optionally used, it can be called the very first
 time EEPROM emulation is used, to prepare FLASH pages for EEPROM
 emulation with empty EEPROM variables. It can also be called at
 any time, to flush all EEPROM variables.
 (++) Initialize EEPROM emulation, and restore the FLASH pages used by
 EEPROM emulation to a known good state in case of power loss
 using EE_Init(). It must be performed at system start up.

The header suggests that EE_Format can be used to format the EEPROM space the first time the MCU is powered and the function name certainly supports that view. That is how I first did it and it didn't work for the reasons already stated. 'It can be called the very first time EEPROM emulation is used'. No it can't.

My use-case is that I am storing calibration parameters in the EEPROM emulation space which are determined the very first time the MCU is powered up, at end of line testing. The MCU is powered up, EE_Format called, the measurements made (actually Time of Flight device calibration), and the results stored.

The headers say that's fine but in fact it can't work. So, please, which should it be, EE_Init or EE_Format? It's really not clear and it's certainly not clear that in fact EE_Format cannot be used. If the answer is EE_Init then what use is EE_Format anyway? It seems that EE_Init(EE_FORCED_ERASE) is always the preferred choice.

Sorry to labour the point but the documentation isn't clear and I'd like to contibute to it being improved to save other developers any wasted time. Many thanks

[pretoriandave]

@Andrew Neil Yes that's correct. AN4061 is the old method and could use arbitrary addresses. The new method requires them to be consecutive beginning at 1.