Skip to content

Remove SD file available size saturation #3209

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

Merged
merged 1 commit into from
May 9, 2017
Merged

Remove SD file available size saturation #3209

merged 1 commit into from
May 9, 2017

Conversation

korzo89
Copy link
Contributor

@korzo89 korzo89 commented May 6, 2017

A problem with file streaming arose during some experiments with the SDWebServer example. It seems that the current version of the software has a problem with sending large files stored on the SD card to connected clients.

After trying to find the cause of this problem, I have found that large files are always streamed up to the first 32767 bytes, regardless of the value returned by the size() method of the File class. This can be easily seen in browser network diagnostic tools:
1

The issue is caused by the SD library's File class available() method, which causes its result to be saturated to 0x7FFF (32767), making the file streaming function behave incorrectly. After removing the value clamping code, the streaming works perfectly for files with sizes larger than 32kB:
2

I have also tested the fix with very large files (~40MB) and can confirm that it works (albeit quite slowly).

I suspect that the value saturation code was borrowed from the original Arduino SD libraries, which usually use 8-bit AVR compilers, where the int type (returned by the available() method) is a 16-bit integer - hence the clamping.

@igrr
Copy link
Member

igrr commented May 8, 2017

Based on the description it looks like this fixes #2915.

@igrr igrr added this to the 2.4.0 milestone May 8, 2017
@igrr igrr merged commit b623613 into esp8266:master May 9, 2017
@codecov-io
Copy link

Codecov Report

Merging #3209 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3209   +/-   ##
======================================
  Coverage    27.6%   27.6%           
======================================
  Files          20      20           
  Lines        3655    3655           
  Branches      678     678           
======================================
  Hits         1009    1009           
  Misses       2468    2468           
  Partials      178     178

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca3a172...f76b86a. Read the comment docs.

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