fix(client): Implement readBytes in NetworkClient for faster downloads #9824

me-no-dev · 2024-06-11T11:35:51Z

Test downloading 22MB zip and using `readBytes` with ESP32-S3

Test results are relative and actual speeds will vary based on the environment.

Download from HTTP server (no SSL):

// python3 -m http.server 8080 -d .
const char *fileDownloadUrl = "http://192.168.254.91:8080/esp32-3.0.1.zip";

New Method:

Download completed!
Total download time: 40.714000 s
Average download speed: 573.149578 Kb/s

Old Method:

Download completed!
Total download time: 63.890000 s
Average download speed: 365.240443 Kb/s

Download from HTTPS server (with SSL):

const char *fileDownloadUrl = "https://github.com/espressif/arduino-esp32/releases/download/3.0.1/esp32-3.0.1.zip";

New Method:

Download completed!
Total download time: 80.371000 s
Average download speed: 290.343680 Kb/s

Old Method:

Download completed!
Total download time: 162.368000 s
Average download speed: 143.718047 Kb/s

github-actions · 2024-06-11T11:36:28Z

	Messages
📖	You might consider squashing your 5 commits (simplifying branch history).

👋 Hello me-no-dev, we appreciate your contribution to this project!

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Addressing info messages (📖) is strongly recommended; they're less critical but valuable.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 3eb2390

github-actions · 2024-06-11T11:43:05Z

Test Results

56 files 56 suites 5m 11s ⏱️
21 tests 21 ✅ 0 💤 0 ❌
135 runs 135 ✅ 0 💤 0 ❌

Results for commit 3eb2390.

♻️ This comment has been updated with latest results.

JAndrassy · 2024-06-11T12:27:15Z

readBytes is a function if the Stream class and should wait Stream's timeout for next byte
https://www.arduino.cc/reference/en/language/functions/communication/stream/streamreadbytes/
please don't override it, it will break sketches.
use read if it shouldn't block

me-no-dev · 2024-06-11T12:41:42Z

@JAndrassy then maybe different solution is needed. reading network streams byte by byte is never a good idea

JAndrassy · 2024-06-11T12:46:20Z

I meant to use read(buffer, size) from Client, not read(byte)

me-no-dev · 2024-06-11T13:05:10Z

@JAndrassy I got it, but readBytes reads byte by byte and that is also not good for Network, is what I meant

JAndrassy · 2024-06-11T13:42:47Z

@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)
the readBytes in Stream is not virtual so it shouldn't be override.

me-no-dev · 2024-06-11T13:48:31Z

So you suggest to leave Client to read byte by byte and waste a lot of time doing it?

JAndrassy · 2024-06-11T16:11:14Z

So you suggest to leave Client to read byte by byte and waste a lot of time doing it?

there is read)buffer, size). that should be used.

me-no-dev · 2024-06-11T20:05:43Z

Stream.readBytes says nowhere that we should be waiting for every single byte, therefore we can wait when there are no bytes. Do you get this, or are we just arguing for no reason?

SuGlider · 2024-06-11T20:40:10Z

This may affect OTA and designed time out...

SuGlider · 2024-06-11T20:40:59Z

Sorry for closing it... It was some bad functioning within my Windows system.

SuGlider · 2024-06-11T20:46:16Z

About Stream::readBytes() --> it uses Stream::timedRead(). Setting timeout to Zero using Stream::setTimeout(0) may help WiFi streams using such function.

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.

SuGlider · 2024-06-11T20:49:55Z

Please check #9823 (comment)
This is why timeout is important for certain Network operations.

me-no-dev · 2024-06-12T05:10:22Z

@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

hitecSmartHome · 2024-06-12T06:37:42Z

I'm more than happy to test if it will be compatible with arduino 2.0.17

me-no-dev · 2024-06-12T07:31:36Z

@hitecSmartHome you can try the current changes on top of WiFiClient

JAndrassy · 2024-06-12T07:52:01Z

now updated implementation in this PR should use getTimeout(), not _timeout;.

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.

me-no-dev · 2024-06-12T08:21:11Z

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)

SuGlider

LGTM

github-actions · 2024-06-13T05:10:35Z

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.

Memory	FLASH [bytes]		FLASH [%]		RAM [bytes]		RAM [%]
Target	DEC	INC	DEC	INC	DEC	INC	DEC	INC
ESP32S3	0	0	0.00	0.00	0	0	0.00	0.00
ESP32S2	0	0	0.00	0.00	0	0	0.00	0.00
ESP32C3	0	0	0.00	0.00	0	0	0.00	0.00
ESP32C6	0	0	0.00	0.00	0	0	0.00	0.00
ESP32H2	0	0	0.00	0.00	0	0	0.00	0.00
ESP32	0	0	0.00	0.00	0	0	0.00	0.00

Click to expand the detailed deltas report [usage change in BYTES]

Target	ESP32S3		ESP32S2		ESP32C3		ESP32C6		ESP32H2		ESP32
Example	FLASH	RAM	FLASH	RAM	FLASH	RAM	FLASH	RAM	FLASH	RAM	FLASH	RAM
Ethernet/examples/ETH_W5500_Arduino_SPI	-	-	-	-	-	-	-	-	-	-	-	-
Ethernet/examples/ETH_W5500_IDF_SPI	-	-	-	-	-	-	-	-	-	-	-	-
NetworkClientSecure/examples/WiFiClientInsecure	-	-	-	-	-	-	-	-	-	-	-	-
NetworkClientSecure/examples/WiFiClientPSK	-	-	-	-	-	-	-	-	-	-	-	-
NetworkClientSecure/examples/WiFiClientSecure	-	-	-	-	-	-	-	-	-	-	-	-
NetworkClientSecure/examples/WiFiClientSecureEnterprise	-	-	-	-	-	-	-	-	-	-	-	-
NetworkClientSecure/examples/WiFiClientSecureProtocolUpgrade	-	-	-	-	-	-	-	-	-	-	-	-
NetworkClientSecure/examples/WiFiClientShowPeerCredentials	-	-	-	-	-	-	-	-	-	-	-	-
NetworkClientSecure/examples/WiFiClientTrustOnFirstUse	-	-	-	-	-	-	-	-	-	-	-	-
PPP/examples/PPP_Basic	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/AdvancedWebServer	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/FSBrowser	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/HelloServer	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/HttpAdvancedAuth	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/HttpAuthCallback	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/HttpAuthCallbackInline	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/HttpBasicAuth	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/HttpBasicAuthSHA1	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/HttpBasicAuthSHA1orBearerToken	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/MultiHomedServers	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/PathArgServer	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/SDWebServer	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/SimpleAuthentification	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/UploadHugeFile	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/WebServer	-	-	-	-	-	-	-	-	-	-	-	-
WebServer/examples/WebUpdate	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/FTM/FTM_Initiator	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/FTM/FTM_Responder	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/SimpleWiFiServer	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WPS	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiAccessPoint	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiBlueToothSwitch	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiClient	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiClientBasic	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiClientConnect	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiClientEnterprise	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiClientEvents	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiClientStaticIP	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiIPv6	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiMulti	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiMultiAdvanced	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiScan	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiScanAsync	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiScanDualAntenna	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiSmartConfig	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiTelnetToSerial	-	-	-	-	-	-	-	-	-	-	-	-
WiFi/examples/WiFiUDPClient	-	-	-	-	-	-	-	-	-	-	-	-
Ethernet/examples/ETH_LAN8720	-	-	-	-	-	-	-	-	-	-	-	-
Ethernet/examples/ETH_TLK110	-	-	-	-	-	-	-	-	-	-	-	-

fix(client): Implement readBytes in NetworkClient for faster downloads

29df67b

SuGlider closed this Jun 11, 2024

SuGlider reopened this Jun 11, 2024

SuGlider self-requested a review June 11, 2024 20:46

fix(client): Implement readBytes to obey the client timeout

2a5a7c5

fix(clieant): use getTimeout() instead

8c1804d

SuGlider approved these changes Jun 12, 2024

View reviewed changes

Merge branch 'master' into bugfix/client_read_bytes

1e2bf1b

me-no-dev added the Status: Pending Merge Pull Request is ready to be merged label Jun 13, 2024

ci(pre-commit): Apply automatic fixes

3eb2390

me-no-dev merged commit bc79feb into master Jun 13, 2024
53 checks passed

me-no-dev deleted the bugfix/client_read_bytes branch June 13, 2024 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(client): Implement readBytes in NetworkClient for faster downloads #9824

fix(client): Implement readBytes in NetworkClient for faster downloads #9824

me-no-dev commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading

JAndrassy commented Jun 11, 2024 •

edited

Loading

me-no-dev commented Jun 11, 2024

JAndrassy commented Jun 11, 2024 •

edited

Loading

me-no-dev commented Jun 11, 2024

JAndrassy commented Jun 11, 2024

me-no-dev commented Jun 11, 2024

JAndrassy commented Jun 11, 2024

me-no-dev commented Jun 11, 2024

SuGlider commented Jun 11, 2024

SuGlider commented Jun 11, 2024

SuGlider commented Jun 11, 2024

SuGlider commented Jun 11, 2024

me-no-dev commented Jun 12, 2024

hitecSmartHome commented Jun 12, 2024

me-no-dev commented Jun 12, 2024

JAndrassy commented Jun 12, 2024 •

edited

Loading

me-no-dev commented Jun 12, 2024

SuGlider left a comment

github-actions bot commented Jun 13, 2024

fix(client): Implement readBytes in NetworkClient for faster downloads #9824

fix(client): Implement readBytes in NetworkClient for faster downloads #9824

Conversation

me-no-dev commented Jun 11, 2024 • edited Loading

Test downloading 22MB zip and using readBytes with ESP32-S3

Download from HTTP server (no SSL):

Download from HTTPS server (with SSL):

github-actions bot commented Jun 11, 2024 • edited Loading

github-actions bot commented Jun 11, 2024 • edited Loading

Test Results

JAndrassy commented Jun 11, 2024 • edited Loading

me-no-dev commented Jun 11, 2024

JAndrassy commented Jun 11, 2024 • edited Loading

me-no-dev commented Jun 11, 2024

JAndrassy commented Jun 11, 2024

me-no-dev commented Jun 11, 2024

JAndrassy commented Jun 11, 2024

me-no-dev commented Jun 11, 2024

SuGlider commented Jun 11, 2024

SuGlider commented Jun 11, 2024

SuGlider commented Jun 11, 2024

SuGlider commented Jun 11, 2024

me-no-dev commented Jun 12, 2024

hitecSmartHome commented Jun 12, 2024

me-no-dev commented Jun 12, 2024

JAndrassy commented Jun 12, 2024 • edited Loading

me-no-dev commented Jun 12, 2024

SuGlider left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 13, 2024

Memory usage test (comparing PR against master branch)

me-no-dev commented Jun 11, 2024 •

edited

Loading

Test downloading 22MB zip and using `readBytes` with ESP32-S3

github-actions bot commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 11, 2024 •

edited

Loading

JAndrassy commented Jun 11, 2024 •

edited

Loading

JAndrassy commented Jun 11, 2024 •

edited

Loading

JAndrassy commented Jun 12, 2024 •

edited

Loading