Fix error in WiFiClient.cpp where the connect function fails for timeouts below 1 second

[rtpmsys]

This change will allow specifying connect timeouts below 1 second. Without this change, if connect timeouts under 1 second are given, the connect defaults to 0ms and fails. This will also allow timeouts in fractions of seconds, e.g. 1500ms. Without this change, connect timeouts are truncated to full second increments.

By completing this PR sufficiently, you help us to review this Pull Request quicker and also help improve the quality of Release Notes

Checklist

1. Please provide specific title of the PR describing the change, including the component name (eg. „Update of Documentation link on Readme.md“)
2. Please provide related links (eg. Issue which will be closed by this Pull Request)
3. Please update relevant Documentation if applicable
4. Please check Contributing guide

This entire section above can be deleted if all items are checked.

---

Description of Change

Please describe your proposed Pull Request and it's impact.

Tests scenarios

Please describe on what Hardware and Software combinations you have tested this Pull Request and how.

Tested this on ESP32 dev board with Arduino 1 and 2 IDE

Related links

Please provide links to related issue, PRs etc.

(eg. Closes #number of issue)

[TD-er]

This is highly inconsistent.
See:

arduino-esp32/libraries/WiFi/src/WiFiClient.cpp

Lines 318 to 334 in 5b1a869

	int WiFiClient::setTimeout(uint32_t seconds)
	{
	Client::setTimeout(seconds * 1000); // This should be here?
	_timeout = seconds * 1000;
	if(fd() >= 0) {
	struct timeval tv;
	tv.tv_sec = seconds;
	tv.tv_usec = 0;
	if(setSocketOption(SO_RCVTIMEO, (char *)&tv, sizeof(struct timeval)) < 0) {
	return -1;
	}
	return setSocketOption(SO_SNDTIMEO, (char *)&tv, sizeof(struct timeval));
	}
	else {
	return 0;
	}
	}

So if you give a timeout during the connect phase (in msec), the behavior differs from when you call the setTimeout function.

As I mentioned before (this is not directed at @rtpmsys ) the timeout needs to be in msec, not in sec.
Cast it to its parent's class and then call the setTimeout function and that one expects a timeout in msec.

This is obviously a wrong design choice made in the past and it should be corrected.
Like I said before, it will likely fix implementations instead of breaking them.

[Jason2866]

There is #6676

[rtpmsys]

There is #6676

#6676 seems to deal with timeouts during receiving and sending. My change doesn't affect those, it's only a correcting of the timeout in the connect function, which is in milliseconds like it should be.

[Jason2866]

Still your PR is inconsistent and makes the time settings even more worse.

[rtpmsys]

Still your PR is inconsistent and makes the time settings even more worse.

I would say it's consistent with what the programmer expects. E.g. when giving a connect timeout of 1500ms, I would expect the connect to time out after 1.5 seconds and not after 1 second, and giving a timeout of 250ms, I would expect the connect function return after 0.25 Seconds and not after 6ms with a failure because the value given in ms gets truncated to zero.
Again, I understand the grief about the setTimeout function, but this change doesn't touch or affect it.

[mrengineer7777]

I have to agree with @rtpmsys here. This PR only fixes the milliseconds portion of the timeout on connection. For clarity here I would also rename "timeout" ->"timeout_ms" in both c and h files.

The change to WiFiClient::connect() should affect WiFiClient::setTimeout() as well, but the parameter there is currently seconds, so the ms portion is always 0 anyways. #6676 will fix the parameter on Client::setTimeout(), targeted for the 3.0.0 branch. It also resolves the inconsistency with WiFiClient.setTimeout() by deleting it and using the timeout on the Stream.

Note: renaming "_timeout" -> "_timeout_ms" here (note the underscore) would help clarity, but conflict with PR 6676.

At the very least I would merge this PR as written.

[SuGlider]

@rtpmsys @TD-er @Jason2866 @mrengineer7777

Thank you @rtpmsys for the PR. I see your point and reason for this change.
Thank you @TD-er @Jason2866 @mrengineer7777 for the review and bringing up the PR #6676 to this debate.

Indeed, there is a problem regarding Timeout, if it should be in seconds or milliseconds.

I'd like to propose another alternative that may keep back compatibility and add the timeout in milliseconds to the implementation.
I'd like to hear your opinion and analysis about it.

// 0.5 second, 2.0 seconds, 1.25 seconds, or any other float value in seconds... 
// the idea is to keep the API as it was, backcompatible, in seconds.

int WiFiClient::connect(IPAddress ip, uint16_t port, unsigned float timeout)    
{
   _timeout = (unsigned long) timeout;
    ....
    tv.tv_sec = _timeout;
    tv.tv_usec = (long) timeout * 1000;
}

It would cover the overload with uint32_t seconds given that C already promotes int to float automatically.

Another thing that I will do is to understand exactly where and how tv_sec and tv_usec are used in order to avoid collateral issues.
But for now, let's think about the approach above.

[SuGlider]

Let's talk more about it...

[TD-er]

At the very least, make the variable name to clearly indicate the intended unit.

A timeout in microcontroller land has an unwritten default unit of msec.
So seconds and usec deviate from this default and thus should be clearly mentioned.

And as I mentioned before (not sure if in this thread), I don't know if we actually break compatibility when changing it into msec units, as it will probably actually fix implementations.
When calling the setTimeout() function of the base class, it is already msec.
Thus the proposed fix will actually only allow those who know about this issue to work around something that's clearly a badly chosen unit in the first place.

If -and only if- there will not be a change of the unit of this parameter, then I guess using a float is probably the best option here. As long as this clearly indicates the unit and while we're at it, also change the variable names of inherited classes of this class to use _ms or something similar as part of the variable name.
Maybe even add some comment to the function declaration indicating it has to be in seconds.

[rtpmsys]

I sense some confusion about the issue at hand: Your proposal would change the expected timeout value given to the connect function into seconds. This would make it the same as the timeout value used elsewhere in the read and write functions. BUT the connect function CURRENTLY expects the timeout value in milliseconds. My change only corrects the mistake that the given value is not correctly used inside the connect function. My change doesn’t change the interface of the connect function at all - and conforms to the c sockets library and general conventions where timeouts are given in milliseconds. Your proposed change into a float and into accepting seconds instead of milliseconds would severely affect existing code. Unfortunately, bringing up #6676 seems to have confused the issue. That timeout deals with read/write timeouts which are a whole different topic and have nothing to do with the connect timeout. If we wanted to make things better, later, we would change the read/write timeout into milliseconds too, but this PR has nothing to do with that.   On Jan 28, 2023, at 9:32 PM, Rodrigo Garcia ***@***.***> wrote: @rtpmsys @TD-er @Jason2866 @mrengineer7777 Thank you @rtpmsys for the PR. I see your point and reason for this change. Thank you @TD-er @Jason2866 @mrengineer7777 for the review and bringing up the PR #6676 to this debate. Indeed, there is a problem regarding Timeout, if it should be in seconds or milliseconds. I's like to propose another alternative that may keep back compatibility and add the timeout in milliseconds to the implementation. I's like to hear your opinion and analysis about it. // 0.5 second, 2.0 seconds, 1.25 seconds, or any other float value in seconds... // the idea is to keep the API as it was, backcompatible, in seconds. int WiFiClient::connect(IPAddress ip, uint16_t port, unsigned float timeout) { _timeout = (unsigned long) timeout; .... tv.tv_sec = _timeout; tv.tv_usec = (long) timeout * 1000; } Another thing that I will do is to understand exactly where and how tv_sec and tv_usec are used in order to avoid collateral issues. But for now, let's think about the approach above. — Reply to this email directly, view it on GitHub <#7686 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZKDAJX6UZFNEDT45MJD6HTWUV64VANCNFSM6AAAAAATVOHTDY> . You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AZKDAJW4QGVVVD4B4D3NIADWUV64VA5CNFSM6AAAAAATVOHTD2WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTST4RY6W.gif> Message ID: ***@***.***>

[SuGlider]

There is one single line that makes it a problem:

_timeout = timeout;

_timeout comes from Stream Class and it is expressed in seconds...
I think that we may need to review it in the whole WiFi code.

[SuGlider]

There is one single line that makes it a problem:

_timeout = timeout;

_timeout comes from Stream Class and it is expressed in seconds... I think that we may need to review it in the whole WiFi code.

Nope... _timeout is expressed in milliseconds. My bad.

[SuGlider]

@rtpmsys @TD-er @Jason2866 @mrengineer7777
It took me a while to read all the code and understand a bit better all contributions.
Sorry guys for the confusion I have added in my previous commentaries.
Thank you all for the contribution to the PR and its review.

@rtpmsys
tv.tv_usec is mesaured in Microseconds.
int WiFiClient::connect(IPAddress ip, uint16_t port, int32_t timeout) - here, timeout is measured in Milliseconds.
The change to tv.tv_usec is local to the function and it affects the Socket Connection timeout only.
thus, your PR is Correct and it is Consistent.

@TD-er
int WiFiClient::setTimeout(uint32_t seconds) - indeed, it sets the timeout in seconds.
This is consistent within the code of this specific function.

int WiFiClient::setTimeout(uint32_t seconds) 
 { 
     Client::setTimeout(seconds * 1000); // This should be here? 
     _timeout = seconds * 1000;    // <<<================ Stream::_timeout in milliseconds 
     if(fd() >= 0) { 
         struct timeval tv; 
         tv.tv_sec = seconds;      // <<<================ local `struct timeval tv` used to change the socket options
         tv.tv_usec = 0;           // <<<================ tv in seconds is the Tx/Rx Socket Timeout

@Jason2866
PR #6676 doesn't affect the proposed change in this PR.

Therefore, the proposed change is correct.

PR Approved.

[SuGlider]

@rtpmsys - Thanks for the PR!
LGTM.

[SuGlider]

For clarity here I would also rename "timeout" ->"timeout_ms" in both c and h files.

Thanks @mrengineer7777, this is important too.
I added this change to the PR.

[mrengineer7777]

LGTM