arduino-esp32
arduino-esp32 copied to clipboard
espressif
ESPLwIPClient::setTimeout conflict fix with Stream::setTimeout
Description of Change
Arduino Stream::settimeout() is not virtual so it cannot be virtual in ESPLwIPClient. This change should fix this conflict.
Tests scenarios
Related links
Closes #5558
Unit Test Results
0 files 0 suites 0s :stopwatch: 0 tests 0 :heavy_check_mark: 0 :zzz: 0 :x:
Results for commit 17adb9d9.
:recycle: This comment has been updated with latest results.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Just one note, LGTM otherwise.
Also please check if any examples used WifiClient::setTimeout and see if they should be updated to take milliseconds instead of seconds.
Checked and changed in this commit
One more comment about leftover +500 and not using comments as a changelog. Otherwise still LGTM.
So the titles says that you are fixing a conflict inheriting timeout between Stream and Client, but in the code I see setTimeout(seconds) being removed and "replaced" by Stream::setTimeout(milliseconds). This is a rather significant change for anyone who uses seconds in their code. Chances are that "seconds" would be low value that would cause premature timeout for their clients after this change is applied. Even if we should go forward with this, it seems like rather big change for 2.0.3
I would like to understand SO_RCVTIMEO and SO_SNDTIMEO. Does it make WiFiClient read() and write() blocking? Is it good to have the same timeout for read() (which shouldn't block) as for Stream class's blocking functions (readBytes, parseInt, find) which internally use read()?
Sorry I do not undestand something about this approach. What would be the correct way of setting the timeout now? I see you removed the setTimeout from WiFiClient and WiFiClientSecure...
I'm currently using a MQTT Class that takes a Client inherited class (WiFiClient, WiFiClientSecure, etc) and when you start a MQTT Class object (mqttObject.connect()), it internally calls Client.connect() but without a timeout parameter (this is out of my control unless the author modifies this). If I loose the ability to set the timeout of my Client beforehand it can be problematic because the MQTT class does not call connect passing a timeout parameter. How this situation can be handled with your proposal?
Sorry I do not undestand something about this approach. What would be the correct way of setting the timeout now? I see you removed the setTimeout from WiFiClient and WiFiClientSecure...
I'm currently using a MQTT Class that takes a Client inherited class (WiFiClient, WiFiClientSecure, etc) and when you start a MQTT Class object (mqttObject.connect()), it internally calls Client.connect() but without a timeout parameter (this is out of my control unless the author modifies this). If I loose the ability to set the timeout of my Client beforehand it can be problematic because the MQTT class does not call connect passing a timeout parameter. How this situation can be handled with your proposal?
setTimeout was removed from WifiClient and WifiClientSecure, but you are still able to call setTimeout from base class Stream.h. I was testing the changes on HTTPClient where you cannot pass the timeout parameter and everything works. See example below.
WiFiClient client;
HTTPClient http;
http.begin(client, serverName);
http.setTimeout(2500);
I would like to understand SO_RCVTIMEO and SO_SNDTIMEO. Does it make WiFiClient read() and write() blocking? Is it good to have the same timeout for read() (which shouldn't block) as for Stream class's blocking functions (readBytes, parseInt, find) which internally use read()?
Just want to mention, that the implementation is the same as it was before. This PR just resolves the conflict between the setTimeout functions. I don't understand, why the read shouldn't block? You may need to wait some time to get the data to read, if data do not come, do timeout.
I don't understand, why the read shouldn't block? You may need to wait some time to get the data to read, if data do not come, do timeout.
read() should only read what is immediately available in buffer. in embedded systems blocking can be a problem.
read() should only read what is immediately available in buffer. in embedded systems blocking can be a problem.
Blocking is a terrible thing for an embedded system. However, P-R-O-C-H-Y 's change replicates the functionality in the previous function int WiFiClient::setTimeout(uint32_t seconds); Removing that would change the library behavior.
The change of timeout from seconds to ms is a breaking change. This needs significant testing before release.
@P-R-O-C-H-Y Where is _timeout set? I see it referenced many times, but only set in the constructors (e.g. WiFiClient::WiFiClient():. Is that correct?
We are postponing this Pull Request as more investigation and testing is needed. Added to 2.1.0 milestone.
The change of timeout from seconds to ms is a breaking change.
Out of my tests setting the client, stream and/or SSL timeouts in ms is an easier way to set all timeouts to the same value.
Out of my tests setting the client, stream and/or SSL timeouts in ms is an easier way to set all timeouts to the same value.
Such change will break ANY current library/sketch that sets the timeout in seconds. This API has been such for long time (probably coming from 8266 even). Even worst, it will break it in such way that probably none of the sketched will be able to even connect to anything (imagine having set a timeout of 5 seconds and that turns into 5ms after the change)
Your right, Or m separate arguments for normal and unit values or by add an argument true/false... I'm still using now the old connect because of the older compability and set the settimeout before or after depending on the class I am using.
Out of my tests setting the client, stream and/or SSL timeouts in ms is an easier way to set all timeouts to the same value.
Such change will break ANY current library/sketch that sets the timeout in seconds. This API has been such for long time (probably coming from 8266 even). Even worst, it will break it in such way that probably none of the sketched will be able to even connect to anything (imagine having set a timeout of 5 seconds and that turns into 5ms after the change)
ESP8266 implementation does call Client::setTimeout, which uses msec.
On ESP32 it is IMHO done incorrect as it is effectively overriding the existing setTimeout function in Stream.
So it makes a difference if you call setTimeout on a point of type Stream or WiFiClient which is very confusing to say the least.
I'm not sure it will actually break existing implementations as I do expect almost anyone expects this to be in msec. So changing it may probably fix those implementations.
Another argument to change it into msec is that WiFiUDP uses msec for its setTimeout() function.
Also HTTPClient::setTimeout does call _client->setTimeout, where _client is a WiFiClient.
So this is ... confusing to say the least.
Just an observation from my side: I have a firmware which builds for ESP32 and ESP8266 which calls setTimeout() in some places (HTTPClient/WiFiClient and WiFIClientSecure) and I never actually realized, that the API wants me to use seconds on ESP32 (the ESP8266 code came first). Now reading this thread I went through my code and tried to fix all occurrences of setTimeout() (ms on ESP8266 and seconds on ESP32), which, you guessed it, broke it completely.
For me HTTPClient::setTimeout(5) sets the timeout to 5ms, which makes it unable to connect, setting it to 5000 seems to let it default to something reasonable like 8 seconds, which at least 'works'.
I think switching to ms might as well fix more programs then break 'em.
I recently ran into this problem as well (Arduino/ESP32 2.0.3). setTimeout() here is unclear if it should be seconds or ms:
void HTTPClient::setTimeout(uint16_t timeout) { _tcpTimeout = timeout; if(connected()) { _client->setTimeout((timeout + 500) / 1000); } }
_client call is WiFiClient::setTimeout(uint32_t seconds), so I assumed seconds but the connection immediately times out. It only works if I call HTTPClient::setTimeout(30000) for 30 seconds.
@mrengineer7777 and @everslick HTTPClient has milliseconds and is not a part of this issue
~~I was reviewing the PR #7686 and I made a suggestion to add time in milliseconds, but still keeping the compatibility to the API with seconds.~~
The idea would be use time as float.
I can tell Arduino to wait 500ms by using 0.5, 2 seconds by using 2 or 2.0, 1250ms by using 1.25 as timeout parameter.
int WiFiClient::setTimeout(float seconds)
~~I started a discussion about it in https://github.com/espressif/arduino-esp32/pull/7686#issuecomment-1407480299~~
It would cover the overload with uint32_t seconds given that C already promotes int to float automatically.
Please let me know your opinion about it.
Form define -> int32_t -> unsigned float -> unsigned long. You might change the name from timeout to a unit. :-) -> int
The idea would be use time as
float.
@SuGlider I cringe at the idea of float overhead. My preference would be to specify everything in ms. To avoid a breaking change perhaps we could introduce a new function setTimeout_ms()? This is a complicated PR so it needs more study. At the moment I would prefer to see some of our other open PRs merged.
I don't know if this is allready been sad and can be off topic, but why there is a timeout function in WiFiCient? Currently here are two moments where the timeout is set... by creating a WiFiClient (set the default [ms]) and in connecting before sending the request (custum timeout[sec]).
We are going in bridges. At the end it will be always in ms. I will comeback on this, but for now lets continue on thinking when the timeout need to be set. I think the solution of Garcia will work, if the promoted from uint32 to an foat works like they say.
I once changed the timeout in a huge lib working with streams and cam to the conclusion that set socket timeout must be able to set at any time. The stream timeout is to read, find etc, but can it go up? Stream is connected to Client, ESPLwIPClient, WiFiClient. The default timeout in Stream is 1ms, that was to small for finding a word, for cutting the streams in to chapters. I already had set the timeout in to WiFiClient, but the Stream timeout was not set. I remember now what the problem was... Running in a timeout with a stream and than auto connecting to the next stream, than the stream broke more often, when it else did not. Was it because that the socket timeout was set to default or was it that a new socket needed to be created. The solution was starting back from the beginning.
Continuing on the second or ms. WiFiClient::connect [ms] WiFiClient::setTimeout [sec]
Because the setsockopt from the LwIp is in seconds. So what I sad before that it always ends up in milliseconds is not true. But it doesn't make it easier and using the word 'timeout' doesn't helps now certainly when it gets more complicated. That is everything I got form working with streams. I hope it helps!
对不起，我对这种方法没有疑问。现在设置超时的正确方法是什么？我看到你从WiFiClient和WiFiClientSecure中删除了setTimeout... 我目前正在使用一个MQTT类，它采用客户端继承的类（WiFiClient，WiFiClientSecure等），当您启动MQTT类对象（mqttObject.connect（））时，它会在内部调用Client.connect（），但没有超时参数（除非作者修改，否则这是我无法控制的）。如果我事先失去了设置客户端超时的能力，这可能会有问题，因为 MQTT 类不会调用传递超时参数的连接。您的提案如何处理这种情况？
setTimeout 已从 WifiClient 和 WifiClientSecure 中删除，但您仍然可以从基类 Stream.h 调用 setTimeout。我正在测试HTTPClient上的更改，您无法传递超时参数并且一切正常。请参阅下面的示例。
WiFiClient client; HTTPClient http; http.begin(client, serverName); http.setTimeout(2500);
setTimeout were not delete WifiClient now
| Warnings | |
|---|---|
| :warning: |
Some issues found for the commit messages in this PR:
Please fix these commit messages - here are some basic tips:
|
| :warning: | Please consider squashing your 13 commits (simplifying branch history). |
| :warning: |
The source branch
|
👋 Hello P-R-O-C-H-Y, 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.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- 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 :no_entry_sign: dangerJS against 0e1d13a1655949c3c779c4b948dd3c2040be6b6e
@me-no-dev I recommend my PR https://github.com/espressif/arduino-esp32/pull/8863 instead of this one
@JAndrassy this PR solves a different thing, which is inheritance of setTimeout. Your PR should be rebased on top of this one, as it will not require some of your changes anymore.