Add NetworkInterface::get_default_instance() #6855

kjbracey · 2018-05-09T14:10:26Z

Description

Provide a framework to make it easier to find a default network
interface.

New version of #6108

Envisaged use:

NetworkInterface &net = NetworkInterface::get_default_instance();
net.connect();

// for advanced users:
WiFiInterface *wifi = net.wifiInterface();
if (wifi) {
wifi->do_something_wifiy();
}

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

cmonr · 2018-05-09T19:27:02Z

@kjbracey-arm I don't think the CI tools work well with changing the target base branch in a PR. Going to close and reopen this PR to see if the issues fix themselves.

cmonr · 2018-05-09T20:00:10Z

@ARMmbed/mbed-os-maintainers FYI, if a PR has it's base branch changed, waiting for current tests to complete, closing, and reopening should fix testing issues. If a PR is closed and reopened too quickly, the merge checks won't update their base branch.

cmonr · 2018-05-09T20:00:48Z

@kjbracey-arm Upon quick inspection the failures in travis look legit. Let us know if something is amis.

kjbracey · 2018-05-11T08:14:17Z

Thanks for fiddling with the CI - my fault for messing up the branch. Will recreate PR if problems turn out to be CI related. (I suspect this does need some more work).

TeroJaasko · 2018-05-17T11:27:56Z

features/netsocket/NetworkInterface.cpp

+#include "EthInterface.h"
+#include "WiFiInterface.h"
+#include "CellularBase.h"
+#include "MeshInterface.h"


Having #include lines in the middle of a C file looks a bit weird, is this a leftover from rebasing?

It's possibly a sign that the thing should be split into two files. Everything above this point is the "core" NetworkInterface stuff which knows nothing about derived types.

This point and below is the "dispatch to known derived types" logic. Which arguably is a piece of "system" code providing the built-in behaviour for the 4 core types.

Possibly move this half of the file to NetworkInterfaceDefaults.cpp?

SeppoTakalo

I like this.
We are finally going to the right direction and getting rid of the easy-connect's #ifdef jungle.

TeroJaasko · 2018-05-17T11:58:43Z

features/nanostack/mbed-mesh-api/source/ThreadInterface.cpp

@@ -231,3 +231,13 @@ mesh_error_t Nanostack::ThreadInterface::device_pskd_set(const char *pskd)
 {
    return (mesh_error_t)thread_tasklet_device_pskd_set(pskd);
 }
+
+#define THREAD 0x2345


Should this macro comparison magic be commented somewhere, eg. here? On #6108 (comment) there was a good explanation for it.

Provide an initial framework to make it easier to find a default network interface.

kjbracey · 2018-05-17T12:09:27Z

Moved new code from NetworkInterface.cpp to a new NetworkInterfaceDefaults.cpp.

TeroJaasko

This is definitely 2^63 times better than anything we currently have.

cmonr · 2018-05-17T15:35:35Z

/morph build

mbed-ci · 2018-05-17T17:35:25Z

Build : SUCCESS

Build number : 2056
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6855/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-05-18T01:00:54Z

Test : SUCCESS

Build number : 1865
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6855/1865

mbed-ci · 2018-05-18T01:11:09Z

Exporter Build : FAILURE

Build number : 1698
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6855/

cmonr · 2018-05-18T02:52:26Z

/morph export-build

mbed-ci · 2018-05-18T06:18:41Z

Exporter Build : FAILURE

Build number : 1702
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6855/

kjbracey · 2018-05-18T10:21:00Z

Exporter build passed after retriggering the one spurious failure. Guess individual triggers don't work for updating the status though?

https://mbed-os.mbedcloudtesting.com/job/exporter-build-matrix/1706/
http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6855/

kjbracey · 2018-05-18T10:25:46Z

/morph export-build

mbed-ci · 2018-05-18T13:36:08Z

Exporter Build : FAILURE

Build number : 1707
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6855/

0xc0170 · 2018-05-18T13:38:26Z

/morph export-build

mbed-ci · 2018-05-18T19:17:49Z

Exporter Build : SUCCESS

Build number : 1710
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6855/

0xc0170 · 2018-05-20T21:12:41Z

features/nanostack/nanostack-interface/NanostackRfPhy.h

+     * Returns the default on-board NanostackRfPhy - this will be target-specific, and
+     * may not be available on all targets.
+     */
+    static NanostackRfPhy &get_default_instance();


curiosity why is this a reference and the rest pointers?

Slightly different approach to "does it exist" handling.

This version aligns with what EMAC::get_default_instance() does, which says that it must exist, if DEVICE_XXX is defined. There's no "null" possibility, hence a reference. It's kind of the "HAL" end.

For all the app-level end - NetworkInterface and similar, there is no controlling macro, but there is the possibility to override at link or run time (including a runtime check for a module being fitted!). In that case, there are underlying weak definitions that return NULL if the thing isn't provided.

So:

#if DEVICE_EMAC EMAC &emac = EMAC::get_default_instance(); // must work #else #error "No default EMAC" #endif WiFiInterface *wifi = WiFiInterface::get_default_instance(); if (!wifi) { error("Wifi not available"); }

This doesn't quite align - the upper style is closer to what the "HAL" does, but the lower is more flexible. First approach was the initial, and is long-existing on feature-emac. We ultimately had to do the second to solve various build problems for the higher levels, and it didn't seem worth going back to change the EMAC form.

kjbracey changed the base branch from master to feature-emac May 9, 2018 14:11

cmonr closed this May 9, 2018

cmonr reopened this May 9, 2018

cmonr closed this May 9, 2018

cmonr reopened this May 9, 2018

cmonr added the needs: work label May 9, 2018

kjbracey force-pushed the default_net branch 3 times, most recently from 533d2c9 to 105d860 Compare May 11, 2018 15:05

cmonr requested a review from mikaleppanen May 14, 2018 15:59

cmonr added release-version: 5.9.0-rc1 needs: review and removed needs: work labels May 14, 2018

kjbracey requested review from SeppoTakalo, JanneKiiskila and TeroJaasko May 15, 2018 07:49

kjbracey force-pushed the default_net branch from 105d860 to 8be90af Compare May 17, 2018 08:30

mikaleppanen approved these changes May 17, 2018

View reviewed changes

TeroJaasko reviewed May 17, 2018

View reviewed changes

SeppoTakalo approved these changes May 17, 2018

View reviewed changes

TeroJaasko reviewed May 17, 2018

View reviewed changes

Add NetworkInterface::get_default_instance()

2f5c66a

Provide an initial framework to make it easier to find a default network interface.

kjbracey force-pushed the default_net branch from 8be90af to 2f5c66a Compare May 17, 2018 12:07

TeroJaasko approved these changes May 17, 2018

View reviewed changes

cmonr added needs: CI and removed needs: review labels May 17, 2018

0xc0170 added ready for merge and removed needs: CI ready for merge release-version: 5.9.0-rc1 labels May 19, 2018

0xc0170 reviewed May 20, 2018

View reviewed changes

0xc0170 approved these changes May 20, 2018

View reviewed changes

0xc0170 added the ready for merge label May 20, 2018

cmonr merged commit 80f228a into ARMmbed:feature-emac May 21, 2018

cmonr removed the ready for merge label May 21, 2018

kjbracey mentioned this pull request May 22, 2018

Merge feature-emac branch into master #6847

Merged

Add NetworkInterface::get_default_instance() #6855

Add NetworkInterface::get_default_instance() #6855

Uh oh!

Conversation

kjbracey commented May 9, 2018

Description

Pull request type

Uh oh!

cmonr commented May 9, 2018

Uh oh!

cmonr commented May 9, 2018

Uh oh!

cmonr commented May 9, 2018

Uh oh!

kjbracey commented May 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey commented May 17, 2018

Uh oh!

TeroJaasko left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented May 17, 2018

Uh oh!

mbed-ci commented May 17, 2018

Triggering tests

Uh oh!

mbed-ci commented May 18, 2018

Uh oh!

mbed-ci commented May 18, 2018

Uh oh!

cmonr commented May 18, 2018

Uh oh!

mbed-ci commented May 18, 2018

Uh oh!

kjbracey commented May 18, 2018

Uh oh!

kjbracey commented May 18, 2018

Uh oh!

mbed-ci commented May 18, 2018

Uh oh!

0xc0170 commented May 18, 2018

Uh oh!

mbed-ci commented May 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kjbracey May 21, 2018 •

edited

Loading