search

u-blox / ubxlib

Portable C libraries which provide APIs to build applications with u-blox products and services. Delivered as add-on to existing microcontroller and RTOS SDKs.
Apache License 2.0
287 stars 82 forks source link

uCellSockCleanup #217

Closed eeFLis closed 3 months ago

eeFLis commented 3 months ago

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 commented 3 months ago

Hi there. Yes, I think you are right, I had misinterpreted my own comment above uCellSockCleanup(): https://github.com/u-blox/ubxlib/blob/1b94ec51334a5a326328341ad1e5a078692c2f46/cell/api/u_cell_sock.h#L193-L200 ...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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

Sounds good. That would help us

RobMeades commented 3 months ago

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 commented 3 months ago

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

RobMeades commented 3 months ago

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

RobMeades commented 3 months ago

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 commented 3 months ago

looks good. thanks

RobMeades commented 3 months ago

The addition of uSockForgetAll() is now on master here, see commit 857e2a6641d747302bc0bcdbce9930965e99b65e. 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.