-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
write and update have been modified #615
base: master
Are you sure you want to change the base?
Conversation
Can you please tell me a reason why is this change needed? What kind of issue is solved with this? |
When I used the eeprom library i found that it didnt overwrite the data to a specific address but created a new one, so the eeprom will give fullfilled error after a bunch of time. My little modification let the data shift from one page to another, so you can overwrite the address that you written before. |
FlashStatus = FLASH_ProgramHalfWord(idx + 2, Address); // Set variable virtual address | ||
if (FlashStatus != FLASH_COMPLETE) | ||
return FlashStatus; | ||
return EEPROM_OK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the indentation of these lines is not correct, please check and correct.
uint16 EEPROMClass::update(uint16 Address, uint16 Data) | ||
{ | ||
if (read(Address) == Data) | ||
return EEPROM_SAME_VALUE; | ||
else | ||
return write(Address, Data); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this function removed?
// Empty slot not found, need page transfer | ||
// Calculate unique variables in page | ||
count = EE_GetVariablesCount(pageBase, Address) + 1; | ||
if (count >= (PageSize / 4 - 1)) | ||
return EEPROM_OUT_SIZE; | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this added here? the "if" statement doesn't have the opening bracket
if (count == Data) | ||
return EEPROM_OK; | ||
if (count == 0xFFFF) | ||
|
||
if (count == 0xFFFF || Data < count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, you can't reprogram 1s in a halfword of flash, only 0s, so comparing Data < count to decide whether the word can be rewritten is invalid and will lead to corruption.
Either leave is as before (comparing to 0xFFFF) or compare to 0 to make sure the new data has all bits 0.
As per the flash programming manual:
Standard programming
In this mode the CPU programs the main Flash memory by performing standard half-word
write operations. The PG bit in the FLASH_CR register must be set. FPEC preliminarily
reads the value at the addressed main Flash memory location and checks that it has been
erased. If not, the program operation is skipped and a warning is issued by the PGERR bit in
FLASH_SR register (the only exception to this is when 0x0000 is programmed. In this case,
the location is correctly programmed to 0x0000 and the PGERR bit is not set).
So the only valid cases to write to a flash halfword are: The halfword is in erased state (0xFFFF) OR the new data is all 0 (0x0000)
if (count == 0xFFFF || Data = 0)
I think this PR is related to this issue: I added some comments in line but upon a quick review of the code, it would fail to work properly since it tries to rewrite data on halfwords already written. That is invalid as per the programming manual. I added some comments in line. This needs to gets rewritten as per the programming manual requirements: |
details described in issue named EEPROM multiple data at the same address