Networking update: general refactoring, unifying EMAC

[kjbracey]

Here's the next pass of #5202, with a major "spelling" rework into C++ following discussions with @sg-.

No huge conceptual changes, but the C++-ification does open up the flexibility. I think that's best summarised by EthernetInterface's constructor prototype:

EthernetInterface(
        EMAC &emac = EMAC::get_default_instance(),
        OnboardNetworkStack &stack = OnboardNetworkStack::get_default_instance());

This is a pull request to feature-emac, with only the K64F driver transformed to the new API, so won't build for any other lwIP-based platform yet. It will be some time until the feature branch can be merged - it's a compatibility break for all lwIP and emac drivers.

A number of notes in the commit message.

Nice diagrams to follow shortly.

---

The network stack

OnboardNetworkStack

OnboardNetworkStack is an abstract class specialising the existing NetworkStack, which is the application-facing thing that provides sockets. In the initial implementation there will be two of these: LWIP and Nanostack. LWIP is present in this PR.

The main extra method is

virtual nsapi_error_t OnboardNetworkStack::add_ethernet_interface(
                   EMAC &emac, bool default_if, Interface **interface_out) = 0;

This allows an EMAC-based driver to attach itself to a stack - the call gives back an OnboardNetworkStack::Interface, which is the stack's "handle" for this interface.

To locate the default stack, we have

static OnboardNetworkStack &OnboardNetworkStack::get_default_instance();

This will return either a LWIP or Nanostack (or other) object, depending on a JSON setting. By using get_default_instance() in a default function parameter, we can ensure the default stack is pulled in by the linker only if used.

OnboardNetworkStack::Interface

This provides methods to support EthernetInterface or any other EMAC-based driver providing a NetworkInterface, which is the application API.

NetworkInterface covers interfaces which can either be using an onboard stack or an offboard one. For those using an onboard stack, most of the work can be deferred to the OnboardNetworkStack::Interface.

Via this C++ objectification, the LWIP adaptation glue naturally becomes multiple-interface - all its static information is now resident in the LWIP::Interface objects created by LWIP::add_ethernet_interface - see next section.

Application-facing network drivers

EthernetInterface

EthernetInterface is now the prototypical "just an EMAC" driver, implementing a NetworkInterface using an EMAC and an OnboardNetworkStack. There's no actual code involved - it derives from EMACInterface, which provides the base implementation.

Construction forms are:

• EthernetInterface net; - default stack, default EMAC
• EthernetInterface net(my_emac); - default stack, specified EMAC
• Ethernetinterface net(my_emac, LWIP::get_instance()); - LWIP, specified EMAC

EthernetInterface::connect is basically

 if (!interface) {
     _stack.add_ethernet_interface(_emac, true, &_interface);
 }
 _interface->bringup(...)

And other bits are simple glue like:

const char *EthernetInterface::get_gateway()
{
   _interface->get_gateway(_gateway, sizeof(_gateway));
   return _gateway;
}

NetworkStack *EthernetInterface::get_stack()
{
    return &_stack;
}

Other EMAC-based NetworkInterfaces

EMACInterface is a "base" EMAC NetworkInterface, from which EMAC Wi-Fi drivers can derive, rather than providing the NetworkInterface->OnboardNetworkStack::Interface glue themselves, as they currently do for NetworkInterface->mbed_lwip_xxx.

They can provide their own overridden connect methods to configure their MAC, which do initial set-up and then call EMACInterface::connect() to bring up the network stack.

EMAC drivers

The EMAC API is respelt into C++ without significant change - an EMAC driver must now supply an implementation of the abstract EMAC class. This replaces emac_interface_t and emac_interface_ops_t, in a way that allows full stack/interface implementation independence.

Memory API

The memory API used by EMAC drivers (emac_stack_mem.h) is not yet changed in this PR. PR#5750 transforms it so that the OnboardNetworkStack provides gives the EMAC reference to a EMACMemoryManager object providing the facilities. This is necessary to permit two stacks to co-exist in the tree without FEATURE_XXX flagging.

Targets

To permit plain EthernetInterface to work without parameters, a target must implement the factory method EMAC::get_default_instance(), eg:

// Weak so a module can override
MBED_WEAK EMAC &EMAC::get_default_instance() {
    return K64F_EMAC::get_instance();
}

My thought is here that a target would provide this weakly, so a module or application could override it at normal strength.

Flagging

DEVICE_EMAC is retained to indicate that EMAC::get_default_instance() is provided by the target. It may be tested by target-specific code to check whether to compile its driver - eg k64f_emac.cpp checks it so it only compiles on relevant Freescale platforms.

DEVICE_EMAC should not be set other than by the target - eg by a module providing EMAC::get_default_instance(). And it is always possible to use EthernetInterface and EMACInterface with a manually specified EMAC.

Question - do we need a flag to indicate EMAC::get_default_instance() is provided at a higher level, eg to gate building tests?

[theotherjimmy]

@kjbracey-arm Review assignments are up to you

[kjbracey]

Quite a few people should look at this. I can think of:

@sg-, @geky, @c1728p9, @bulislaw, @SeppoTakalo, @mikaleppanen, @0xc0170

What I want reviewed at this point is the concept of the new OnboardNetworkStack, OnboardNetworkStack::Interface and EMAC classes and how they're used in EthernetInterface, more than the specific implementations of LWIP, LWIP::Interface and K64F_EMAC. The generic API needs to be locked down before further work proceeds.

Also check if we're happy with the mechanism for default EMAC and stack selection.

Our current assumption is that the emac_mem api will also be transformed into an abstract OnboardNetworkStack::MemoryMgr - at the moment it's untouched.

Rather hard to see what's going on in a GitHub web diff. You can get a clearer diff result in Git itself if you do a diff with -M25% to set the rename detection threshold way down.

Might be easiest just look at the end result, rather than the patch.

I'll try and get some design documentation added to the description to help clarify this week.

Another general conceptual concern - does more C++ to drivers increase binary blob compatibility headaches excessively? (cf #5546) In this case though, even #4079's C EMAC tweaks was a binary break in various ways - the C++ may not make things qualitatively worse.

[pan-]

Another general conceptual concern - does more C++ to drivers increase binary blob compatibility headaches excessively? (cf #5546) In this case though, even #4079's C EMAC tweaks was a binary break in various ways - the C++ may not make things qualitatively worse.

Maintaining binary compatibility may be difficult as virtual member functions are part of a data structure: the vtable. The pimpl idiom¹ ² may be used to work around this issue but it is not applicable in our case because it allocates memory. There is a good document in KDE Wiki that sumarize do's and dont's; the itanium ABI might be consulted to resolve all ambiguities.

Is there a commitment to maintain binary compatibility in mbed OS ? RTX updates, cmsis updates, vendor SDK updates may just break it and it is out of our control with the current architecture.

[SeppoTakalo]

It is not out of our hands when we merge in those changes.
So we should maintain binary compatibility as well as unchanged API between all patch versions.
Then changes can happen on major releases, but they need communication beforehand.

[pan-]

@sg- Do we have a commitment to maintain binary - and not just API - compatibility in mbed OS ?

[SeppoTakalo]

We do have binary drivers within the source tree so how can we not commit to support them.

[pan-]

@SeppoTakalo It is a software dependency problem. If the binary driver doesn't use mbed os APIs then it won't suffer of any binary compatibility issue. However if the binary driver exploits mbed OS APIs then breaking mbed OS binary compatibility may affect the binary driver; that has been seen in #5546 .

A clear answer on binary compatibility rather than guesses might help us. Binary compatibility was broken in the past (like during the RTX2 update); it was known and it didn't resulted in major version bump.

[kjbracey]

Realistically I think we'll inevitably hit some binary breakage on pretty much every minor version bump, and can endeavour to avoid it on patches with a little more care.

Achieving more than that would require a significant redesign and much more care about the C++ - I'd say anyone using binary blobs should be prepared to recompile on each minor update.

[0xc0170]

There is a good document in KDE Wiki that sumarize do's and dont's; the itanium ABI might be consulted to resolve all ambiguities.

Very good guide, always visit it when in doubt

Realistically I think we'll inevitably hit some binary breakage on pretty much every minor version bump, and can endeavour to avoid it on patches with a little more care.

Patches should not have any breakages, period. @SeppoTakalo noted above in #5558 (comment) nicely.

A clear answer on binary compatibility rather than guesses might help us. Binary compatibility was broken in the past (like during the RTX2 update); it was known and it didn't resulted in major version bump.

If that was the case, that should have been sent to minor. We had cases when we broke it 😞 We should document this better .

We are drifting away from networking update here, we can create a new issue to continue there.

[c1728p9]

Should there be an option to have an allocator per network stack? If not, this API will force all network stacks to use lwip's pbuf mechanism (or copy the data). From your pr description this will be changing anyway.

[kjbracey]

Yes, allocator absolutely needs to be per-stack, or this won't take off. The current mem API will need a little generalisation and "what exactly is this call specified to do?" work anyway, so we've yet to tackle it.

That's in contrast to the other APIs which remain largely unchanged apart from being objectified and C++ified.

I guess we stick with the assumption that each interface is only being used by 1 stack at once, so the interface knows how to pre-allocate its RX buffers.

(In the other system I did this stuff we did permit multiple stacks (IP/NetBIOS/Appletalk/...) to use an interface simultaneously, but everyone was forced to use the same central allocator.)

[c1728p9]

A clear answer on binary compatibility rather than guesses might help us.

There is no requirement to maintain binary compatibility in mbed-os. If you are using mbed-os's public API from an application or library, you must recompile your binary if you are updating to a newer version of mbed-os.
CC @sg-

Each Driver class (Serial, SPI , I2C,...) has vendor specific structures (serial_s, spi_s, i2c_s,...), which can be changed at any time by vendors. If the code using these classes is out of date then it will not work properly, as the offsets are different.

That being said, some libraries in mbed-os are called from mbed, but do not call into or use the mbed-os API. For example the binary form of nanostack did not need to be re-compiled on each mbed-os release because it didn't call into the mbed-os API. Instead that was done from source which wrapped nanostack.

[kjbracey]

Added a second commit which might allow Wifi drivers to reduce boilerplate - inherit from "EMACInterface" to handle all the core stack attachment stuff and then they only need to implement the WiFi-specific connect/credential/scan methods from "WiFiInterface".

Does mean a diamond virtual inheritance pattern. Too gnarly?

[kjbracey]

@0xc0170 - it appears I rebased this, inadvertantly.

Could you fast-forward feature-emac to 2e1c2a1 to clean up the view here?

[0xc0170]

Could you fast-forward feature-emac to 2e1c2a1 to clean up the view here?

Done, ff to the latest master (I could do the specific commit but should not matter here, to get the latest one?), let me know if that is a problem.

[kjbracey]

Thanks!

That didn't seem to clear up the display at first - don't know why - but did another rebase and it's fine.

[adbridge]

@kjbracey-arm Looks like another rebase is needed.
Also we are missing reviews from @sg-, @geky, @bulislaw, @SeppoTakalo, @mikaleppanen, @0xc0170

[cmonr]

/morph build

[mbed-ci]

Build : SUCCESS

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

Triggering tests

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

[cmonr]

Let's see here...
/morph uvisor-test

[mbed-ci]

Exporter Build : SUCCESS

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

[mbed-ci]

Test : FAILURE

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

[kjbracey]

Connect twice failures hopefully resolved now. Was a real bug introduced to the lwip connect logic by the C++-ification.

[0xc0170]

/morph build

[mbed-ci]

Build : SUCCESS

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

Triggering tests

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

[mbed-ci]

Test : SUCCESS

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

[mbed-ci]

Exporter Build : SUCCESS

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

[0xc0170]

All green ! This is now ready for integration to the feature branch