Skip to content
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

Add 2 functions to adjust timeout value in WiFiClient.h and WiFiClient.cpp #9073

Closed
wants to merge 2 commits into from

Conversation

RobertGnz
Copy link
Contributor

Default value of timeout is 5000 ms but currently it can't be changed in case the user think it is inadequate for its application.
For ESP32 we already have the possibility to specifie the timeout value as an argument of the connect function.
The aim of the request is to provide the possibilty to specifie a value for ESP8266 too.

Usage example:
int oldValue = getTimeout();
setTimeout( newValue );
client.connect( args );
setTimeout( oldValue );

WiFiClient.h:
// Default timeout is set to 5000 ms
// setTimeout gives the possibility to adjust this value.
// getTimeout return the timeout current value.
bool setTimeout( int );
int getTimeout();

WiFiClient.cpp:
// In case you need to increase/decrease timeout current value
bool WiFiClient::setTimeout( int timeout_ms )
{
if ( timeout_ms <= 0 || timeout_ms > 3600000 ) return(false); // More than 0 and less than 1 hour
_timeout = timeout_ms;
if (!_client) return(true);
else _client->setTimeout(_timeout);
return(true);
}

int WiFiClient::getTimeout()
{
return( _timeout );
}

Add setTimeout( int ) and int getTimeout
Add setTimeout and getTimeout to allow changes of timeout value.
@mcspr
Copy link
Collaborator

mcspr commented Jan 19, 2024

WiFiClient is Stream, so both Stream::setTimeout and Stream::getTimeout are used and this actually hides both. (and with different signatures, too)

void setTimeout(unsigned long timeout); // sets maximum milliseconds to wait for stream data, default is 1 second
unsigned long getTimeout () const { return _timeout; }

Aren't these enough? Timeout value is used, just not immediately

_client->setTimeout(_timeout);

_client->setTimeout(_timeout);

_client->setTimeout(_timeout);

@JAndrassy
Copy link
Contributor

I started a discussion about this, but there were no responses
#9004

@JAndrassy
Copy link
Contributor

But with this I don't see any possibility to set timeout before calling client.connect .

client.setTimeout(1000);
if (client.connect(host, port)) {

doesn't work?

@RobertGnz
Copy link
Contributor Author

RobertGnz commented Jan 19, 2024

But with this I don't see any possibility to set timeout before calling client.connect .

client.setTimeout(1000);
if (client.connect(host, port)) {

doesn't work?

Yes it works, I made a mistake. But seems only to work for WiFiClient.

@RobertGnz RobertGnz closed this Jan 19, 2024
@mcspr
Copy link
Collaborator

mcspr commented Jan 19, 2024

For ESP32 we already have the possibility to specifie the timeout value as an argument of the connect function.

(#9004) Other libraries have client.setConnectionTimeout. It was first used in the 2.0.0 version of the Ethernet library in 2018. There it is used in client.stop() too.

Looking at ESP32 and Ethernet, setConnectionTimeout modifies the same value of _timeout and not something separate :/ So... setTimeout() would still have to appear for I/O, just in a different place.

https://github.com/arduino-libraries/Ethernet/blob/39103da0e1bc569023625ee4693272773397dbb6/src/Ethernet.h#L242
https://github.com/espressif/arduino-esp32/blob/47666082ff30404fad8594e00001667e23785404/libraries/WiFi/src/WiFiClient.cpp#L360

ESP32 ::connect() also overwrites the member value, so I/O has to setTimeout() or connect() value would be used afterwards
https://github.com/espressif/arduino-esp32/blob/47666082ff30404fad8594e00001667e23785404/libraries/WiFi/src/WiFiClient.cpp#L222
(notably, default connect without timeout arg is _timeout and not some default value)

Read / write timeouts are sourced from _timeout, but forced by the impl to track previous values for setsockopt calls.

Our implementation in ClientContext::connect can use timeout arg immediately, since the only thing it is needed for is esp_delay(timeout, []() { ... waiting for lwip task flag ... });
(and for the disconnection, ::stop() already has the arg)

@JAndrassy
Copy link
Contributor

the same value of _timeout

EthernetClient and WiFiClient in esp32 have own _timeout field. it is a different variable than _timeout in Stream

@mcspr
Copy link
Collaborator

mcspr commented Jan 19, 2024

the same value of _timeout

EthernetClient and WiFiClient in esp32 have own _timeout field. it is a different variable than _timeout in Stream

Badly worded. ESP32 shares the timeout member within the client class, similar to the way we (ab)use Stream::_timeout

In case of ESP8266/Ethernet I just noticed an inconsistency, as it seems to only affect connect() and stop(). No mention of timeouts in libraries/LwIP_, it is never used in any hw operations.
Stream::_timeout continues to be used for I/O, though, and thus by our generic send(Stream
)

@JAndrassy
Copy link
Contributor

JAndrassy commented Jan 19, 2024

ESP32 shares the timeout member within the client class, similar to the way we (ab)use Stream::_timeout

but Stream's blocking methods have they own separate timeout with a different value

it seems to only affect connect() and stop()

that is why it is set with setConnectionTimeout()

it is never used in any hw operations.

and that is OK. read() and write() should not block in Arduino.

@RobertGnz
Copy link
Contributor Author

But with this I don't see any possibility to set timeout before calling client.connect .

client.setTimeout(1000);
if (client.connect(host, port)) {

doesn't work?

Unfortunately it does not work with a WiFiClientSecure. You can put whatever you want, it will not have any effect on _timeout value when using connect.

@RobertGnz
Copy link
Contributor Author

WiFiClient is Stream, so both Stream::setTimeout and Stream::getTimeout are used and this actually hides both. (and with different signatures, too)

void setTimeout(unsigned long timeout); // sets maximum milliseconds to wait for stream data, default is 1 second
unsigned long getTimeout () const { return _timeout; }

Aren't these enough? Timeout value is used, just not immediately

_client->setTimeout(_timeout);

_client->setTimeout(_timeout);

_client->setTimeout(_timeout);

Unfortunately it does not work with a WiFiClientSecure. You can put whatever you want, it will not have any effect on _timeout value when using connect method.
Did I missed something ?

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 19, 2024

#8899 has yet to be finished

@RobertGnz
Copy link
Contributor Author

#8899 has yet to be finished

Thanks for your support.
In fact my problem is not to decrease but to increase timeout but just for the step connect of a WiFiClientSecure. I use it to connect to an already quite loaded SMTP server through a 4G router. So the SSL handshake is sometimes very long and the connect method may sometimes abort too early. Would you have any suggestion for that kind of problem ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants