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.

Sign up for GitHub

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

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

Merged
merged 13 commits into from
Dec 13, 2023
Merged

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

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5564372
Removed virtual + moved socketOptions ot read/write
P-R-O-C-H-Y May 2, 2022
12699eb
Removed no needed code + edit
P-R-O-C-H-Y May 2, 2022
c1183e4
removed Client::getTimeout
P-R-O-C-H-Y May 2, 2022
2f49062
removed setTimeout from WifiClient - read/write timeouts in construct…
P-R-O-C-H-Y May 2, 2022
e6e77ff
Changed seconds to miliseconds in other classes relaed + examples
P-R-O-C-H-Y May 2, 2022
b1193c2
Applied same changes for WifiClientSecure
P-R-O-C-H-Y May 2, 2022
6420e3a
Added 0 init values to constructor
P-R-O-C-H-Y May 2, 2022
bee59c1
Seconds are not rounded now
P-R-O-C-H-Y May 2, 2022
b986041
removed +500 for previous rounding
P-R-O-C-H-Y May 2, 2022
675484c
Merge branch 'master' into WifiClient_settimeout_conflict
P-R-O-C-H-Y Dec 13, 2023
77830cf
fix rebased code in WifiClientSecure
P-R-O-C-H-Y Dec 13, 2023
7d86e1d
Fix rebased code
P-R-O-C-H-Y Dec 13, 2023
0e1d13a
Fix rebase code
P-R-O-C-H-Y Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions libraries/HTTPClient/src/HTTPClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ void HTTPClient::setTimeout(uint16_t timeout)
{
_tcpTimeout = timeout;
if(connected()) {
_client->setTimeout((timeout + 500) / 1000);
_client->setTimeout(timeout);
}
}

Expand Down Expand Up @@ -1165,7 +1165,7 @@ bool HTTPClient::connect(void)
}

// set Timeout for WiFiClient and for Stream::readBytesUntil() and Stream::readStringUntil()
_client->setTimeout((_tcpTimeout + 500) / 1000);
_client->setTimeout(_tcpTimeout);

log_d(" connected to %s:%u", _host.c_str(), _port);

Expand Down
2 changes: 1 addition & 1 deletion libraries/HTTPUpdate/examples/httpUpdateSecure/httpUpdateSecure.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ void loop() {
client.setCACert(rootCACertificate);

// Reading data over SSL may be slow, use an adequate timeout
client.setTimeout(12000 / 1000); // timeout argument is defined in seconds for setTimeout
client.setTimeout(12000); // timeout argument is defined in miliseconds for setTimeout

// The line below is optional. It can be used to blink the LED on the board during flashing
// The LED will be on during download of one buffer of data from the network. The LED will
Expand Down
2 changes: 1 addition & 1 deletion libraries/WebServer/src/WebServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ void WebServer::handleClient() {
if (_parseRequest(_currentClient)) {
// because HTTP_MAX_SEND_WAIT is expressed in milliseconds,
// it must be divided by 1000
_currentClient.setTimeout(HTTP_MAX_SEND_WAIT / 1000);
_currentClient.setTimeout(HTTP_MAX_SEND_WAIT); /* / 1000 removed, WifiClient setTimeout changed to ms */
_contentLength = CONTENT_LENGTH_NOT_SET;
_handleRequest();

Expand Down
45 changes: 26 additions & 19 deletions libraries/WiFi/src/WiFiClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,8 @@ void WiFiClient::stop()
clientSocketHandle = NULL;
_rxBuffer = NULL;
_connected = false;
_lastReadTimeout = 0;
_lastWriteTimeout = 0;
}

int WiFiClient::connect(IPAddress ip, uint16_t port)
Expand Down Expand Up @@ -331,25 +333,6 @@ int WiFiClient::getSocketOption(int level, int option, const void* value, size_t
return res;
}


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

int WiFiClient::setOption(int option, int *value)
{
return setSocketOption(IPPROTO_TCP, option, (const void*)value, sizeof(int));
Expand Down Expand Up @@ -418,6 +401,18 @@ size_t WiFiClient::write(const uint8_t *buf, size_t size)
tv.tv_usec = WIFI_CLIENT_SELECT_TIMEOUT_US;
retry--;

if(_lastWriteTimeout != _timeout){
if(fd() >= 0){
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)
{
_lastWriteTimeout = _timeout;
}
}
}

if(select(socketFileDescriptor + 1, NULL, &set, NULL, &tv) < 0) {
return 0;
}
Expand Down Expand Up @@ -477,6 +472,18 @@ size_t WiFiClient::write(Stream &stream)

int WiFiClient::read(uint8_t *buf, size_t size)
{
if(_lastReadTimeout != _timeout){
if(fd() >= 0){
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)
Copy link
Contributor

@cziter15 cziter15 Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@TD-er TD-er Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@cziter15 cziter15 Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
_lastReadTimeout = _timeout;
}
}
}

int res = -1;
if (_rxBuffer) {
res = _rxBuffer->read(buf, size);
Expand Down
4 changes: 2 additions & 2 deletions libraries/WiFi/src/WiFiClient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ class ESPLwIPClient : public Client
public:
virtual int connect(IPAddress ip, uint16_t port, int32_t timeout) = 0;
virtual int connect(const char *host, uint16_t port, int32_t timeout) = 0;
virtual int setTimeout(uint32_t seconds) = 0;
};

class WiFiClient : public ESPLwIPClient
Expand All @@ -43,6 +42,8 @@ class WiFiClient : public ESPLwIPClient
std::shared_ptr<WiFiClientRxBuffer> _rxBuffer;
bool _connected;
int _timeout;
int _lastWriteTimeout;
int _lastReadTimeout;

public:
WiFiClient *next;
Expand Down Expand Up @@ -91,7 +92,6 @@ class WiFiClient : public ESPLwIPClient
int getSocketOption(int level, int option, const void* value, size_t size);
int setOption(int option, int *value);
int getOption(int option, int *value);
int setTimeout(uint32_t seconds);
int setNoDelay(bool nodelay);
bool getNoDelay();

Expand Down
27 changes: 26 additions & 1 deletion libraries/WiFiClientSecure/src/WiFiClientSecure.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ WiFiClientSecure::WiFiClientSecure(int sock)
{
_connected = false;
_timeout = 30000; // Same default as ssl_client
_lastReadTimeout = 0;
_lastWriteTimeout = 0;

sslclient = new sslclient_context;
ssl_init(sslclient);
Expand Down Expand Up @@ -94,6 +96,8 @@ void WiFiClientSecure::stop()
sslclient->socket = -1;
_connected = false;
_peek = -1;
_lastReadTimeout = 0;
_lastWriteTimeout = 0;
}
stop_ssl_socket(sslclient, _CA_cert, _cert, _private_key);
}
Expand Down Expand Up @@ -199,6 +203,16 @@ size_t WiFiClientSecure::write(const uint8_t *buf, size_t size)
if (!_connected) {
return 0;
}
if(_lastWriteTimeout != _timeout){
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
_lastWriteTimeout = _timeout;
}
}

int res = send_ssl_data(sslclient, buf, size);
if (res < 0) {
stop();
Expand All @@ -209,6 +223,18 @@ size_t WiFiClientSecure::write(const uint8_t *buf, size_t size)

int WiFiClientSecure::read(uint8_t *buf, size_t size)
{
if(_lastReadTimeout != _timeout){
if(fd() >= 0){
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
_lastReadTimeout = _timeout;
}
}
}

int peeked = 0;
int avail = available();
if ((!buf && size) || avail <= 0) {
Expand Down Expand Up @@ -396,4 +422,3 @@ int WiFiClientSecure::fd() const
{
return sslclient->socket;
}

1 change: 1 addition & 0 deletions libraries/WiFiClientSecure/src/WiFiClientSecure.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class WiFiClientSecure : public WiFiClient
bool getFingerprintSHA256(uint8_t sha256_result[32]) { return get_peer_fingerprint(sslclient, sha256_result); };
int setTimeout(uint32_t seconds);
int fd() const;
int setSocketOption(int option, char* value, size_t len);
P-R-O-C-H-Y marked this conversation as resolved.
Show resolved Hide resolved

operator bool()
{
Expand Down
Loading