eclipse-threadx / netxduo

Eclipse ThreadX - NetXDuo is an advanced, industrial-grade TCP/IP network stack designed specifically for deeply embedded real-time and IoT applications
https://github.com/eclipse-threadx/rtos-docs/blob/main/rtos-docs/netx-duo/index.md
MIT License
230 stars 131 forks source link

_nxe_tcp_socket_receive_notify invalid input pointer check breaks documented functionality. #139

Closed timp-exi closed 1 year ago

timp-exi commented 1 year ago

https://github.com/azure-rtos/netxduo/blob/846932ee7b379edee9f87627f4d23cb995da4cde/common/src/nxe_tcp_socket_receive_notify.c#L92-L96

As described in the NetX documentation for nx_tcp_socket_receive_notify:

"This service configures the receive notify function pointer with the callback function specified by the application. This callback function is then called whenever one or more packets are received on the socket. If a NX_NULL pointer is supplied, the notify function is disabled."

With the code as it stands, if you run without defining the macro "NX_DISABLE_ERROR_CHECKING" then the error checking function _nxe_tcp_socket_receive_notify will break the documented functionality. Effectively at present you cannot disable the socket receive notify callback if error checking is enabled!

The lines 92 - 96 from _nxe_tcp_socket_receive_notify.c should be removed. (Note in the NetX repo the code is correct for this function. Seems to only be an issue in NetxDuo).

TiejunMS commented 1 year ago

@timp-exi, thanks for reaching out! This function is designed to set callback function but not clear it. So NX_NULL is not allowed. We will keep as it is now. You are free to modify it in your own project.

timp-exi commented 1 year ago

Hi, thanks for your quick response but with all due respect in this case you are mistaken.

If you check the documentation for NetX Duo:

https://learn.microsoft.com/en-us/azure/rtos/netx-duo/chapter4#nx_tcp_socket_receive_notify

It specifically states that the nx_tcp_socket_receive_notify function is used both to set and clear the callback function:

"This service configures the receive notify function pointer with the callback function specified by the application. This callback function is then called whenever one or more packets are received on the socket. If a NX_NULL pointer is supplied, the notify function is disabled."

It makes no sense that the error checking wrapper to this function (nxe_tcp_socket_receive_notify) blocks this functionality. It makes even less sense knowing that this behaviour also contradicts the current NetX implementation ( https://github.com/azure-rtos/netx/blob/master/common/src/nxe_tcp_socket_receive_notify.c ).

So either:

or

I would kindly ask that you please take the time to review this again and reconsider your decision.

TiejunMS commented 1 year ago

Good catch! We will honor what user guide describes and remove this check in _nxe_xxx function. Many thanks!

timp-exi commented 1 year ago

That's great, thanks :)

TiejunMS commented 1 year ago

Fixed by commit https://github.com/azure-rtos/netxduo/commit/2973652d801e92615caa285ad9ee13db19a26cb9