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.

enforcing stop() when calling connect a second time

97b6f59

rjtokenring reviewed

Sep 12, 2024

View reviewed changes

libraries/SocketWrapper/src/MbedClient.cpp

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

Copy link

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

ContributorAuthor

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

rjtokenring reviewed

Sep 12, 2024

View reviewed changes

libraries/SocketWrapper/src/MbedClient.cpp

		_status =true;
		}

		intarduino::MbedClient::connect(SocketAddress socketAddress) {

Copy link

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

ContributorAuthor

Choose a reason for hiding this comment

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

as stated hereTLSSocketWrapper requires the timeout to be set before the connect is called. In the plain TCP version we callset_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?

ArduinoCore-mbed/libraries/SocketWrapper/src/MbedClient.cpp

Lines 154 to 157 in702daa0

	/* 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

pennamSep 27, 2024

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 asock-> set_timeout() also inarduino::MbedClient::connect(SocketAddress socketAddress) just beforensapi_error_t returnCode = static_cast<TCPSocket *>(sock)->connect(socketAddress); since alsoTCPSocket::connect(const SocketAddress &address) is using the_timeout value, but i would do it in a separate PR.

andreagilardoni requested a review frompennam

September 25, 2024 08:21

Copy link

Contributor

JAndrassy commentedSep 27, 2024•
edited
Loading

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.)

Copy link

Contributor

pennam commentedSep 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 flagblocking_connect_in_progress is used for?https://github.com/arduino/mbed-os/blob/e0cad5c4277b3c5ee9cb01a9529df9333612bbeb/connectivity/netsocket/source/TCPSocket.cpp#L54

Copy link

Contributor

JAndrassy commentedSep 27, 2024

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 wayhttps://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

Copy link

ContributorAuthor

andreagilardoni commentedSep 30, 2024

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?