Add property API to InternetSocket #12522

artokin · 2020-02-27T09:10:16Z

Summary of changes

Add new API to network interface.

Property API allows application to ask network specific properties
from the ~~NetworkInterface~~InternetSocket . Queried information can be used to avoid
network congestion by adjusting transmission jitter and retry
timeouts.

~~Please note, this work is in progress.~~

Impact of changes

Migration actions required

Documentation

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

@mikter

ciarmcom · 2020-02-27T10:00:39Z

@artokin, thank you for your changes.
@mikter @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

AnttiKauppila · 2020-02-27T11:21:14Z

features/netsocket/NetworkInterface.h

+     * @return NSAPI_ERROR_OK on success
+     * @return NSAPI_ERROR_UNSUPPORTED, NSAPI_ERROR_NO_CONNECTION
+     */
+    virtual nsapi_error_t get_interface_property(unsigned int data_amount, unsigned int *jitter);


Why is it needed in top level?

Having API here allows application to be independent of actual interface.

The ROM cost here due to the virtual methods, particularly if broken up, is significant. And these are quite specialised calls.

Therefore my inclination would be to do this via new reason codes for getsockopt, avoiding adding anything to the vtable.

Particularly as something like RTT information may in principle be connection/remote-peer-specific, not simply a property of the interface, and the application ultimately is going to want to be working with a socket anyway.

Note that you can still have "nice wrappers" around a setsockopt, such as InternetSocket::join_multicast_group or TLSSocket::set_hostname (offboard implementation). They would be non-virtual, so no overhead.

mikter · 2020-03-10T07:17:30Z

Is there need to get this information without socket creation? example setting DTLS retry timers on creation?

Is the random Jitter for startup in separate pull request?

artokin · 2020-03-17T14:37:20Z

Latency and stagger helper methods added to InternetSocket.
Previous commits squashed and work rebased (won't compile with new Nanostack).

@mikter , @kjbracey-arm, @AnttiKauppila, @SeppoTakalo would you please review?

mikter · 2020-03-20T06:57:10Z

features/netsocket/InternetSocket.cpp

+    ret = this->getsockopt(NSAPI_SOCKET, NSAPI_STAGGER, (void *)&nsapi_stagger, &opt_len);
+    if (ret == NSAPI_ERROR_OK) {
+        // success, stagger found
+        *stagger_min = nsapi_stagger.stagger_min;


Should you have null checks for parameter existence so this can be called just to get the rand_value only

Absolutely! Added in the next commit.

Pull request has been modified.

SeppoTakalo · 2020-03-20T09:58:21Z

I don't understand the point of extending InternetSocket API with wrappers that only call getsockopt().

If properly documented, application may call getsockopt() directly.

0xc0170 · 2020-04-14T09:29:42Z

@artokin What is the next action for this PR? Shall we keep it still opened or not?

artokin · 2020-04-14T10:12:14Z

@0xc0170 , this is waiting for new Nanostack that is coming via #12754 . Please keep this open, work will continue soon.

Add getsockopt options NSAPI_LATENCY and NSAPI_STAGGER to read network specific timing constraints from socket. -NS_LATENCY returns estimated latency to given address. -NSAPI_STAGGER returns estimated initial delay that application should wait before transmitting data to network. Application can use the new options to avoid network congestion by adjusting transmission delays and retry timeouts.

-Check pointer before writing to it. It allows application to ask only some of the stagger values. -Change type of data_amount in get_stagger_estimate_to_address to be uint16_t.

Add unit tests for added methods: -get_rtt_estimate_to_address -get_stagger_estimate_to_address

Pull request has been modified.

artokin · 2020-04-23T09:08:36Z

@0xc0170 , thanks for reminding. A new test case added to unit tests.

I'm assuming that method descriptions would be enough and there is no need to update documentation because of these added methods.

0xc0170 · 2020-04-23T18:58:24Z

CI started

mbed-ci · 2020-04-23T21:59:33Z

Test run: SUCCESS

Summary: 7 of 7 test jobs passed
Build number : 4
Build artifacts

kjbracey

Seems fine. Not 100% sure of the value of the wrapper around the socket option - stagger is a complex multivalue call, so not sure that all the pointer out parameters are really any simpler than using the socket option with the structure output. But I guess we set the precedent with the multicast options.

kjbracey · 2020-04-27T07:11:42Z

features/netsocket/InternetSocket.cpp

+    // Set up address
+    memcpy(ns_api_latency_req.addr, address.get_ip_bytes(), 16);
+
+    ret = this->getsockopt(NSAPI_SOCKET, NSAPI_LATENCY, (void *)&ns_api_latency_req, &opt_len);


Unnecessary cast.

Pull request has been modified.

0xc0170 · 2020-04-27T15:48:01Z

CI started

mbed-ci · 2020-04-27T18:01:35Z

Test run: FAILED

Summary: 1 of 7 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_wisun-mesh-test

artokin · 2020-04-28T08:15:37Z

Restarted failing CI job, seems to be OK now.

Add getsockopt options NSAPI_LATENCY and NSAPI_STAGGER to read network specific timing constraints from socket. -NS_LATENCY returns estimated latency to given address. -NSAPI_STAGGER returns estimated initial delay that application should wait before transmitting data to network. Application can use the new options to avoid network congestion by adjusting transmission delays and retry timeouts. Add wrappers to make API usage easier: -get_rtt_estimate_to_address -get_stagger_estimate_to_address This is backport of the ARMmbed#12522

mergify bot added the needs: work label Feb 27, 2020

ciarmcom requested review from mikter and a team February 27, 2020 10:00

ciarmcom added the needs: review label Feb 27, 2020

AnttiKauppila reviewed Feb 27, 2020

View reviewed changes

0xc0170 changed the title ~~[WIP] Add property API to NetworkInterface~~ WIP: Add property API to NetworkInterface Feb 28, 2020

0xc0170 removed the needs: work label Feb 28, 2020

mergify bot added the do not merge label Feb 28, 2020

artokin force-pushed the network_interface_property_api branch from f23e06c to 52a00e2 Compare March 3, 2020 11:12

mergify bot added needs: work and removed needs: review labels Mar 3, 2020

artokin force-pushed the network_interface_property_api branch from cffa107 to 1e0e5f8 Compare March 9, 2020 15:23

artokin requested a review from kjbracey March 10, 2020 09:38

mikter requested a review from yogpan01 March 16, 2020 09:45

artokin force-pushed the network_interface_property_api branch from 1e0e5f8 to db2c970 Compare March 17, 2020 14:32

artokin requested review from AnttiKauppila and SeppoTakalo March 19, 2020 14:34

AnttiKauppila previously approved these changes Mar 20, 2020

View reviewed changes

mikter reviewed Mar 20, 2020

View reviewed changes

Arto Kinnunen added 2 commits April 17, 2020 08:55

Review corrections

a14ccad

-Check pointer before writing to it. It allows application to ask only some of the stagger values. -Change type of data_amount in get_stagger_estimate_to_address to be uint16_t.

artokin force-pushed the network_interface_property_api branch from 9f209c5 to a14ccad Compare April 17, 2020 05:56

Add unittests for added methods

329c5d8

Add unit tests for added methods: -get_rtt_estimate_to_address -get_stagger_estimate_to_address

0xc0170 added needs: CI and removed ready for merge labels Apr 23, 2020

kjbracey previously approved these changes Apr 27, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Apr 27, 2020

Remove unnecessary casts from stagger/RTT API

f39da5e

0xc0170 added needs: CI mergify skip and removed ready for merge labels Apr 27, 2020

0xc0170 added needs: review ready for merge and removed mergify skip needs: CI needs: review labels Apr 28, 2020

0xc0170 approved these changes Apr 28, 2020

View reviewed changes

0xc0170 merged commit 58c0259 into ARMmbed:master Apr 28, 2020

mergify bot removed the ready for merge label Apr 28, 2020

artokin mentioned this pull request Apr 29, 2020

[Mbed OS 5.15] Add getsockopt option to read network property #12882

Merged

artokin mentioned this pull request Apr 29, 2020

[feature-wisun] Add getsockopt option to read network property #12883

Merged

Add property API to InternetSocket #12522

Add property API to InternetSocket #12522

Uh oh!

Conversation

artokin commented Feb 27, 2020 • edited by 0xc0170 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Feb 27, 2020

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!

mikter commented Mar 10, 2020

Uh oh!

artokin commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo commented Mar 20, 2020

Uh oh!

0xc0170 commented Apr 14, 2020

Uh oh!

artokin commented Apr 14, 2020

Uh oh!

artokin commented Apr 23, 2020

Uh oh!

0xc0170 commented Apr 23, 2020

Uh oh!

mbed-ci commented Apr 23, 2020

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 27, 2020

Uh oh!

mbed-ci commented Apr 27, 2020

Uh oh!

artokin commented Apr 28, 2020

Uh oh!

Uh oh!

artokin commented Feb 27, 2020 •

edited by 0xc0170

Loading

artokin commented Mar 17, 2020 •

edited

Loading