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

enforcing stop() when calling connect a second time #949

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

andreagilardoni
Copy link
Contributor

This PR aims to fix the issue for which the client is not able to connect again after the connection is lost. In this way we stop the connection every time we desire to connect to something.

}
}
// if a connection is aready ongoing, a disconnection must be enforced before starting another one
stop();

Choose a reason for hiding this comment

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

ok. I don't know the reason why the the socker->recv was there... Do you know why?
Maybe better to re-initialize always.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why it was there, but I suppose that if someone calls connect he wants to reset the connection before connecting, since I could connect to a new host

@@ -80,14 +80,8 @@ void arduino::MbedClient::configureSocket(Socket *_s) {
}

int arduino::MbedClient::connect(SocketAddress socketAddress) {

Choose a reason for hiding this comment

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

In connect and connectSSL I see a discrepancy in how timesout are set.
Can you please take a look and make them uniform?

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as stated here TLSSocketWrapper requires the timeout to be set before the connect is called. In the plain TCP version we call set_timeout before write, I think the reason for this is to update the timeout value internally if the user changes it. I would not touch it for the time being. @pennam can you confirm that?

/* For TLS connection timeout needs to be configured before handshake starts
* otherwise socket timeout is not adopted. See TLSSocketWrapper::set_timeout(int timeout)
*/
sock->set_timeout(_timeout);

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we can add a sock-> set_timeout() also in arduino::MbedClient::connect(SocketAddress socketAddress) just before nsapi_error_t returnCode = static_cast<TCPSocket *>(sock)->connect(socketAddress); since also TCPSocket::connect(const SocketAddress &address) is using the _timeout value, but i would do it in a separate PR.

@JAndrassy
Copy link
Contributor

JAndrassy commented Sep 27, 2024

in LwIP (underlying implementation of the Mbed networking) the timeout socket-option doesn't apply to connect. it is for read and write operations. (In Arduino read() should not block.)

@pennam
Copy link
Contributor

pennam commented Sep 27, 2024

in LwIP (underlying implementation of the Mbed networking) the timeout socket-option doesn't apply to connect. it is for read and write operations. (In Arduino read() should not block.)

so what this flag blocking_connect_in_progress is used for? https://github.com/arduino/mbed-os/blob/e0cad5c4277b3c5ee9cb01a9529df9333612bbeb/connectivity/netsocket/source/TCPSocket.cpp#L54

@JAndrassy
Copy link
Contributor

so what this flag blocking_connect_in_progress is used for?

by default connect is not blocking. repeated invocation of connect then can be used to test if it already connected. but the recommended way for LwIP is this way https://github.com/espressif/arduino-esp32/blob/b05f18dad55609ae2a569be81c7535021b880cf3/libraries/Network/src/NetworkClient.cpp#L251

I didn't see how to do it on Mbed networking API

@andreagilardoni
Copy link
Contributor Author

I think that this timeout setting issue is somehow related to this PR, but it is not addressing the same issue we are trying yo fix. Can we address this on another PR?

Copy link
Contributor

@pennam pennam left a comment

Choose a reason for hiding this comment

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

🚢

@pennam pennam merged commit 22e6d91 into arduino:main Oct 2, 2024
11 checks passed
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