-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
fix(client): Implement readBytes in NetworkClient for faster downloads #9824
Conversation
👋 Hello me-no-dev, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results 56 files 56 suites 5m 11s ⏱️ Results for commit 3eb2390. ♻️ This comment has been updated with latest results. |
readBytes is a function if the Stream class and should wait Stream's timeout for next byte |
@JAndrassy then maybe different solution is needed. reading network streams byte by byte is never a good idea |
I meant to use |
@JAndrassy I got it, but readBytes reads byte by byte and that is also not good for Network, is what I meant |
Stream doesn't have read(buffer, size), only read(byte) (pure virtual) |
So you suggest to leave Client to read byte by byte and waste a lot of time doing it? |
there is |
|
This may affect OTA and designed time out... |
Sorry for closing it... It was some bad functioning within my Windows system. |
About This PR should created some other Function Name... Or at least, test timeout for Zero and in case it is not zero, use the Stream Class original function. This PR implementation is equivalent to ignoring Timeout setup. |
Please check #9823 (comment) |
@SuGlider yes. It should wait the timeout, but should not read a byte by byte with timeout. Nobody said this is final implementation. This is network client, it gets many bytes. Currently the code that triggered this was downloading a large file from internet. Now imagine reading this file byte by byte and every time checking if more are available, if it got that single byte before that timeout and so on. Result is that you can not handle anything with speeds above 1Mbps on an S3 (I imagine much less on single core and slow RV chips). If we instead grab all bytes that are available at once and wait up to the timeout for more, then we can achieve speeds 3 times higher. I hope this makes it a bit more clear for you @SuGlider and @JAndrassy |
I'm more than happy to test if it will be compatible with arduino 2.0.17 |
@hitecSmartHome you can try the current changes on top of WiFiClient |
now updated implementation in this PR should use this implementation of readBytes will be used only if invoked on pointer of reference of type NetworkClient. if the pointer is ESPLwIPClient, Client or Stream type then the reaBytes from Stream will be executed. |
That is correct. Stream implementation works fine. This is to fix it for NetworkClient and NetworkClientSecure (and HTTPClient) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
Fixes: #9822
Test downloading 22MB zip and using
readBytes
with ESP32-S3Test results are relative and actual speeds will vary based on the environment.
Download from HTTP server (no SSL):
New Method:
Old Method:
Download from HTTPS server (with SSL):
New Method:
Old Method: