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

SD EEPROM: Create eeprom.dat if not found on SD #14824

Closed

Conversation

pinchies
Copy link
Contributor

@pinchies pinchies commented Aug 3, 2019

fix for SD as eeprom - create file if no eeprom.dat file found.

@pinchies pinchies changed the title Fix sd no file eeprom SD EEPROM: Create eeprom.dat if not found on SD Aug 3, 2019
@pinchies pinchies marked this pull request as ready for review August 3, 2019 17:52
@pinchies
Copy link
Contributor Author

pinchies commented Aug 3, 2019

I didn't have any problems with the root directory thing we discussed, but I found there was no eeprom.dat created automatically if it wasnt present

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

seems weird to me, we never had to create it manually...

@pinchies
Copy link
Contributor Author

pinchies commented Aug 4, 2019

I didn't either, until the recent builds. I can't figure out why, but 3am me probably was missing something in one of the revisions. It was repeatable for me however, I do think this PR is required.

@pinchies
Copy link
Contributor Author

pinchies commented Aug 4, 2019

I now guess I wonder if adding O_CREAT permission to the initial open would make it return a success.......

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

I dont see why that could help to store junk and return true

@pinchies
Copy link
Contributor Author

pinchies commented Aug 4, 2019

Because if no file is found, the PersistentStore::access_start() aborts the save eeprom process - the same as if no card was detected, so the PersistentStore::access_finish() is never reached, and so the correct file is never created.

But I wonder if adding O_CREAT permission to the initial file.open would achieve the same result.

Regardless, I'm just sharing my findings after people informed me that they had problems, and I diagnosed the cause. Not sure what the best way to patch this bug is.

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

format the sdcard imo, seems an hardware to me

@pinchies
Copy link
Contributor Author

pinchies commented Aug 4, 2019

Tried multiple sd cards, formatted, with files or completely blank, multiple printers, checked write switch etc etc. Repeatable.

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

so you are saying the eeprom data is reloaded before save ? cant be...

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

hmm maybe it is... need to read the shared ocde

@pinchies
Copy link
Contributor Author

pinchies commented Aug 4, 2019

I'm not claiming to be able to write C++ code well any better than I can claim to speak french, but I can at least read some of it :-P

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

i dont see any abort on access_start failure.. just a serial message warning

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

oh i see it now:
#define EEPROM_START() if (!persistentStore.access_start()) { SERIAL_ECHO_MSG("No EEPROM."); return false; } \

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

commit fdbc733

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

but its not the right solution.. this commit... you dont have to create the file to fix that... just need to return true

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

rmm im currently testing the flash eeprom PR, and maybe my issue is the same one... :p
edit: no

bool PersistentStore::access_start() {
  firstWrite = true;
  return true;
}

@pinchies
Copy link
Contributor Author

pinchies commented Aug 4, 2019

Ahhhh, that was the breaking commit! That makes sense now.

@pinchies
Copy link
Contributor Author

pinchies commented Aug 4, 2019

tagging #14776

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

yep, sorry i made tests recently but was before this change... and i was confused about your issue

@pinchies
Copy link
Contributor Author

pinchies commented Aug 4, 2019

I appreciate that you're looking into flash eeprom, would be even nicer to have that working too.

@tpruvot
Copy link
Contributor

tpruvot commented Aug 4, 2019

not won for now, the PR didnt work on any of my 2 different boards.. (skrmini/longer3D)
edit: in fact it worked... on the skrmini hmm, maybe some hope

else the sd fix to appy: tpruvot@b97409d

@trouch
Copy link
Contributor

trouch commented Aug 4, 2019

⚠️ test_index isn't initialized to EEPROM_OFFSET
But we won't need it anymore once my PR #14829 get merged.

@pinchies
Copy link
Contributor Author

pinchies commented Aug 5, 2019

@trouch not sure I understand your comment in relation to this PR

I think that simply returning true is the wrong solution as if there is another failure to open the file we do want it to return false.

@tpruvot
Copy link
Contributor

tpruvot commented Aug 5, 2019

see the other implementations, they return most true by default... doing nothing on start.
if (!card.isDetected()) return false; remains for the common case..

and the write may return false in case of write problem anyway

@pinchies
Copy link
Contributor Author

pinchies commented Aug 5, 2019

Fair enough - I guess there’s no need for it to be in an if statement at all then if we don’t care about the returned value being false?

tpruvot added a commit to tpruvot/Marlin that referenced this pull request Aug 5, 2019
Variant of the PR MarlinFirmware#14824, required since the PR MarlinFirmware#14776 (31 july)

the typo is only seen with DEBUG_EEPROM_READWRITE
@yangwenxiong
Copy link
Contributor

The version I downloaded in July. Don't create eeprom. Dat file.

@pinchies
Copy link
Contributor Author

pinchies commented Aug 6, 2019

@yangwenxiong - if you do the top change at this link it should work:
tpruvot@b97409d

@yangwenxiong
Copy link
Contributor

oh i see it now:
#define EEPROM_START() if (!persistentStore.access_start()) { SERIAL_ECHO_MSG("No EEPROM."); return false; } \

For the MCU of STM32F103xx series, it is necessary to build a file EEPROM.DAT in SD card in advance to simulate EEPROM data storage in SD card.Otherwise it's not gonna work and you're going to have a problem with it,

@yangwenxiong
Copy link
Contributor

oh i see it now:
#define EEPROM_START() if (!persistentStore.access_start()) { SERIAL_ECHO_MSG("No EEPROM."); return false; } \

For the MCU of STM32F103xx series, it is necessary to build a file EEPROM.DAT in SD card in advance to simulate EEPROM data storage in SD card.Otherwise it's not gonna work and you're going to have a problem with it,

#define EEPROM_START() int eeprom_index = EEPROM_OFFSET; persistentStore.access_start()

@pinchies
Copy link
Contributor Author

pinchies commented Aug 7, 2019

You don’t need to do that. You do need to change the file containing the access_start()

@tpruvot
Copy link
Contributor

tpruvot commented Aug 9, 2019

This one can be closed now.. (you can delete the branch pinchies:fix-sd-no-file-eeprom @pinchies to do it)

@pinchies
Copy link
Contributor Author

pinchies commented Aug 9, 2019

I don’t see the issue fixed yet?

@tpruvot
Copy link
Contributor

tpruvot commented Aug 9, 2019

commit 92c35d1

@pinchies pinchies closed this Aug 9, 2019
@pinchies pinchies deleted the fix-sd-no-file-eeprom branch August 9, 2019 09:52
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