[ONME-2927] Socket adaptation layer for nanostack #3517

hasnainvirk · 2017-01-03T16:37:27Z

STATUS: ready

As a result of major overhaul in Nanostack generally for socket API and especially
for TCP, the adaptation layer for mbed-OS is being upgraded.

Previously, as nanostack was not able to provide receive queue, adaptation layer had been
faking it. Now with Stream Socket by default Nanostack provides 2K receive queue and 2K send queue.
Receive queue size can be changed using setsockopt(). Batre metal nanostack would not provide with any
receive queues with Datagram Socket, however in this adaptation layer we introduce a 2K receive queue size
for the Datagram Socket as well.

Layer state machine handling is polished to ensure robustness.

::socket_connect() will can return 2 new error codes now. NSAPI_ERROR_ALREADY (like posix EALREADY) in case
if the connection is in progress or NSAPI_ERROR_IS_CONNECTED (like posix EISCONN) if already connected.
NSAPI_ERROR_WOULDBLOCK is now mapped directly to nanostack NS_WOULDBLOCK.

NOTE: Target is 5.4 release. Should not go into 5.3.x patch releases.

hasnainvirk · 2017-01-03T16:45:40Z

@c1728p9 @sg- @bridadan @geky Please review. Will not pass tests as we need to update Nanostack binaries first.

@kjbracey-arm @SeppoTakalo
When should we update nanostack binaries ?

SeppoTakalo · 2017-01-04T09:11:32Z

@hasnainvirk Please have following line in the message: STATUS: IN PROGRESS because this cannot be merged in before we build new binaries.

bridadan · 2017-01-04T16:54:05Z

@SeppoTakalo @hasnainvirk went ahead and marked this as do not merge due to the in progress status. When the new binaries are ready please update us here, thanks!

c1728p9

PR looks good aside from some minor changes. I saw that TCP accept was added. Does this bring in support for TCP server sockets? That would be awesome.

c1728p9 · 2017-01-06T22:06:58Z

features/nanostack/FEATURE_NANOSTACK/nanostack-interface/NanostackInterface.cpp

 const char * NanostackInterface::get_ip_address()
 {
+    static char text_address[40];


test_address should probably be a part of the NanostackInterface class rather than static. You should also probably be holding nanostack's lock during this call.

c1728p9 · 2017-01-06T22:09:31Z

features/nanostack/FEATURE_NANOSTACK/nanostack-interface/NanostackInterface.cpp

-        return NSAPI_ERROR_UNSUPPORTED;
+    nsapi_size_or_error_t ret;
+
+    if (socket->closed() || (!address && !socket->is_connected())) {


This check should be protected with nanostack_lock

c1728p9 · 2017-01-06T22:20:50Z

features/nanostack/FEATURE_NANOSTACK/nanostack-interface/NanostackInterface.cpp

+    ns_address_t ns_address;
+    convert_mbed_addr_to_ns(&ns_address, &address);
+
+    return do_sendto(handle, &ns_address, data, size);


might add a comment that no locking is needed in socket_sendto since do_sendto is thread safe.

c1728p9 · 2017-01-06T22:21:36Z

features/nanostack/FEATURE_NANOSTACK/nanostack-interface/NanostackInterface.cpp


    nsapi_size_or_error_t ret;
+
    if (socket->closed()) {


Socket could change state so the check should be done when nanostack_lock is held

c1728p9 · 2017-01-06T22:27:15Z

features/nanostack/FEATURE_NANOSTACK/nanostack-interface/NanostackInterface.cpp

+        goto out;
+    }
+
+    if (socket->closed()) {


Need to lock before checking state that could change

c1728p9 · 2017-01-06T22:27:42Z

features/nanostack/FEATURE_NANOSTACK/nanostack-interface/NanostackInterface.cpp

        MBED_ASSERT(false);
        return NSAPI_ERROR_NO_SOCKET;
    }

-    nanostack_lock();
+    if (!socket->is_listening()) {


Need to lock before checking state that could change

hasnainvirk · 2017-01-09T09:44:22Z

@c1728p9 Hi Russ, yes we have introduced TCP server sockets. Thanks for the review.

kjbracey · 2017-01-09T10:18:11Z

Another notable benefit is that both client and server TCP sockets now have reliable delivery of incoming data. The queuing system used previously couldn't totally achieve that - there was no way to provide flow-control back-pressure to the sender, and if we ended up running out of queue memory and dropping an incoming chunk of data, it would be silently lost.

(So far it has worked quite well in practice, but it has relied on apps reading data fast enough.)

Nanostack's new queuing allows a per-socket limit to be imposed for both UDP and TCP rather than just queuing until memory exhaustion as NanostackInterface did.

bridadan · 2017-01-09T15:59:17Z

@VeliMattiLahtela the mbed-os-cliapp failed again, I've restarted the CI

SeppoTakalo · 2017-01-09T16:05:12Z

@bridadan It is supposed to fail.
STATUS: IN PROGRESS
Requires nanostack binaies

There is a API change in Nanostack so the build cannot pass until we provide new binaries.

bridadan · 2017-01-09T16:23:52Z

@SeppoTakalo My mistake, should have read the discussion! Thanks for pointing that out.

SeppoTakalo · 2017-02-02T10:47:58Z

Rebased the branch and imported new Nanostack binaries with coap-service update.

SeppoTakalo · 2017-02-02T15:49:36Z

Tested with mbed-os-cliapp network testsuite.

+----------------------------------------------+---------+-------------+-------------+------------------+-----------+
| Testcase                                     | Verdict | Fail Reason | Skip Reason |    platforms     |  duration |
+----------------------------------------------+---------+-------------+-------------+------------------+-----------+
| boot_smoke_test                              |   pass  |             |             |       K64F       | 62.145883 |
| interface_up_down                            |   pass  |             |             |       K64F       | 49.723533 |
| socket_UDP_transfer_verify_data_payload_100  |   skip  |             |   Unstable  | unknown platform |     0     |
| socket_UDP_transfer_verify_data_payload_1000 |   skip  |             |   Unstable  | unknown platform |     0     |
| cumulative_UDP_transfer_payload_500          |   skip  |             |   Unstable  | unknown platform |     0     |
| ticker_test_with_network                     |   pass  |             |             |       K64F       | 69.703006 |
| ticker_with_ping                             |   skip  |             |   Unstable  | unknown platform |     0     |
+----------------------------------------------+---------+-------------+-------------+------------------+-----------+
+---------------+----------------+
|    Summary    |                |
+---------------+----------------+
| Final Verdict |      PASS      |
|     count     |       7        |
|      pass     |       3        |
|      skip     |       4        |
|    Duration   | 0:03:01.572422 |
+---------------+----------------+

This is not ready to be merged.

NOTE: Should not go into 5.3.x patch releases. Target is 5.4

SeppoTakalo · 2017-02-02T15:53:27Z

From Cam-CI I can see:

11:00:37 mbedgt: test suite report:
11:00:37 +--------------+---------------+------------+---------+--------------------+-------------+
11:00:37 | target       | platform_name | test suite | result  | elapsed_time (sec) | copy_method |
11:00:37 +--------------+---------------+------------+---------+--------------------+-------------+
11:00:37 | K64F-GCC_ARM | K64F          | 1          | TIMEOUT | 145.8              | shell       |
11:00:37 +--------------+---------------+------------+---------+--------------------+-------------+
11:00:37 mbedgt: test suite results: 1 TIMEOUT
11:00:37 mbedgt: test case report:
11:00:37 +--------------+---------------+------------+-----------+--------+--------+--------+--------------------+
11:00:37 | target       | platform_name | test suite | test case | passed | failed | result | elapsed_time (sec) |
11:00:37 +--------------+---------------+------------+-----------+--------+--------+--------+--------------------+
11:00:37 | K64F-GCC_ARM | K64F          | 1          | 1         | 0      | 1      | ERROR  | 145.8              |
11:00:37 +--------------+---------------+------------+-----------+--------+--------+--------+--------------------+
11:00:37 mbedgt: test case results: 1 ERROR
11:00:37 mbedgt: completed in 157.00 sec
11:00:37 mbedgt: exited with code 1

Is this CI error?

SeppoTakalo · 2017-02-06T12:39:04Z

@adbridge @0xc0170 This is ready.
However, I don't know how to relaunch that Cam-CI. Can somebody?
I don't believe this changed anything that is tested within those tests.

adbridge · 2017-02-06T14:36:02Z

retest uvisor

adbridge · 2017-02-06T15:30:19Z

@mazimkhan I appear not to be able to restart uvisor...

mazimkhan · 2017-02-06T15:35:41Z

retest uvisor

bridadan · 2017-02-06T16:24:42Z

/morph test-nightly

mbed-bot · 2017-02-06T18:57:19Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1511

All builds and test passed!

As a result of major overhaul in Nanostack generally for socket API and especially for TCP, the adaptation layer for mbed-OS is being upgraded. Previously, as nanostack was not able to provide receive queue, adaptation layer had been faking it. Now with Stream Socket by default Nanostack provides 2K receive queue and 2K send queue. Receive queue size can be changed using setsockopt(). Batre metal nanostack would not provide with any receive queues with Datagram Socket, however in this adaptation layer we introduce a 2K receive queue size for the Datagram Socket as well. Layer state machine handling is polished to ensure robustness. ::socket_connect() will can return 2 new error codes now. NSAPI_ERROR_ALREADY (like posix EALREADY) in case if the connection is in progress or NSAPI_ERROR_IS_CONNECTED (like posix EISCONN) if already connected. NSAPI_ERROR_WOULDBLOCK is now mapped directly to nanostack NS_WOULDBLOCK. NanostackLockGaurd class is introduced which enables us to claim and release mutex using RAII style.

… from a1982c1..e125164 e125164 Check secure session pointer in timer callback (ARMmbed#61) f49e596 Update unit tests (ARMmbed#59) 6a5634a Support for multiple virtual services (ARMmbed#58) 7fe6b98 Remove yotta files (ARMmbed#57) 5c5c8fe Fix socket send return value overflow (ARMmbed#56) 0870d05 Update unit test stubs to match latest socket api (ARMmbed#55) e687be8 Merge pull request ARMmbed#54 from ARMmbed/warn_fixes b8fe613 updated unittests 8640d05 Compilation warnings fixed eea83e5 Flag out entropy source addition (ARMmbed#53) 7d72eb4 Fix unittests (ARMmbed#52) 4a6991e Avoid referencing ns_msghdr_t::flags git-subtree-dir: features/nanostack/FEATURE_NANOSTACK/coap-service git-subtree-split: e125164

hasnainvirk · 2017-02-12T20:13:03Z

@c1728p9 @kjbracey-arm @SeppoTakalo , I have amended some stuff pointed out by Russ. This should be okay now. Please proceed with the tests and merge once again.

hasnainvirk · 2017-02-13T07:53:37Z

@bridadan Can you please run nightly tests on it ? We need to move forward here.

kjbracey

There are still some remaining nanostack_locks, and actually some apparently erroneous nanostack_unlocks after LockGuard.

kjbracey · 2017-02-13T09:00:32Z

features/nanostack/FEATURE_NANOSTACK/nanostack-interface/NanostackInterface.cpp

@@ -381,7 +382,9 @@ void NanostackSocket::event_connect_fail(socket_callback_t *sock_cb)
 }

 void NanostackSocket::event_incoming_connection(socket_callback_t *sock_cb)
+


Editing error

NanostackLockGuard usage is now consistent throughout. Missing Lock assertion added at a few locations in the code

In sendto(), memory allocation failures were mistakenly being treated as would blocks (assumption was that the device might be able to recover). However, that put the blocking socket into deep sleep and there was no mechanism to wake it up ever again. Somehow that got slipped through testing. Fixed in this amenment

SeppoTakalo · 2017-02-13T15:17:03Z

@0xc0170 @adbridge This is ready to go in.

bridadan · 2017-02-13T15:49:49Z

/morph test-nightly

mbed-bot · 2017-02-13T22:00:50Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1573

All builds and test passed!

0xc0170 · 2017-02-14T13:29:48Z

retest uvisor

0xc0170 · 2017-02-14T13:31:23Z

I restarted uvisor and jenkins CI, lets wait for results

SeppoTakalo · 2017-02-14T14:52:59Z

That Cam-CI uvisor shows

[Pipeline] End of Pipeline
Finished: SUCCESS

Is it not capable of setting the status here?

bridadan · 2017-02-14T20:16:55Z

@SeppoTakalo Usually this is due to an issue with the CI GitHub account running into the API query limit (there doesn't seem to be a way to increase the limit). I've talked with @tommikas and @mazimkhan multiple times about this, I think the plan is each CI will get a separate account.

mazimkhan · 2017-02-14T20:36:26Z

@SeppoTakalo I have restarted the job to update CI status. As it shows now.
This issue is due to GitHub rate limit issue. I suppose user ciarmcom was supposed to be left for Cambridge CI. We can discuss this issue offline.

bridadan added the do not merge label Jan 4, 2017

c1728p9 reviewed Jan 6, 2017

View reviewed changes

hasnainvirk force-pushed the ONME_2927 branch 2 times, most recently from 3b96ae4 to aa87036 Compare January 9, 2017 09:42

kjbracey approved these changes Jan 9, 2017

View reviewed changes

SeppoTakalo added needs: work scope: feature labels Jan 25, 2017

SeppoTakalo force-pushed the ONME_2927 branch from aa87036 to d6a60ac Compare February 2, 2017 10:46

SeppoTakalo added release-version: 5.4.0 and removed do not merge needs: work labels Feb 2, 2017

adbridge added the needs: CI label Feb 6, 2017

bridadan removed the needs: CI label Feb 6, 2017

Hasnain Virk and others added 3 commits February 12, 2017 22:05

Nanostack 6.0.1

667006e

hasnainvirk force-pushed the ONME_2927 branch from d6a60ac to 67ffdc4 Compare February 12, 2017 20:09

kjbracey suggested changes Feb 13, 2017

View reviewed changes

hasnainvirk force-pushed the ONME_2927 branch 2 times, most recently from 5e65978 to b464ed0 Compare February 13, 2017 09:12

Minor amendments

1f24533

NanostackLockGuard usage is now consistent throughout. Missing Lock assertion added at a few locations in the code

hasnainvirk force-pushed the ONME_2927 branch from b464ed0 to 1f24533 Compare February 13, 2017 09:14

kjbracey approved these changes Feb 13, 2017

View reviewed changes

SeppoTakalo approved these changes Feb 13, 2017

View reviewed changes

c1728p9 approved these changes Feb 13, 2017

View reviewed changes

bridadan added needs: CI and removed needs: review labels Feb 13, 2017

bridadan added ready for merge and removed needs: CI labels Feb 14, 2017

sg- removed the ready for merge label Feb 17, 2017

sg- merged commit 769712f into ARMmbed:master Feb 17, 2017

		@@ -381,7 +382,9 @@ void NanostackSocket::event_connect_fail(socket_callback_t *sock_cb)
		}

		void NanostackSocket::event_incoming_connection(socket_callback_t *sock_cb)

[ONME-2927] Socket adaptation layer for nanostack #3517

[ONME-2927] Socket adaptation layer for nanostack #3517

Uh oh!

Conversation

hasnainvirk commented Jan 3, 2017 • edited by SeppoTakalo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasnainvirk commented Jan 3, 2017

Uh oh!

SeppoTakalo commented Jan 4, 2017

Uh oh!

bridadan commented Jan 4, 2017

Uh oh!

c1728p9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasnainvirk commented Jan 9, 2017

Uh oh!

kjbracey commented Jan 9, 2017

Uh oh!

bridadan commented Jan 9, 2017

Uh oh!

SeppoTakalo commented Jan 9, 2017

Uh oh!

bridadan commented Jan 9, 2017

Uh oh!

SeppoTakalo commented Feb 2, 2017

Uh oh!

SeppoTakalo commented Feb 2, 2017

Uh oh!

SeppoTakalo commented Feb 2, 2017

Uh oh!

SeppoTakalo commented Feb 6, 2017

Uh oh!

adbridge commented Feb 6, 2017

Uh oh!

adbridge commented Feb 6, 2017

Uh oh!

mazimkhan commented Feb 6, 2017

Uh oh!

bridadan commented Feb 6, 2017

Uh oh!

mbed-bot commented Feb 6, 2017

Output

Uh oh!

hasnainvirk commented Feb 12, 2017

Uh oh!

hasnainvirk commented Feb 13, 2017

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo commented Feb 13, 2017

Uh oh!

bridadan commented Feb 13, 2017

Uh oh!

mbed-bot commented Feb 13, 2017

Output

Uh oh!

0xc0170 commented Feb 14, 2017

Uh oh!

0xc0170 commented Feb 14, 2017

Uh oh!

SeppoTakalo commented Feb 14, 2017

Uh oh!

bridadan commented Feb 14, 2017

hasnainvirk commented Jan 3, 2017 •

edited by SeppoTakalo

Loading