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
303 stars 92 forks source link

uSockRead should return if a socket is closed by remote #168

Closed arnoutdekimo closed 8 months ago

arnoutdekimo commented 10 months ago

Hi,

Just a comment on the workings of the function uSockRead. If we set a read timeout of 10s (uSockOptionSet(sock, U_SOCK_OPT_LEVEL_SOCK, U_SOCK_OPT_RCVTIMEO), and poll uSockRead in a loop, we expected indeed after 10s of no data received, that the functions returns.

However, if the socket is closed within that time, the module does report this:

0:00:37:859 - OK 0:00:37:859 - 0:00:37:915 - +UUSOCL: 0 0:00:37:915 - 0:00:37:915 - OK 0:00:37:916 - 0:00:37:916 - +UUSOCL: 0 0:00:37:985 - AT+USORD=0,0 0:00:38:036 - 0:00:38:073 - +CME ERROR: Operation not allowed 0:00:38:074 - 0:00:38:074 - AT+USORD=0,0 0:00:38:191 - 0:00:38:231 - +CME ERROR: Operation not allowed 0:00:38:232 - 0:00:38:232 - AT+USORD=0,0 0:00:38:349 - 0:00:38:389 - +CME ERROR: Operation not allowed 0:00:38:390 - 0:00:38:390 - AT+USORD=0,0 0:00:38:507 -

But ubxlib does not use this information and keeps blocking for the timeout setting. (loop in receivefunction)

Ideally, a posix socket would directly return in this case, indicating that the connection has closed.

It's possible to work around this behavior, but just letting it know that it is unexpected.

RobMeades commented 10 months ago

Yes, I've wondered about capturing this and reporting it somehow, I will add it to the TODO list.

arnoutdekimo commented 10 months ago

(FYI, my current workaround is using small socket timeouts (using my own big one externally) + uSockRegisterCallbackClosed, and letting it set a boolean, so I can check after uSockRead returns -1 whether the socket is still alive or not. )

RobMeades commented 8 months ago

Hi Arnout: I've pushed a preview branch of the change here https://github.com/u-blox/ubxlib/commit/4d29ef0f4a5195dc5f43b7a312582620952c2463. Is that the kind of change you were expecting? If so I'll push it to master here (then delete the preview branch).

arnoutdekimo commented 8 months ago

Hi, yup, sounds like what was missing, thanks!

RobMeades commented 8 months ago

The fix is now pushed to master here in commit c315b4ffad17e6581185e9234cbe86263ce90275; I will delete the preview branch.