ESPLwIPClient::setTimeout conflict fix with Stream::setTimeout #6676

P-R-O-C-H-Y · 2022-05-02T11:51:20Z

Description of Change

Arduino Stream::settimeout() is not virtual so it cannot be virtual in ESPLwIPClient.
This change should fix this conflict.

Tests scenarios

Unit Test Results

0 files 0 suites 0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌

Results for commit 17adb9d.

♻️ This comment has been updated with latest results.

igrr

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.

libraries/WiFi/src/WiFiClient.cpp

P-R-O-C-H-Y · 2022-05-02T13:26:08Z

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

igrr · 2022-05-02T13:52:06Z

libraries/HTTPClient/src/HTTPClient.cpp

@@ -495,7 +495,7 @@ void HTTPClient::setTimeout(uint16_t timeout)
 {
    _tcpTimeout = timeout;
    if(connected()) {
-        _client->setTimeout((timeout + 500) / 1000);
+        _client->setTimeout(timeout + 500); /* / 1000 removed, WifiClient setTimeout changed to ms */


@P-R-O-C-H-Y I would suggest to put notes such as this into commit message body, instead of adding them as comments. The reader of the code is probably not interested in every change which has been done to each line, they just need to understand the final version. (If they are interested in change history, they can use git blame and git log and find the details in commit messages.)

For example, I don't understand why there is timeout+500 here.

I think previously +500 in (timeout + 500)/1000 was related to rounding. But with /1000 removed, looks like +500 is not needed anymore?

Yes, this was there for rounding. +500 should be removed

The rounding was removed, missed that.
Thanks for suggestions about commenting the code / commits. Will have it in mind for future changes :)

igrr · 2022-05-02T13:52:58Z

One more comment about leftover +500 and not using comments as a changelog. Otherwise still LGTM.

me-no-dev · 2022-05-02T14:06:18Z

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

JAndrassy · 2022-05-02T15:27:03Z

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()?

gonzabrusco · 2022-05-03T12:09:59Z

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?

P-R-O-C-H-Y · 2022-05-03T12:55:19Z

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);

P-R-O-C-H-Y · 2022-05-03T13:01:03Z

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.

JAndrassy · 2022-05-03T17:14:39Z

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.

mrengineer7777 · 2022-05-03T17:36:03Z

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?

VojtechBartoska · 2022-05-04T08:16:22Z

We are postponing this Pull Request as more investigation and testing is needed. Added to 2.1.0 milestone.

podaen · 2022-05-07T07:09:52Z

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.

me-no-dev · 2022-05-09T09:31:52Z

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)

podaen · 2022-05-09T09:45:23Z

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.

TD-er · 2022-07-22T13:39:14Z

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.

VojtechBartoska · 2023-12-12T08:34:19Z

relates #6835

me-no-dev · 2023-12-13T11:14:46Z

@P-R-O-C-H-Y can you rebase this please

…or now

+ unnecessary comments removed.

github-actions · 2023-12-13T11:36:44Z

	Warnings
⚠️	Some issues found for the commit messages in this PR: the commit message `"Added 0 init values to constructor"`: summary looks empty type/action looks empty the commit message `"Applied same changes for WifiClientSecure"`: summary looks empty type/action looks empty the commit message `"Changed seconds to miliseconds in other classes relaed + examples"`: summary looks empty type/action looks empty the commit message `"Fix rebase code"`: summary looks empty type/action looks empty the commit message `"Fix rebased code"`: summary looks empty type/action looks empty the commit message `"Removed no needed code + edit"`: summary looks empty type/action looks empty the commit message `"Removed virtual + moved socketOptions ot read/write"`: summary looks empty type/action looks empty the commit message `"Seconds are not rounded now"`: summary looks empty type/action looks empty the commit message `"fix rebased code in WifiClientSecure"`: summary looks empty type/action looks empty the commit message `"removed +500 for previous rounding"`: summary looks empty type/action looks empty the commit message `"removed Client::getTimeout"`: summary looks empty type/action looks empty the commit message `"removed setTimeout from WifiClient - read/write timeouts in constructor now"`: summary looks empty type/action looks empty Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: `<type/action>(<scope/component>): <summary>`, for example `fix(esp32): Fixed startup timeout issue` allowed types are: `change,ci,docs,feat,fix,refactor,remove,revert` sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) `TIP:` Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).
⚠️	Please consider squashing your 13 commits (simplifying branch history).
⚠️	The source branch `"WifiClient_settimeout_conflict"` incorrect format: contains uppercase letters. This can cause troubles on case-insensitive file systems (macOS). Please rename your 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 🚫 dangerJS against 0e1d13a

libraries/WiFiClientSecure/src/WiFiClientSecure.cpp

libraries/WiFiClientSecure/src/WiFiClientSecure.h

JAndrassy · 2023-12-13T12:06:27Z

@me-no-dev I recommend my PR #8863 instead of this one

me-no-dev · 2023-12-13T14:02:00Z

@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.

JAndrassy · 2023-12-13T15:29:39Z

so my PR renamed setTimeout to setConnectionTimeout. this one now removed setTimeout, and my PR adds it back as setConnectionTimeout. Except of WiFiClientSecure where this PR didn't remove the shadowing setTimeout so my PR still renames it to setConnectionTimeout.
the rest of the PR was same except a comment like
/* /1000 removed, WifiClient setTimeout changed to ms */
(which doesn't make sense for a future reader)
end of rant

EDIT: sorry, I missed the _lastReadTimeout and _lastWriteTimeout. why was it in one PR?

cziter15 · 2023-12-22T07:12:46Z

libraries/WiFi/src/WiFiClient.cpp

+            struct timeval timeout_tv;
+            timeout_tv.tv_sec = _timeout / 1000;
+            timeout_tv.tv_usec = (_timeout % 1000) * 1000;
+            if(setSocketOption(SO_RCVTIMEO, (char *)&timeout_tv, sizeof(struct timeval)) >= 0)


Cosmethic change, but I suggest to use sizeof(timeout_tv) instead of sizeof(struct timeval)

I disagree, as it is good practice to actually use the struct/class for the sizeof() instead of the object.
This way you are less likely to end up with the value 4 (or 8 when compiling for a 64-bit platform) as result of your sizeof() call when working with pointers instead of reference.
Thus this will reduce the chance of programming errors.

I agree, when it comes to pointers... but imagine you change the type of timeout_tv somewhere earlier and forget to replace all sizeof (struct timeval) occurences later in the code. Maybe a better thing is to declare a const variable/define/constexpr timeval_size just after the timeout_tv declaration and then use this variable later in the code.

Sure, or maybe there is a compiler flag which can warn when sizeof() is given a pointer as that's the actual problem.

cziter15 · 2023-12-22T07:13:36Z

libraries/WiFiClientSecure/src/WiFiClientSecure.cpp

+        struct timeval timeout_tv;
+        timeout_tv.tv_sec = _timeout / 1000;
+        timeout_tv.tv_usec = (_timeout % 1000) * 1000;
+        if(setSocketOption(SO_SNDTIMEO, (char *)&timeout_tv, sizeof(struct timeval)) >= 0)


Cosmethic change, but I suggest to use sizeof(timeout_tv) instead of sizeof(struct timeval)

cziter15 · 2023-12-22T07:13:55Z

libraries/WiFiClientSecure/src/WiFiClientSecure.cpp

+            struct timeval timeout_tv;
+            timeout_tv.tv_sec = _timeout / 1000;
+            timeout_tv.tv_usec = (_timeout % 1000) * 1000;
+            if(setSocketOption(SO_RCVTIMEO, (char *)&timeout_tv, sizeof(struct timeval)) >= 0)


Cosmethic change, but I suggest to use sizeof(timeout_tv) instead of sizeof(struct timeval)

P-R-O-C-H-Y self-assigned this May 2, 2022

P-R-O-C-H-Y added the Area: BT&Wifi BT & Wifi related issues label May 2, 2022

P-R-O-C-H-Y added this to the 2.0.3 milestone May 2, 2022

P-R-O-C-H-Y requested review from me-no-dev and igrr May 2, 2022 11:51

P-R-O-C-H-Y mentioned this pull request May 2, 2022

ESPLwIPClient setTimeout conflicts with Stream setTimeout #5558

Closed

P-R-O-C-H-Y marked this pull request as draft May 2, 2022 12:19

igrr approved these changes May 2, 2022

View reviewed changes

libraries/WiFi/src/WiFiClient.cpp Outdated Show resolved Hide resolved

P-R-O-C-H-Y marked this pull request as ready for review May 2, 2022 13:31

igrr reviewed May 2, 2022

View reviewed changes

P-R-O-C-H-Y removed this from the 2.0.3 milestone May 3, 2022

VojtechBartoska added this to the 2.1.0 milestone May 4, 2022

P-R-O-C-H-Y mentioned this pull request May 13, 2022

HTTPClient setTimeout hasn't any effect #1433

Closed

VojtechBartoska mentioned this pull request Jun 7, 2022

Inconsistency in setTimeout implementation #6835

Closed

VojtechBartoska mentioned this pull request Nov 28, 2023

WiFiClients.setConnectionTimeout added #8863

Merged

VojtechBartoska modified the milestones: 3.0.0, 3.0.0-RC1 Nov 28, 2023

P-R-O-C-H-Y added 9 commits December 13, 2023 12:33

Removed virtual + moved socketOptions ot read/write

5564372

Removed no needed code + edit

12699eb

removed Client::getTimeout

c1183e4

removed setTimeout from WifiClient - read/write timeouts in construct…

2f49062

…or now

Changed seconds to miliseconds in other classes relaed + examples

e6e77ff

Applied same changes for WifiClientSecure

b1193c2

Added 0 init values to constructor

6420e3a

Seconds are not rounded now

bee59c1

removed +500 for previous rounding

b986041

+ unnecessary comments removed.

P-R-O-C-H-Y force-pushed the WifiClient_settimeout_conflict branch from 17adb9d to b986041 Compare December 13, 2023 11:36

Merge branch 'master' into WifiClient_settimeout_conflict

675484c

me-no-dev reviewed Dec 13, 2023

View reviewed changes

libraries/WiFiClientSecure/src/WiFiClientSecure.cpp Outdated Show resolved Hide resolved

P-R-O-C-H-Y added 3 commits December 13, 2023 12:41

fix rebased code in WifiClientSecure

77830cf

Fix rebased code

7d86e1d

Fix rebase code

0e1d13a

me-no-dev reviewed Dec 13, 2023

View reviewed changes

libraries/WiFiClientSecure/src/WiFiClientSecure.h Outdated Show resolved Hide resolved

me-no-dev merged commit 29cde94 into espressif:master Dec 13, 2023
39 checks passed

P-R-O-C-H-Y mentioned this pull request Dec 14, 2023

Remove setTimeout from WifiClientSecure #8998

Merged

cziter15 reviewed Dec 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESPLwIPClient::setTimeout conflict fix with Stream::setTimeout #6676

ESPLwIPClient::setTimeout conflict fix with Stream::setTimeout #6676

P-R-O-C-H-Y commented May 2, 2022

P-R-O-C-H-Y commented May 2, 2022

github-actions bot commented May 2, 2022 •

edited

Loading

igrr left a comment

P-R-O-C-H-Y commented May 2, 2022

igrr May 2, 2022 •

edited

Loading

me-no-dev May 2, 2022

P-R-O-C-H-Y May 2, 2022

igrr commented May 2, 2022

me-no-dev commented May 2, 2022

JAndrassy commented May 2, 2022

gonzabrusco commented May 3, 2022

P-R-O-C-H-Y commented May 3, 2022

P-R-O-C-H-Y commented May 3, 2022

JAndrassy commented May 3, 2022

mrengineer7777 commented May 3, 2022

VojtechBartoska commented May 4, 2022

podaen commented May 7, 2022

me-no-dev commented May 9, 2022

podaen commented May 9, 2022 •

edited

Loading

TD-er commented Jul 22, 2022

VojtechBartoska commented Dec 12, 2023

me-no-dev commented Dec 13, 2023

github-actions bot commented Dec 13, 2023 •

edited

Loading

JAndrassy commented Dec 13, 2023

me-no-dev commented Dec 13, 2023

JAndrassy commented Dec 13, 2023 •

edited

Loading

cziter15 Dec 22, 2023 •

edited

Loading

TD-er Dec 22, 2023 •

edited

Loading

cziter15 Dec 22, 2023 •

edited

Loading

TD-er Dec 22, 2023

cziter15 Dec 22, 2023

cziter15 Dec 22, 2023

ESPLwIPClient::setTimeout conflict fix with Stream::setTimeout #6676

ESPLwIPClient::setTimeout conflict fix with Stream::setTimeout #6676

Conversation

P-R-O-C-H-Y commented May 2, 2022

Description of Change

Tests scenarios

Related links

P-R-O-C-H-Y commented May 2, 2022

github-actions bot commented May 2, 2022 • edited Loading

Unit Test Results

igrr left a comment

Choose a reason for hiding this comment

P-R-O-C-H-Y commented May 2, 2022

igrr May 2, 2022 • edited Loading

Choose a reason for hiding this comment

me-no-dev May 2, 2022

Choose a reason for hiding this comment

P-R-O-C-H-Y May 2, 2022

Choose a reason for hiding this comment

igrr commented May 2, 2022

me-no-dev commented May 2, 2022

JAndrassy commented May 2, 2022

gonzabrusco commented May 3, 2022

P-R-O-C-H-Y commented May 3, 2022

P-R-O-C-H-Y commented May 3, 2022

JAndrassy commented May 3, 2022

mrengineer7777 commented May 3, 2022

VojtechBartoska commented May 4, 2022

podaen commented May 7, 2022

me-no-dev commented May 9, 2022

podaen commented May 9, 2022 • edited Loading

TD-er commented Jul 22, 2022

VojtechBartoska commented Dec 12, 2023

me-no-dev commented Dec 13, 2023

github-actions bot commented Dec 13, 2023 • edited Loading

JAndrassy commented Dec 13, 2023

me-no-dev commented Dec 13, 2023

JAndrassy commented Dec 13, 2023 • edited Loading

cziter15 Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

TD-er Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

cziter15 Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

TD-er Dec 22, 2023

Choose a reason for hiding this comment

cziter15 Dec 22, 2023

Choose a reason for hiding this comment

cziter15 Dec 22, 2023

Choose a reason for hiding this comment

github-actions bot commented May 2, 2022 •

edited

Loading

igrr May 2, 2022 •

edited

Loading

podaen commented May 9, 2022 •

edited

Loading

github-actions bot commented Dec 13, 2023 •

edited

Loading

JAndrassy commented Dec 13, 2023 •

edited

Loading

cziter15 Dec 22, 2023 •

edited

Loading

TD-er Dec 22, 2023 •

edited

Loading

cziter15 Dec 22, 2023 •

edited

Loading