Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Damel91
Copy link

@Damel91 Damel91 commented Mar 27, 2019

details described in issue named EEPROM multiple data at the same address

@stevstrong
Copy link
Collaborator

Can you please tell me a reason why is this change needed? What kind of issue is solved with this?
Any test sketch which can demonstrate the issue and the solution you provided?

@Damel91
Copy link
Author

Damel91 commented Sep 19, 2019

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.

Comment on lines +260 to +263
FlashStatus = FLASH_ProgramHalfWord(idx + 2, Address); // Set variable virtual address
if (FlashStatus != FLASH_COMPLETE)
return FlashStatus;
return EEPROM_OK;
Copy link
Contributor

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.

Comment on lines -538 to -545
uint16 EEPROMClass::update(uint16 Address, uint16 Data)
{
if (read(Address) == Data)
return EEPROM_SAME_VALUE;
else
return write(Address, Data);
}

Copy link
Contributor

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;

}
Copy link
Contributor

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)
Copy link
Contributor

@victorpv victorpv Jan 2, 2020

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)

@victorpv
Copy link
Contributor

victorpv commented Jan 2, 2020

I think this PR is related to this issue:
#606

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.
I haven't checked the whole rest of the changes, since that part already wouldn't work as intended.

This needs to gets rewritten as per the programming manual requirements:
https://www.st.com/content/ccc/resource/technical/document/programming_manual/10/98/e8/d4/2b/51/4b/f5/CD00283419.pdf/files/CD00283419.pdf/jcr:content/translations/en.CD00283419.pdf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants