esp8266 / Arduino

ESP8266 core for Arduino
GNU Lesser General Public License v2.1
16.09k stars 13.33k forks source link

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

Closed RobertGnz closed 10 months ago

RobertGnz commented 10 months ago

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

mcspr commented 10 months ago

WiFiClient is Stream, so both Stream::setTimeout and Stream::getTimeout are used and this actually hides both. (and with different signatures, too) https://github.com/esp8266/Arduino/blob/b3b9276bf95c4fb3bc27d6c074fa45bf4c68eed6/cores/esp8266/Stream.h#L68-L69

Aren't these enough? Timeout value is used, just not immediately https://github.com/esp8266/Arduino/blob/b3b9276bf95c4fb3bc27d6c074fa45bf4c68eed6/libraries/ESP8266WiFi/src/WiFiClient.cpp#L163 https://github.com/esp8266/Arduino/blob/b3b9276bf95c4fb3bc27d6c074fa45bf4c68eed6/libraries/ESP8266WiFi/src/WiFiClient.cpp#L219 https://github.com/esp8266/Arduino/blob/b3b9276bf95c4fb3bc27d6c074fa45bf4c68eed6/libraries/ESP8266WiFi/src/WiFiClient.cpp#L241

JAndrassy commented 10 months ago

I started a discussion about this, but there were no responses https://github.com/esp8266/Arduino/discussions/9004

JAndrassy commented 10 months ago

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 commented 10 months ago

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.

mcspr commented 10 months ago

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 commented 10 months ago

the same value of _timeout

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

mcspr commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

https://github.com/esp8266/Arduino/blob/b3b9276bf95c4fb3bc27d6c074fa45bf4c68eed6/cores/esp8266/Stream.h#L68-L69

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

https://github.com/esp8266/Arduino/blob/b3b9276bf95c4fb3bc27d6c074fa45bf4c68eed6/libraries/ESP8266WiFi/src/WiFiClient.cpp#L163

https://github.com/esp8266/Arduino/blob/b3b9276bf95c4fb3bc27d6c074fa45bf4c68eed6/libraries/ESP8266WiFi/src/WiFiClient.cpp#L219

https://github.com/esp8266/Arduino/blob/b3b9276bf95c4fb3bc27d6c074fa45bf4c68eed6/libraries/ESP8266WiFi/src/WiFiClient.cpp#L241

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 commented 10 months ago

8899 has yet to be finished

RobertGnz commented 10 months ago

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 ?