Skip to content

CurieEEPROM refactor #281

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

Closed
wants to merge 6 commits into from
Closed

Conversation

facchinm
Copy link
Member

@facchinm facchinm commented Sep 5, 2016

This PR refactors CurieEEPROM library, syncing it with AVR version.
Since EEPROM on Curie chip is based on OTP flash storage, the minimum write size is 4 bytes. The current implementation brings some problems like http://forum.arduino.cc/index.php?topic=421348.0 , and the examples are quite broken.

The refactor also changes the name, from CurieEEPROM to EEPROM, so code for AVR sketches compiles out of the box. The new library resolution engine (from IDE 1.6.8 onward) handles this perfectly but we can stick to older name if we need compatibility with older IDEs.

the new version will be based on AVR EEPROM lib
Since FLASH_STTS.ER_DONE and FLASH_STTS.WRITE_DONE are not being set, the delays are hardcoded.
@zeptochain
Copy link

This fix passes my local tests :) I had been struggling for many hours with CurieEEPROM 1.0.6

uint32_t data32 = (currentDword & ~(uint32_t)(0xFF << ((3-offset)*8)));
data32 = data32 | (data << ((3-offset)*8));

if (currentDword != 0xFFFFFFFF) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe instead of checking the value of CurieRead32 to be 0xFFFFFFFF
check the value of CurieRead8 to be 0xFF instead?
That way we are not clearing and restoring the entire block 4x as much as before.

You can take a look at the implementation of CurieEEPROM::write8
I basically read the dword and insert the byte inside the dword depending on where it needs to be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I believe I misinterpreted the datasheet, so I thought that the whole dword needed to be erased before rewriting. Testing with the "reduced" version works too in my test sketches. I'm pushing the modification right now.
@zeptochain , could you give a try to the updated patch with your test sketches? Thanks

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@facchinm will try this over the weekend and report back

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just looked at the commit - not really confident, but will try it - i have a feeling that Martino got it right first time - i'll first write further test sketches to prove out 281 before trying 291 for comparison...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK take it all back -- this update passes my test sketches including a complex config job I needed this library to accomplish. Has my thumbs up @facchinm

Previously, if any of the bytes composing a dword was not 0xFF, a clear was executed.
Copy link
Contributor

@bigdinotech bigdinotech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@calvinatintel calvinatintel force-pushed the master branch 2 times, most recently from 5340b0c to 83c76a6 Compare September 19, 2016 22:23
@calvinatintel
Copy link
Contributor

Cherry-picked into the 1.0.7 branch. Will be closed once 1.0.7 is final.

@calvinatintel
Copy link
Contributor

cherry-picked

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

Successfully merging this pull request may close these issues.

5 participants