uCellSockCleanup

[eeFLis]

Hi there

We see a potential problem in uCellSockCleanup which was introduced in commit 3ffd36f.
In uSockCleanUp all URCs are removed if at least one socket is closed.
As a result, uCellSockGetBytesPending() no longer works for sockets that are still open.

as an example:

1. We open a UDP socket and check with uCellSockGetBytesPending whether new data is available.
2. we open another TCP socket and transfer some data.
3. the TCP socket is closed.
4. uCellSockGetBytesPending will always return zero because the +UUSORD URC has been removed.

Does that make sense or am I wrong?

[RobMeades]

Hi there. Yes, I think you are right, I had misinterpreted my own comment above uCellSockCleanup():

ubxlib/cell/api/u_cell_sock.h

Lines 193 to 200 in 1b94ec5

	/** Clean-up. This function should be called when
	* there is no socket activity, either locally or from
	* the remote host, in order to free memory occupied
	* by closed sockets.
	*
	* @param cellHandle the handle of the cellular instance.
	*/
	void uCellSockCleanup(uDeviceHandle_t cellHandle);

...to mean that all sockets had been closed, which happens to be how we use it in our test cases, but of course that's not what it says, it should clean-up only unused sockets-related resources.

Let me try to figure out why I didn't put this in uCellSockDeinit() in the first place.

[RobMeades]

You may find a proposed fix in a preview branch here:

https://github.com/u-blox/ubxlib/tree/preview_fix_sock_clean_up_rmea

This passes our testing, but so did the original code, so please give it some through checking at your end; I am now just removing all of the URCs in uCellSockDeinit(), just concerned that there was a [potentially thread-safety/race-condition related?] reason why I wasn't doing that before which I can no longer recall.

When you have confirmed that you are happy I will merge the change, push it out to master here and delete the preview branch some time after that.

[eeFLis]

Hi Rob

looks good thank you.

another question regarding socket celanup:
What do you think about having a function that forces the socketcleanup.
After the module has switched to PSM, we are sure that all sockets are closed.

The reason why we do not close the socket beforehand is that we do not know exactly when the module will switch to PSM.
However, data can still be received from the socket as long as the module has not switched to PSM.

The second reason is that uSockClose would wake up the module from power save for another 6 seconds.

[philwareublox]

Hi,

You are correct, when the module goes into PSM sockets are closed.
Waking up from PSM you must recreate the sockets and connect to the servers (TCP).

Regards,
Phil.

[eeFLis]

Hi
Thank you for your quick reply.

That's how we do it. But at some point the closed UDP sockets must be cleaned up in ubxlib.
Because the ubxlib does not recognise that the sockets were closed when the module switched to PSM.

[RobMeades]

Hmmm, yes, subtle. I was going to say that uSockDeinit() should do that but of course that would talk to the module, which you do not want.

So maybe uSockForgetAll()? This would only do a ubxlib-cleanup of socket resources, for all sockets, it would not deinitialise the sockets layer (i.e. URC handlers would remain loaded), it would not talk to the module at all. I would explain in the function header that the use-case for this is the returning-from-3GPP-sleep case.

[eeFLis]

Sounds good. That would help us

[RobMeades]

OK, I will do that as a separate change. Have you been able to test that the fix for the clean-up issue is good? If so I will get it reviewed and pushed back to master here.

[eeFLis]

Yes, it has now been running for several hours without any problems. I think it is OK. Thank you

[RobMeades]

Fix for issue that was the subject of the original post now pushed to 3335108 here. Will do the "uSockForgetAll()" change next and post a preview branch for that here.

[RobMeades]

Find here:

https://github.com/u-blox/ubxlib/tree/preview_feature_sock_forget_rmea

...a preview branch of the proposed uSockForgetAll(). Let me know if it meets your expectations, or feel free to suggest any improvements.

[eeFLis]

looks good. thanks

[RobMeades]

The addition of uSockForgetAll() is now on master here, see commit 857e2a6. I will close this issue now [please feel free to re-open if there is more to discuss on this topic] and will delete the preview_fix_sock_clean_up_rmea and preview_feature_sock_forget_rmea preview branches some time next week.