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 File::availableForWrite() #7650

Closed
fabianoriccardi opened this issue Oct 10, 2020 · 12 comments · Fixed by #7658
Closed

Sd File::availableForWrite() #7650

fabianoriccardi opened this issue Oct 10, 2020 · 12 comments · Fixed by #7658

Comments

@fabianoriccardi
Copy link

Hi, I have recently discovered int availableForWrite() to check the amount of available bytes without waiting for new "sector" (hence avoiding blocking writings). here) the example that demonstrate it.

Is there any plan to add to esp8266 core?

@earlephilhower
Copy link
Collaborator

@fabiuz7, interesting note. I'm not sure that the same technique can be used by our newer version of the SdFat library (the Arduino fork is ancient...), but if it is then it seems like it would be a reasonably simple PR. Would you be interested in submitting one?

@JAndrassy
Copy link
Contributor

@fabianoriccardi
Copy link
Author

I may try, but I'm not very familiar with sdfat library. However I was looking at esp8266 arduino repo, and I found that Serial and WiFi already as a similar method. Does a virtual "availableForWrite" in Print class introduce conflict with WiFi and Serial classes?

@earlephilhower
Copy link
Collaborator

Good pointer, @JAndrassy! Looks like that was added ~1yr ago to the Arduino upstream core.

@fabiuz7 In the end if there's no compile-time ambiguity, it should be fine. I think that's what would be needed, anyway. You can add it in a PR and the continuous integration system will verify no compiles get broken.

@JAndrassy
Copy link
Contributor

Good pointer, @JAndrassy! Looks like that was added ~1yr ago to the Arduino upstream core.

I linked to API repo which is not really used. it missed it too. to AVR and SAMD it was added in 2017

earlephilhower added a commit to earlephilhower/Arduino that referenced this issue Oct 17, 2020
Adds an availableForWrite() method to the Print class, matching current
ArduinoCore-API commit 398e70f188e2b861c10d9ffe5e2bfcb6a4a4f489 .

Hook availableForWrite into the SDFS filesystem (other FSes don't have
this capability built-in).

Fixes esp8266#7650
@earlephilhower
Copy link
Collaborator

@fabiuz7 can you give #7658 a try and report back? I don't have my SD card reader hooked up to verify the new call.

@earlephilhower earlephilhower added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label Oct 17, 2020
@fabianoriccardi
Copy link
Author

Hi, I had tried this sketch. It compiles but it continues to store the data in the local buffer, the SD card is never written. availableForWrite return always 0. Is it intended?

@earlephilhower
Copy link
Collaborator

Hi, I had tried this sketch. It compiles but it continues to store the data in the local buffer, the SD card is never written. availableForWrite return always 0. Is it intended?

Thanks for the feedback. I double checked (added the avail4Write call to LittleFS and got expected results) the code and it's actually working correctly.

The latest SDFat library does not support availableForWrite (see https://github.com/greiman/SdFat ), so that sketch can't work on it. It looks like the Arduino guys preserved the 7(!) year old fork they took and have added the call to it. So, on the 8266 it's calling the default "return 0" handler.

I don't know why Arduino would have added new stuff to the ancient fork vs. pulling in the currently maintained version and doing a PR.

https://github.com/arduino-libraries/SD/blame/85dbcca432d1b658d0d848f5f7ba0a9e811afc04/src/utility/SdFile.cpp#L1492

Let me see if the hack they did can be added to our own ESP8266SdFat library. I'm not so sure, given their ancient codebase, but will let you know if something comes up.

@earlephilhower earlephilhower removed the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label Oct 21, 2020
@earlephilhower
Copy link
Collaborator

I've spent some time trying to hack in the Arduino patch to the modern SD library and have to say that it's beaten me, @fabiuz7 . :(

There have been a lot of little changes to the SD library architecture which make some of the hacks Arduino did not directly translatable. So, I think you's out of luck for SD.availableForWrite() for the time being. The actual patch to consolidate it under a Print class, however, should still go through if only for consistency.

@fabianoriccardi
Copy link
Author

Thanks for the investigation. That's a pity that Arduino's version diverged so much from the original SdFat, limiting the inclusion of features in other cores :(

d-a-v pushed a commit that referenced this issue Oct 27, 2020
* Add Print::availableForWrite method

Adds an availableForWrite() method to the Print class, matching current
ArduinoCore-API commit 398e70f188e2b861c10d9ffe5e2bfcb6a4a4f489 .

Hook availableForWrite into the SDFS filesystem (other FSes don't have
this capability built-in).

Fixes #7650

* WiFiClient::availableForWrite proto matching Print

* Fix Netdump signedness warning

* Clean up Serial availableForWrite

This is evidently a breaking change due to the type difference.
Arduino's `availableForWrite` returns an `int`, while the
(multiply-implemented, non-virtual) core `availableForWrite` returned
`size_t`.
@fabianoriccardi
Copy link
Author

Hi again,
I digged a bit into sdfat library, and I discovered the old isBusy() method. Is it the core method to implement availableForWrite, isn't it? This method is pretty old, and it can be found in the fork of @earlephilhower, so maybe there is a change to implement it, or at least to avoid long sporadic writings.

earlephilhower added a commit to earlephilhower/Arduino that referenced this issue Dec 22, 2020
@earlephilhower
Copy link
Collaborator

See #7779

earlephilhower added a commit that referenced this issue Dec 23, 2020
* Update to upstream SdFat 2.0.2

Increases the read/write performance for SD card accesses
by a significant amount, up to 5x (3+MB/s) in testing.

Fixes #7772 

* Add SDFS::availableForWrite handler

Peek into the sector cache to determine the maximum number of
bytes that can be written w/o needing a (slow) SD operation.

Fixes #7650
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 a pull request may close this issue.

3 participants