Skip to content

Conversation

@me-no-dev
Copy link
Member

No description provided.

@me-no-dev me-no-dev merged commit 4ce2cc3 into master Sep 16, 2019
@me-no-dev me-no-dev deleted the ssl-fix branch September 16, 2019 16:14
@Jeroen88
Copy link
Contributor

@me-no-dev Why so difficult? Why where the ::connect() methods with timeout introduced in the first place? Wouldn't it be far better to call the parent class WiFiClient::setTimeout() method?

I did not have time to check it, but I think there is a nasty error in start_ssl_client() that might be the root cause of the problems. In this line and the next, not a pointer and a sizeof() an int should be passed, but a pointer and a sizeof a struct timeval. I think because the first element of the struct is a timeout in seconds, the error was never discovered...

struct timeval tv; tv.tv_sec = 0; tv.tv_usec = timeout * 1000; if(lwip_setsockopt(SO_RCVTIMEO, &tv, sizeof(struct timeval)) < 0) { return -1; } if(lwip_setsockopt(SO_SNDTIMEO, &tv, sizeof(struct timeval)) < 0) { return -1; } I

In the above code a timeout in milliseconds is set, but, since WiFiClient::setTimeout() is in seconds, I strongly believe (just because this method existed earlier than ::setHandshakeTimeout(), which is in milliseconds) that the connect() timeout parameter should be in seconds. But, even better: remove these methods and call the parent class setTimeout()

Also, because the timeouts are messy between WiFiClient, HTTPClient and the socket timeout that the timeout parameters should be renamed to show in what unit they are, so not int32_t timeout but int32_t timeout_milliseconds or something like that.

Also I think that this line could also better be replaced by the parent class WiFiClient::setNoDelay()

I did not test any of this, I did not even try to code it, so maybe I am completely wrong...

@me-no-dev
Copy link
Member Author

this will do for now :) rewrite of both clients is in order ;) Also is integrating with the official Arduino API. Timeouts are good ;) we need them but they can be implemented better

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.

3 participants