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: implement readBytes #4931

Merged
merged 3 commits into from
Oct 5, 2018
Merged

Conversation

icosaeder
Copy link
Contributor

There is a speed issue when sending the files from SD card over HTTP.
The problem is, that the SD-specific class File inherited from Stream does not override the
virtual size_t readBytes(char *buffer, size_t length)
method, while this method is used under the hood of ESP8266WebServer::streamFile:

Stream::readBytes
BufferedStreamDataSource::get_buffer
ClientContext::_write_some
ClientContext::_write_from_source
ClientContext::write
WiFiClient::write
ESP8266WebServer::streamFile

The default implementation of readBytes in the Stream class does not actually use buffered reading, just a loop reading bytes one by one (through Stream::timedRead() method), which is obviously slow.

The proposed fix redirects readBytes to buffered read of SD-specific File class.

This speeds up the ESP8266WebServer::streamFile more than 3 times. Tested on streaming the 800+ Kb file from SD (FAT32), average time without a fix was 9000 ms, with the fix is 2600 ms (maximal possible SPI speed used), which is as fast as streaming the same file from internal SPIFFS. Hardware: WeMos D1 mini.

@devyte
Copy link
Collaborator

devyte commented Jul 17, 2018

I have some concerns about the casts, but that is a matter for discussion, and likely eventual cleanup elsewhere.
=> approving.

@devyte devyte self-assigned this Jul 17, 2018
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I think the cast ugliness, @devyte, is generally dictated by the mismash that is Arduino File and Stream classes.

@icosaeder
Copy link
Contributor Author

Thank you for approving :)

@earlephilhower earlephilhower merged commit 9bc8ea1 into esp8266:master Oct 5, 2018
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.

3 participants