ONME-2857: Ethernet interface for Nanostack

[SeppoTakalo]

Description

• Refactored mesh-api classes so to get rid of duplicate classes that serve same purpose.
• Moved code to LoWPANNDInterface and ThreadInterface classes, so that if 6LoWPAN is not enabled, linker will drop its internal tasklets.
• Refactored NanostackRfPhy to NanostackPhy and provided NanostackEthernetPhy.
• Provided NanostackEthernetInterface so applications can use Nanostack over Ethernet. (This was the main goal)

Status

READY

Migrations

No changes to public APIs, except new NanostackEthernetInterface class provided.

Todos

• Tests
• Documentation

Tests

Tested with K64F using different radios and connectivity options. Application used for testing was mbed-os-example-mesh-minimal
When using Ethernet, the app was modified to use the Ethernet instead of 6LoWPAN or Thread. Example Ethernet driver is here: https://github.com/ARMmbed/sal-nanostack-driver-k64f-eth/tree/nanostackphy

Modified basically like this:

NanostackEthernetInterface eth;
NanostackEthernetPhyK64F phy;
 
main() {
    ...
    eth.initialize(&phy);
    eth.connect();
...

After joining the network, connectivity was verified by sending ping requests to its IPv6 address.

Interface/Driver	MCR20A	Atmel AT86RF233	K64F Ethernet
LoWPANNDInterface	OK	OK	-
ThreadInterface	OK	OK	-
NanostackEthernetInterface	-	-	OK

@kjbracey-arm @mikaleppanen @hasnainvirk Please review.

[SeppoTakalo]

How 6LoWPAN and Thread interfaces now look

[kjbracey]

Like it on the whole, but I think we do need to do the class shenanigans so that NanostackEthernetInterface is an EthInterface, not a MeshInterface.

Currently matters not a jot, given that both those are empty classes derived from NetworkInterface, but it will cause future trouble the moment someone puts any methods in either of those, and wants to switch between an LWIP "EthernetInterface" and our "NanostackEthernetInterface".

[SeppoTakalo]

The problem is that actually all the logic that starts nanostack when its interface is instantiated lives in MeshInterfaceNanostack and therefore it provides the NetworkStack *get_stack() and some common logic between all Nanostack interfaces.

If we still want to inherit EthInterface directly, probably MeshInterfaceNanostack should directly inherit NetworkInterface like this:

this becames messy... Perhaps we need to rethink and refactor Nanostack's Interface and Stack inheritance later.
At least they cannot directly inherit NanostackInterface (which is actually Stack not Interface) because it is a singleton.

[hasnainvirk]

I think mbed-mesh-api is not just a mesh-api anymore. IMO its time to retire mbed-mesh-api. As an Ethernet Interface is also added, it should be called as mbed-ns-api where ns stands for nanostack.

[hasnainvirk]

I know it will sound confusing to refactor MeshInterfaceNanostack() as InterfaceNanostack() as we already have a NanostackInterface()

How about NanostackInterfaceFactory() or NanostackInterfaceAdaptor() ?
Use of word in mesh while describing Ethernet is confusing.

[hasnainvirk]

Other than that, it looks pretty nice. However, in Diagram 2, I think NanostackEthernetINterface does not inherit EthInterface. Actually it inherirts MeshInterfaceNanostack only.

[SeppoTakalo]

OK. Good feedback.
Seems that we need to have follow up tasks to refactor all mesh-api related classes. Even this refactoring was unplanned and out of scope. I just did it to make things clearer (and save 3kB of flash).

If all agree, I think this is now good to go in, and I will need follow up task in backlog to continue working on this (but not a scope for 5.3 release).

@0xc0170 @sg- From my point of view, this is now good to go in.

[0xc0170]

/morph test-nightly

[0xc0170]

@bridadan Can you pls abort the CI?

@SeppoTakalo Is this PR ready for review/integration?

[mbed-bot]

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1103

Test Prep failed!

[SeppoTakalo]

I added one more fix that I just found.

There was a problem when querying DNS addresses using Nanostack. It always defaulted to IPv4.

So actually the problem was that SocketAddress::_SocketAddress(*addr,port) did a NetworkStack::gethostbyname() query with address type NSAPI_UNSPEC. Then NetworkStack::gethostbyname() tried to guess a IP address version by first asking a network stack of its own IP address by calling NetworkStack::get_ip_address() and then created a new SocketAddress class from the returned string. Then checked its version.

Problem is that network stacks don't have addresses. Only interfaces have addresses. Therefore Nanostack::get_ip_address() returned NULL and the SocketAddress::set_ip_address() failed and defaulted to IPv4.
Fixed by returning localhost address.

This is ready for review now. I can run valid DNS query. Sorry for inconveniently sending a bugfix into this same branch.

[SeppoTakalo]

@0xc0170 Ready for review and integration.

[bridadan]

Sorry for the failure y'all, the test network has been experiencing big slow downs for GitHub cloning this week. I've increased the timeouts on all the clone steps in the meantime while these slow downs are occurring.

/morph test-nightly

[kjbracey]

Seems like we should either do a little better for the get_ip_address(), or try to get it deprecated.

Trying to get some sort of valid address for a stack doesn't seem unreasonable as a utility function - search interfaces belonging to that stack for a global address? Could this be done in the generic layers?

Not for this PR though. That fudge is an incremental improvement - noting that it isn't actually "localhost", its "IPv6 unspecified". I think I do prefer unspecified to localhost ("::1"). (Micronit: returning a literal "::" might also be better - it can save ROM by addressing tricks and string sharing. You don't actually need a unique "::" string array, which that gives you.

[SeppoTakalo]

@bridadan
Testjobs are not reporting back. http://mbed-ci-master-2.austin.arm.com:8081/job/pr_pipeline/1107/artifact/github_comment_text.md/*view*/

mbed Build Number: 1107

All builds and test passed!

[bridadan]

@SeppoTakalo Haha you've figured out how to take my bot's place! 😄 Yesterday we had an issue where the CI server's power cord was kicked so the bot lost track of all the jobs. Shouldn't be an issue anymore.

[0xc0170]

@bridadan Shall we restart tests?

[bridadan]

@0xc0170 Shouldn't need to as the tests actually completed fine, they just didn't change the GitHub status. If you click the "Details" link next to the "pending" check, you can see everything completed successfully.

[adbridge]

@SeppoTakalo do you wish to address @kjbracey-arm comment about returning a string literal? Or is this ready to just be merged ?

[SeppoTakalo]

@adbridge This is ready.

We can address Kevin's proposal on next round. Functionality will be the same anyway.