New feature: send/recv message implementation added to network stack #14847

tymoteuszblochmobica · 2021-06-30T19:18:58Z

Summary of changes

Mbed doesn't support ancillary data sending receive during sendto / recvfrom.
New interface added to Network interface, LWIP Stack implementation and necessary stubs for Cellular and Nanostack.

Impact of changes

Need for rebuild libraries and applications using netsocket.

Migration actions required

None

Documentation

None

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@pan-
@kjbracey-arm

ciarmcom · 2021-06-30T19:30:16Z

@tymoteuszblochmobica, thank you for your changes.
@pan- @ARMmbed/mbed-os-connectivity @ARMmbed/mbed-os-mesh @ARMmbed/mbed-os-maintainers please review.

pan-

Thanks for the PR, I think it would be good to update all stacks implementation so their sendto/recvfrom calls into sendmsg/recvmsg. That would allow the socket level to do the same.

I think a helper to build the control options will be needed to but it might not be necessary yet as just pkt_info is available.

connectivity/cellular/include/cellular/framework/AT/AT_CellularStack.h

connectivity/lwipstack/include/lwipstack/LWIPStack.h

connectivity/lwipstack/include/lwipstack/lwipopts.h

connectivity/lwipstack/source/LWIPStack.cpp

connectivity/netsocket/include/netsocket/MsgHeader.h

pan- · 2021-06-30T20:22:51Z

connectivity/netsocket/include/netsocket/nsapi_types.h

+typedef struct nsapi_pktinfo {
+    nsapi_msghdr_t hdr;         /* Header identifying the message control structure */
+    nsapi_addr_t   ipi_addr;    /* Address associated with the packet */
+    int            ipi_ifindex; /* Interface associated with the packet */


This will return the internal interface index. I believe we should return/accept a pointer to a NetworkInterface. Some translation must be done in the NetworkStack implementation to make that possible.

I believe this chunk of API is currently C... That might be rather intractible.

For starters, we do need some way of mapping IDs to/from NetworkInterfaces. I think that is currently missing - we've hit this before. A comparator is the multicast group membership sockopt.

connectivity/netsocket/include/netsocket/nsapi_types.h

connectivity/netsocket/source/InternetDatagramSocket.cpp

Pull request has been modified.

kjbracey · 2021-07-01T11:31:09Z

connectivity/netsocket/include/netsocket/TCPSocket.h

@@ -145,6 +145,14 @@ class TCPSocket : public InternetSocket {
    nsapi_size_or_error_t recvfrom(SocketAddress *address,
                                   void *data, nsapi_size_t size) override;

+    virtual nsapi_size_or_error_t sendmsg(const SocketAddress &address,


Existing code (and recommended style) omits virtual with override.

kjbracey · 2021-07-01T11:35:43Z

connectivity/netsocket/include/netsocket/nsapi_types.h

+typedef struct nsapi_pktinfo {
+    nsapi_msghdr_t hdr;         /* Header identifying the message control structure */
+    nsapi_addr_t   ipi_addr;    /* Address associated with the packet */
+    int            ipi_ifindex; /* Interface associated with the packet */


I believe this chunk of API is currently C... That might be rather intractible.

For starters, we do need some way of mapping IDs to/from NetworkInterfaces. I think that is currently missing - we've hit this before. A comparator is the multicast group membership sockopt.

connectivity/netsocket/source/InternetDatagramSocket.cpp

kjbracey · 2021-07-01T11:46:08Z

connectivity/netsocket/include/netsocket/NetworkStack.h

+     *  @return         Number of sent bytes on success, negative error
+     *                  code on failure
+     */
+    virtual nsapi_size_or_error_t socket_sendmsg(nsapi_socket_t handle, const SocketAddress &address,


This isn't terribly close to POSIX sendmsg, it's an API halfway house between sendto and sendmsg. Maybe it should have a different name like sendto_ancillary?

I agree, it's between sendto and sendmsg. I don't think we want a full blown sendmsg/recvmsg as it requires an extra buffer if there is more than one buffer submitted in the iovec field. Adding ancillary would talk to the one that are used to the POSIX API but for newcomers I always find it confusing. These are control flags, not just random additional/meta data.

On the other hand I'm not sure what a good name would be.

Yep, the iovec is nice-to-have occasionally, but implementing it as emulation on top of non-vector stuff is counter-productive. Most underlying stacks will not support it.

Maybe _control makes sense, as that is the actual name of the field in the struct msghdr.

kjbracey · 2021-07-01T11:47:59Z

connectivity/netsocket/include/netsocket/NetworkStack.h

+    virtual nsapi_size_or_error_t socket_recvmsg(nsapi_socket_t handle, SocketAddress *address,
+                                                 void *data, nsapi_size_t size,
+                                                 nsapi_msghdr_t *control, nsapi_size_t control_size) = 0;
+


This could have a default implementation that passes onto recvfrom if control is NULL, otherwise returns "not supported".

Indeed, I think a default implementation is needed to not be a breaking API change for all existing NetworkStack implementors. Their stacks (including all those little Wifi modules) won't compile.

connectivity/lwipstack/include/lwipstack/LWIPStack.h

connectivity/netsocket/include/netsocket/InternetDatagramSocket.h

connectivity/netsocket/include/netsocket/Socket.h

connectivity/netsocket/include/netsocket/TCPSocket.h

mergify · 2021-07-08T22:36:57Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

ciarmcom · 2021-07-22T15:50:40Z

This pull request has automatically been marked as stale because it has had no recent activity. @tymoteuszblochmobica, please carry out any necessary work to get the changes merged. Thank you for your contributions.

0xc0170 · 2021-08-12T14:49:45Z

@tymoteuszblochmobica Travis failed - unit tests. Please review

@pan- Can you review after the last update?

pan- · 2021-08-19T22:04:24Z

@0xc0170 I'm continuing this PR from here.

mergify · 2021-09-09T07:36:06Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

0xc0170 · 2021-09-09T07:41:04Z

#15040 merged, closing this one.

ciarmcom added the release-type: feature label Jun 30, 2021

ciarmcom requested review from pan- and a team June 30, 2021 19:30

ciarmcom added needs: review component: connectivity component: mesh labels Jun 30, 2021

mergify bot added needs: work and removed needs: review labels Jun 30, 2021

pan- previously requested changes Jun 30, 2021

View reviewed changes

tymoteuszblochmobica force-pushed the SendRecvMessage branch from e34f63f to 8fd9671 Compare June 30, 2021 21:06

tymoteuszblochmobica force-pushed the SendRecvMessage branch from 8fd9671 to 2c59834 Compare June 30, 2021 21:31

kjbracey reviewed Jul 1, 2021

View reviewed changes

tymoteuszblochmobica force-pushed the SendRecvMessage branch 3 times, most recently from 600839b to a97b2e5 Compare July 5, 2021 12:07

pan- reviewed Jul 5, 2021

View reviewed changes

tymoteuszblochmobica force-pushed the SendRecvMessage branch 2 times, most recently from 5214725 to 8b4d3f1 Compare July 8, 2021 22:36

tymoteuszblochmobica force-pushed the SendRecvMessage branch 2 times, most recently from e76a54e to acbb32e Compare July 12, 2021 11:53

0xc0170 added needs: review and removed needs: work labels Jul 12, 2021

mergify bot added needs: work and removed needs: review labels Jul 12, 2021

tymoteuszblochmobica force-pushed the SendRecvMessage branch from acbb32e to 3a1ec8a Compare July 12, 2021 15:15

New feature: send/recv message implementation added to network stack

0a9b3c7

tymoteuszblochmobica force-pushed the SendRecvMessage branch from 3a1ec8a to 0a9b3c7 Compare July 12, 2021 16:33

ciarmcom added the stale Stale Pull Request label Jul 22, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jul 23, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Aug 1, 2021

ciarmcom added the stale Stale Pull Request label Aug 8, 2021

pan- mentioned this pull request Aug 10, 2021

Chip uses new Mbed ARMmbed/connectedhomeip#179

Merged

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Aug 13, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Aug 20, 2021

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Aug 27, 2021

mat-kalinowski mentioned this pull request Sep 1, 2021

New feature: send/recv message implementation added to network stack #15040

Merged

ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Sep 3, 2021

0xc0170 closed this Sep 9, 2021

mergify bot removed needs: work component: connectivity release-type: feature stale Stale Pull Request component: mesh labels Sep 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature: send/recv message implementation added to network stack #14847

New feature: send/recv message implementation added to network stack #14847

tymoteuszblochmobica commented Jun 30, 2021

ciarmcom commented Jun 30, 2021

pan- left a comment

pan- Jun 30, 2021

kjbracey Jul 1, 2021

tymoteuszblochmobica Jul 8, 2021

kjbracey Jul 1, 2021

tymoteuszblochmobica Jul 8, 2021

kjbracey Jul 1, 2021

kjbracey Jul 1, 2021

pan- Jul 5, 2021

kjbracey Jul 6, 2021

kjbracey Jul 1, 2021

mergify bot commented Jul 8, 2021

ciarmcom commented Jul 22, 2021

0xc0170 commented Aug 12, 2021

pan- commented Aug 19, 2021

mergify bot commented Sep 9, 2021

0xc0170 commented Sep 9, 2021

New feature: send/recv message implementation added to network stack #14847

New feature: send/recv message implementation added to network stack #14847

Conversation

tymoteuszblochmobica commented Jun 30, 2021

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

@pan- @kjbracey-arm

ciarmcom commented Jun 30, 2021

pan- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jul 8, 2021

ciarmcom commented Jul 22, 2021

0xc0170 commented Aug 12, 2021

pan- commented Aug 19, 2021

mergify bot commented Sep 9, 2021

0xc0170 commented Sep 9, 2021

@pan-
@kjbracey-arm