EEPROM programming delays are not always correct.

[Amomum]

EEPROM writing and erasing functions relay on this code for delays: ProgramDelay(GET_US_LOOPS(5));

GET_US_LOOPS implementation is as follows:

define GET_US_LOOPS(N) ((uint32_t)((float)(N) * FLASH_PROG_FREQ_MHZ / DELAY_LOOP_CYCLES))

This code triggers a warning on implicit double promotion because FLASH_PROG_FREQ_MHZ is by default defined like this (and therefore is double): #define FLASH_PROG_FREQ_MHZ (8.0)

But the main problem here is that it is a constant when SYS_CLK can actually change during runtime (by enabling/disabling PLL, for example). So all EEPROM delays will go out of the window.

I suppose FLASH_PROG_FREQ_MHZ should be made dependent on SystemCoreClock variable. And there is no point in making it double or float, since it is used only after convertion to uint32_t);

__RAMFUNC static void ProgramDelay(uint32_t Loops)
{
  volatile uint32_t i = Loops;
  for (; i > 0; i--)
  {
  }
}

I guess it should be something like this (I tried to do a ceiling instead of simple division but I'm 100% sure I did it wrong): GET_US_LOOPS(N) (N*((SystemCoreClock + 1000000 - 1)/1000000) + DELAY_LOOP_CYCLES - 1)/DELAY_LOOP_CYCLES

[eldarkg]

@Amomum You should set the FLASH_PROG_FREQ_MHZ to maximum work frequency value here. I don't want change the current behaviour (the current library should be compatible). Maybe the current behaviour is more delayed but it is more safe (you should know that you should control and update of current SystemCoreClock value).

[Amomum]

If you insist on remaining compatibility, then maybe we should consider the following:

• Current implementation won't work for frequensies less then 1 MHz because the result of the computation will be assigned to uint32_t i - and it will become equal to zero. I'm not sure if the resulting delay will be enough. And I'm not sure if anybody will ever use frequency below 1 MHz.

• I find this define rather obscure (I literally had no idea of it's existance for a couple of years - I suppose it doesn't make me look good but still) so I personally would prefer if it wouldn't be defined by default in config.h. And than we can have something like

  #ifndef FLASH_PROG_FREQ_MHZ 
  #  error "The user should set this in config.h"

• At lhe very least config.h should contain a comment about this FLASH_PROG_FREQ_MHZ. Now it's the only define without any comments at all; that makes it even more obscure. I spotted it only because default value produced a warning with -Wdouble-promotion.

[eldarkg]

@Amomum

Current implementation won't work for frequensies less then 1 MHz because the result of the computation will be assigned to uint32_t i - and it will become equal to zero. I'm not sure if the resulting delay will be enough. And I'm not sure if anybody will ever use frequency below 1 MHz.

If a MCU frequency is equal or less 1 MHz then 1 period of clock is equal or greater 1 us. Therefore the ProgramDelay(1) call should give delay >= 1 us. But ProgramDelay(5) call give wrong (less) delay. We can to do it as (use double and add 1):

#define GET_US_LOOPS(N) ((uint32_t)((double)(N) * FLASH_PROG_FREQ_MHZ / DELAY_LOOP_CYCLES + 1.0))

I find this define rather obscure (I literally had no idea of it's existance for a couple of years - I suppose it doesn't make me look good but still) so I personally would prefer if it wouldn't be defined by default in config.h. And than we can have something like

I had the same problem. I will try to resolve this problem with use your suggestions.

[eldarkg]

@Amomum Can you test this commit?