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

mat-kalinowski · 2021-09-01T09:05:26Z

Summary of changes

This PR is continuation of another stale PR: #14847. I am opening this one, as I didn't have access to push to the fork created by other developer.

I have added few changes related to unresolved comments:

function names were changed from socket_sendmsg/socket_recvmsg to socket_sendto_control/socket_recvfrom_control.
default implementation of this functions was provided in the NetworkStack class to not break the existing API. This was the issue that caused tests to fail in the previous PR.
MsgHeaderIterator accesses elements on the aligned addresses

.

Impact of changes

Migration actions required

Documentation

Stale PR with this feature:
#14847

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-

ciarmcom · 2021-09-01T09:30:15Z

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

0xc0170 · 2021-09-01T12:36:00Z

If you rebase to the latest master, license check should stop failing with python error. Also review styling check - code needs fixing.

mat-kalinowski · 2021-09-02T09:51:46Z

I have added some changes to solve the bugs present in the unit tests:

default argument added to the function add_ethernet_interface was not included in the gmock's MOCK_METHOD. I have added wrapper function to solve this and adjusted EXPECT_CALL invocations.
recvfrom_control from NetworkStackstub was implemented in a wrong way. Unit test TestUDPSocket.recv_address_filtering was running into timeout because of that.
default implementations for recvfrom_control/sendto_control in the NetworkStack were moved to the header file. They were implemented to not break the API for all existing NetworkStack implementors and we don't want to change linking information.

This patch contains improvements mentioned in the unresolved PR comments: - function names were changed from socket_sendmsg/socket_recvmsg to socket_sendto_control/socket_recvfrom_control. - default implementation of this functions was provided in the NetworkStack class. - MsgHeaderIterator accesses elements on the aligned addresses.

pan-

Thanks for adding the test and applying the change requested in the previous PR. I left comments for few improvements that should be made to that PR.

connectivity/lwipstack/include/lwipstack/lwipopts.h

connectivity/netsocket/include/netsocket/MsgHeader.h

connectivity/netsocket/include/netsocket/nsapi_types.h

Doxygen documentation for MsgHeaderIterator was added. Type used for alignment of the nsapi_msghdr_t struct was changed.

Minor errors in the documentation were fixed. These caused failure of the doxygen checks in the PR.

0xc0170

As this is a feature pull request, should there be referenced docs update?

connectivity/netsocket/tests/UNITTESTS/netsocket/NetworkStack/test_MsgHeaderIterator.cpp

0xc0170 · 2021-09-06T12:58:16Z

connectivity/netsocket/source/TCPSocket.cpp

+nsapi_size_or_error_t TCPSocket::sendto_control(const SocketAddress &address, const void *data, nsapi_size_t size,
+                                                nsapi_msghdr_t *control, nsapi_size_t control_size)
+{
+    // FIXME: Implement


Can you rather create an tracking issue for this and remove the comment (reference the issue in the commit message, it can be easily found then)

Done, new issue - #15057

Issue number should be added in the amended commit message when merging this PR into master

pan- · 2021-09-07T12:59:24Z

@0xc0170 Yes a doc update is required, I'm doing it.

Pull request has been modified.

0xc0170 · 2021-09-08T10:30:15Z

CI started

mbed-ci · 2021-09-08T11:03:27Z

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	❌
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	❌
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-greentea-ARM	❌
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	❌
jenkins-ci/mbed-os-ci_build-example-ARM	❌
jenkins-ci/mbed-os-ci_cmake-example-ARM	❌

0xc0170 · 2021-09-08T13:28:41Z

The build errors are related, please review the logs

Macro MBED_ALIGN expands in C to _Alignas which can't be used in the type declaration. This patch moves it to the first type definition which makes this code compile properly in CPP and C.

Pull request has been modified.

mat-kalinowski · 2021-09-08T14:38:46Z

The build errors are related, please review the logs

Done, It should work fine now

0xc0170 · 2021-09-09T06:15:06Z

CI started

mbed-ci · 2021-09-09T07:14:50Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

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

bdfd98e

ciarmcom added the release-type: feature label Sep 1, 2021

ciarmcom requested review from pan- and a team September 1, 2021 09:30

ciarmcom added needs: review component: connectivity component: mesh labels Sep 1, 2021

mat-kalinowski force-pushed the sendrecv_message branch from ad047e3 to ec3f437 Compare September 3, 2021 11:51

pan- reviewed Sep 3, 2021

View reviewed changes

connectivity/lwipstack/include/lwipstack/lwipopts.h Outdated Show resolved Hide resolved

connectivity/netsocket/include/netsocket/MsgHeader.h Show resolved Hide resolved

connectivity/netsocket/include/netsocket/nsapi_types.h Outdated Show resolved Hide resolved

mat-kalinowski added 2 commits September 6, 2021 11:14

Adding documentation and minor code changes

c3d16f7

Doxygen documentation for MsgHeaderIterator was added. Type used for alignment of the nsapi_msghdr_t struct was changed.

Fixing documentation errors

575f489

Minor errors in the documentation were fixed. These caused failure of the doxygen checks in the PR.

pan- previously approved these changes Sep 6, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Sep 6, 2021

0xc0170 previously requested changes Sep 6, 2021

View reviewed changes

mergify bot added needs: work and removed needs: CI labels Sep 6, 2021

pan- mentioned this pull request Sep 7, 2021

Netsocket: mention control versions of sendto/recvfrom. ARMmbed/mbed-os-5-docs#1484

Merged

Removing FIXME comment and adding empty line in the test file

f8db068

pan- previously approved these changes Sep 8, 2021

View reviewed changes

mergify bot added the needs: CI label Sep 8, 2021

mergify bot removed the needs: work label Sep 8, 2021

0xc0170 previously approved these changes Sep 8, 2021

View reviewed changes

mergify bot added needs: work and removed needs: CI labels Sep 8, 2021

Moving alignment information in the nsapi_msghdr_t

ad0eb0e

Macro MBED_ALIGN expands in C to _Alignas which can't be used in the type declaration. This patch moves it to the first type definition which makes this code compile properly in CPP and C.

pan- approved these changes Sep 8, 2021

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Sep 8, 2021

mergify bot added ready for merge and removed needs: CI labels Sep 9, 2021

0xc0170 merged commit f89655f into ARMmbed:master Sep 9, 2021

mergify bot removed the ready for merge label Sep 9, 2021

0xc0170 mentioned this pull request Sep 9, 2021

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

Closed

mbedmain added release-version: 6.15.0 Release-pending and removed release-type: feature Release-pending labels Sep 16, 2021

rardiol mentioned this pull request Sep 28, 2021

DNS query fails with Nanostack on socket_sendto_control #15118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mat-kalinowski commented Sep 1, 2021

ciarmcom commented Sep 1, 2021

0xc0170 commented Sep 1, 2021

mat-kalinowski commented Sep 2, 2021 •

edited

Loading

pan- left a comment

0xc0170 left a comment

0xc0170 Sep 6, 2021

mat-kalinowski Sep 7, 2021

mat-kalinowski Sep 7, 2021

pan- commented Sep 7, 2021

0xc0170 commented Sep 8, 2021

mbed-ci commented Sep 8, 2021

0xc0170 commented Sep 8, 2021

mat-kalinowski commented Sep 8, 2021 •

edited

Loading

0xc0170 commented Sep 9, 2021

mbed-ci commented Sep 9, 2021

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

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

Conversation

mat-kalinowski commented Sep 1, 2021

Summary of changes

Impact of changes

Migration actions required

Documentation

Stale PR with this feature: #14847

Pull request type

Test results

Reviewers

@pan-

ciarmcom commented Sep 1, 2021

0xc0170 commented Sep 1, 2021

mat-kalinowski commented Sep 2, 2021 • edited Loading

pan- left a comment

Choose a reason for hiding this comment

0xc0170 left a comment

Choose a reason for hiding this comment

0xc0170 Sep 6, 2021

Choose a reason for hiding this comment

mat-kalinowski Sep 7, 2021

Choose a reason for hiding this comment

mat-kalinowski Sep 7, 2021

Choose a reason for hiding this comment

pan- commented Sep 7, 2021

0xc0170 commented Sep 8, 2021

mbed-ci commented Sep 8, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

0xc0170 commented Sep 8, 2021

mat-kalinowski commented Sep 8, 2021 • edited Loading

0xc0170 commented Sep 9, 2021

mbed-ci commented Sep 9, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Stale PR with this feature:
#14847

mat-kalinowski commented Sep 2, 2021 •

edited

Loading

mat-kalinowski commented Sep 8, 2021 •

edited

Loading