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

add SDCard support for ESP32 #1780

Closed
wants to merge 1 commit into from
Closed

add SDCard support for ESP32 #1780

wants to merge 1 commit into from

Conversation

MaBecker
Copy link
Contributor

by removing FLASHFS

by removing FLASHFS
@MaBecker
Copy link
Contributor Author

added pr #1779

@gfwilliams
Copy link
Member

FlashFS was there for a reason (to provide easy access to on-chip flash). I'm not sure it should just be removed?

@wilberforce was the one that added it AFAIK.

@MaBecker
Copy link
Contributor Author

Just checked, ESP32 is the one and only that is using FlashFS.

FlashFS was there for a reason (to provide easy access to on-chip flash). I'm not sure it should just be removed?

Can you please lookup the download stat for ESP32.

Personally I am a big fan of the Storage lib and try to avoid SDCards where ever it is possible.

There are only very little section added as flash 8k and 64k

JsVar *jshFlashGetFree() {
JsVar *jsFreeFlash = jsvNewEmptyArray();
if (!jsFreeFlash) return 0;
// Space reserved here in the parition table - using sub type 0x40
// This should be read from the partition table
addFlashArea(jsFreeFlash, 0xE000, 0x2000);
addFlashArea(jsFreeFlash, 0x2B0000, 0x10000);
return jsFreeFlash;
}

Ups 0x2B0000 has to be changed to 0x310000

@MaBecker
Copy link
Contributor Author

just added #1781

@wilberforce
Copy link
Member

I don't understand why removing this is an improvement to the ESP32 implementation.

All chips have 4MB or more of memory, and 1Mb had been reserved at work as FlashFS.

Why would you remove this to use an external SD card that only some of the boards have?

Worst case - make it switchable to use either - I just don't see what the benefit is here?

@MaBecker
Copy link
Contributor Author

Thanks for your input. Will check if both can be used.

@gfwilliams
Copy link
Member

Another option is now Storage is better, to extend the flash area used for that and remove FlashFS.

@MaBecker
Copy link
Contributor Author

Using both at the same time cause a undefined reference to sdSPISetup

libs/filesystem/jswrap_file.o:(.literal.jswrap_file_kill+0x0): undefined reference to `sdSPISetup'
libs/filesystem/jswrap_file.o: In function `jswrap_file_kill':
/Espruino/repos/MaBecker/libs/filesystem/jswrap_file.c:214: undefined reference to `sdSPISetup'
libs/filesystem/jswrap_file.o: In function `jswrap_E_connectSDCard':
/Espruino/repos/MaBecker/libs/filesystem/jswrap_file.c:155: undefined reference to `sdSPISetup'
collect2: error: ld returned 1 exit status

@MaBecker MaBecker mentioned this pull request Mar 23, 2020
33 tasks
@MaBecker
Copy link
Contributor Author

Not sure how to continue.

Fact : Adding FLASHFS has broken SDCard function.

Suggestion: Remove FLASHFS for now and bring it back when someone has fixed the linker issue.

OR: Close this and remove SDCard support form ESP32 docs.

@MaBecker
Copy link
Contributor Author

Any hints how to handle the conflict named above?

@gfwilliams
Copy link
Member

I guess I feel a little like given how much flash there is available, having the filesystem built in is probably more useful than SD card support? I haven't seen too many ESPxx boards with SD cards on? Maybe just the ESP32CAM ones?

I think probably just update the docs to say it's not supported? I don't think you can easily have them coexist since FlashFS uses a 4k block size but SD cards will only want 512

@MaBecker
Copy link
Contributor Author

So let's close this.

@MaBecker MaBecker closed this Jul 13, 2020
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.

3 participants