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

[FATFileSystem] Disabled flush on new sector #1458

Closed
wants to merge 1 commit into from
Closed

[FATFileSystem] Disabled flush on new sector #1458

wants to merge 1 commit into from

Conversation

neilt6
Copy link
Contributor

@neilt6 neilt6 commented Nov 26, 2015

Disabled "flush on new sector" by default for a ~2.87x write performance improvement.

Disabled "flush on new sector" by default for a ~2.87x write performance improvement.
@neilt6
Copy link
Contributor Author

neilt6 commented Nov 26, 2015

As an example of the effect on performance, my SDFileSystem_HelloWorld program yielded an average write speed of 142.33KB/s with FLUSH_ON_NEW_SECTOR enabled, and 409.18KB/s with it disabled.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 7, 2015

I'm not sure what the justification for having these enabled by default is, they're detrimental to write performance

I am neither. @sg- Any help here?

@adamgreen
Copy link
Contributor

This feature was added and enabled in commit 2251b02. Disabling it by default would make the code work as the mbed port did originally.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 7, 2015

cc @oliviermartin

@sg-
Copy link
Contributor

sg- commented Dec 11, 2015

from the commit message we should look at why fflush isnt/cant be retargeted.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 23, 2015

@oliviermartin @neilt6 any more info for this changeset?

@neilt6
Copy link
Contributor Author

neilt6 commented Dec 23, 2015

Apparently _sys_ensure() was the function to override in order to retarget fflush(), but it's deprecated and no longer called by any library function. In my opinion, this will only be a concern to data logging applications where power might be removed before files are closed, in which case developers could enable automatic flushing on their own.

@oliviermartin
Copy link
Contributor

I added this feature when I wrote a basic data logger with mbed.
As I said in my patch there is no easy way to retarget fflush() for mbed. Adding this feature would allow to revert this patch and write better data logger code (ie: code that invokes fflush() only when needed).

@neilt6
Copy link
Contributor Author

neilt6 commented Feb 24, 2016

Is this pull request still being considered, or should I withdraw it?

@neilt6 neilt6 closed this Feb 29, 2016
@neilt6 neilt6 deleted the fatfs-ffconf-patch branch February 29, 2016 16:13
@adamgreen adamgreen mentioned this pull request Mar 9, 2016
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