Created PPP interface and PPP service classes to netsocket #10974

mikaleppanen · 2019-07-05T12:43:25Z

Description

Created PPP interface and PPP service classes to netsocket. This allows that both lwip and nanostack can use a common PPP service for cellular connectivity. Lwip PPP protocol is separated from lwip and moved under netsocket as an independent entity. PPP offers much similar interface for (onboard) network stack as the EMAC and L3IP. PPPInterface class has been created as similar helper class as the EMACInterface and L3IPInterface are.

The "nsapi_ppp.h" interface is kept same, and the implementation for it is in ppp service file "ppp_nsapi.cpp". Cellular classes using the "nsapi_ppp.h" interface should not need updates. Existing ppp configuration under lwip mbed_lib.json is still supported, although configuration under ppp service should be used instead.

Most major changes to internal structure of PPP were, to replace its interface to lwip netif with new one and to make PPP to use network stack's memory manager for memory allocations, instead of direct access to lwip memory buffers.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

Add Cellular PPP connection support to Nanostack. Support is made by creating a common PPP service class that both lwIP and Nanostack can use for cellular connectivity.

Affected components are lwIP and Nanostack on board IP stacks, new netsocket PPP interface and PPP service class. Cellular network interface, cellular classes and "nsapi_ppp.h" interface are not modified. No changes are required from the users of the Cellular network interface to continue using the lwIP as the default IP stack.

To use the Nanostack IP stack with the Cellular network interface instead of the lwIP stack, cellular application configuration must be modified in following way:

Set the default stack to nanostack

"nsapi.default-stack": "NANOSTACK"

Configure PPP to IPv6 only mode (Nanostack does not support IPv4)

"ppp.ipv4-enabled": false, "ppp.ipv6-enabled": true,

mikaleppanen · 2019-07-05T12:54:08Z

Although not ready for official review, comments appreciated.
@mikter @artokin @kjbracey-arm @AnttiKauppila @kivaisan @AriParkkila

ciarmcom · 2019-07-05T15:00:41Z

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

mikaleppanen · 2019-08-06T14:26:16Z

Rebased to master and updated with latest changes. Pull request is ready for review.

@mikter @artokin @kjbracey-arm @AnttiKauppila @kivaisan @AriParkkila
@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers

kivaisan · 2019-08-07T11:42:29Z

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

+    if (address_type != PHY_MAC_64BIT) {
+        return -1;
+    }
+    //memcpy(unique_id, address_ptr, 8);


Are these commented codes needed?

Removed the write function since not valid for ppp.

kivaisan · 2019-08-07T11:43:41Z

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

+        phy.PHY_MAC = iid64;
+        phy.address_write = ppp_phy_address_write;
+        phy.driver_description = const_cast<char *>("PPP");
+        phy.link_type = static_cast<phy_link_type_e>(5); // PHY_LINK_PPP To compile without nanostack changes


Is there some bigger reason not to define PHY_LINK_PPP in phy_link_type_e?

Added the constant to phy_link_type_e.

kivaisan · 2019-08-07T11:45:00Z

features/netsocket/PPP.h

+     * @param uname User name
+     * @param passwrd Password
+     */
+    virtual void set_uname_password(const char *uname, const char *password) = 0;


Cellular has used set_credentials name for these.

kivaisan · 2019-08-07T11:48:29Z

features/netsocket/ppp/ppp.c

@@ -927,8 +905,8 @@ void ppp_input(ppp_pcb *pcb, struct pbuf *pb) {
       */
      for (i = 0; (protp = protocols[i]) != NULL; ++i) {
        if (protp->protocol == protocol) {
-          pb = pbuf_coalesce(pb, PBUF_RAW);
-          (*protp->input)(pcb, (u8_t*)pb->payload, pb->len);
+          ///////// pb = pbuf_coalesce(pb, PBUF_RAW);


Why commented out?

Removed as not needed (always a single buffer on this stage).

artokin · 2019-08-09T08:57:38Z

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

+
+void PPPPhy::get_mac_address(uint8_t *mac)
+{
+}


Are get_mac_address and set_mac_address left intentionally empty? If so then comment would be nice.

Refactored nanostack phy classes so that mac address get/set are no longer part of the base class.

artokin · 2019-08-09T09:04:20Z

features/netsocket/PPPInterface.cpp

@@ -0,0 +1,193 @@
+/*
+ * Copyright (c) 2018 ARM Limited


Please update copyrigth year in all new files

artokin · 2019-08-09T09:06:32Z

features/netsocket/PPPInterface.cpp

+
+void PPPInterface::set_as_default()
+{
+#if 0


Please add comment why PPPINterface can't be set as default interface

Removed the "if 0" and casting to lwip stack.

mikaleppanen · 2019-08-09T12:02:15Z

Updated pull request with latest changes. Ready for review.

kjbracey

Architecturally, I'm extremely impressed. This pulls together a whole bunch of Mbed OS concepts that have been partially formed since 5.7 and 5.9, and extends them very elegantly to cover this generic PPP engine,

The whole set-up looks fantastic now, and OnboardNetworkStack is beautiful.

My only real concern is the forking out of the PPP core from lwip, and what the plan is to track any upstream lwIP PPP changes in future. Do we have the tooling to do that elegantly? I guess git subtree squash might still manage to handle the file renamings - it's just a merge, ultimately, and that can sometimes...?

VeijoPesonen · 2019-08-14T06:34:19Z

features/netsocket/NetworkInterface.h

@@ -338,6 +339,14 @@ class NetworkInterface: public DNS {
        return 0;
    }

+    /** Return pointer to a MeshInterface.


PPPInterface

VeijoPesonen · 2019-08-14T06:43:35Z

features/netsocket/PPPInterface.h

+     *
+     *  Requires that the network is disconnected
+     *
+     *  @param dhcp     False to disable dhcp (defaults to enabled)


Does this actually have a default argument?

Removed whole function from PPP interface since dhcp is not valid for ppp. It is set false on stack bring up.

VeijoPesonen · 2019-08-14T06:56:44Z

features/netsocket/PPPInterface.h

+
+    mbed::FileHandle *_stream;
+    nsapi_ip_stack_t _ip_stack;
+    char _uname[30];


Should a define be used instead?

Chained user name and password to pointers inside ppp interface class (similar to cellular interface).

VeijoPesonen · 2019-08-14T07:46:20Z

features/netsocket/ppp/source/auth.c

-#include <netdb.h>
-#include <netinet/in.h>
-#include <arpa/inet.h>
+//#include <netdb.h>


Should be removed?

mikaleppanen · 2019-08-14T11:18:56Z

@kivaisan @VeijoPesonen Corrected review defects. Could you check that they are ok.

mikaleppanen · 2019-08-15T08:44:11Z

All review comments handled.
Tidied up the commit history by merging corrections found by review and hardware testing to original commits.
Cellular automated greentea hardware tests and manual tests are passing for this PR.
@AriParkkila / @AnttiKauppila is the review ok? If it is, then I think PR should enter testing phase.

mikaleppanen · 2019-08-16T07:39:55Z

@ARMmbed/mbed-os-maintainers can this progress?

artokin · 2019-08-19T09:05:19Z

Changes approved, CI started.

0xc0170

[x] Functionality change

Please add release notes section describing this in detail for users

mbed-ci · 2019-08-19T10:54:16Z

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_build-GCC_ARM

mikaleppanen · 2019-08-19T12:40:37Z

Corrected compilation errors.

artokin · 2019-08-19T13:04:18Z

CI started

mbed-ci · 2019-08-20T00:11:34Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

Moved PPP and renamed files and constants.

Created PPP service class that encapsulates the PPP protocol. Class is similar to EMAC and L3IP classes with additional methods to read IP and DNS server addresses negotiation using PPP and to set PPP specific parameters (file handle for modem access etc.). PPP service can use on its own thread or in run in mbed os event Queue thread. Added ppp_nsapi.cpp module that implements the nsapi_ppp.h services. Added ppp_nsapi.cpp module that implements the nsapi_ppp.h services.

PPP service encapsulates the PPP protocol. PPP interface can be used as helper class to bind PPP protocol with network stack (similar to EMAC and L3IP interface). Added PPP interface to onboard network stack class.

Created (a new) PPP interface for PPP service. Removed lwip dependencies to PPP (memory allocations etc.). Moved PPP configuration options away from lwIP mbed_lib.json to new PPP service. For backwards compatibility, using the old options is also currently supported.

Created PPP interface for PPP service. Re-used the ethernet tasklet and PHY driver structure for PPP.

…re words

If PPP interface is the lwIP default interface, adds the PPP DNS servers to default DNS server storage. If PPP is not default interface, then adds DNS servers to interface specific storage.

Increased stack size from 768 to 816.

Corrected PPP thread stack size for RZ_A1_EMAC, CYW943012P6EVB_01, CY8CPROTO_062_4343W, CY8CKIT_062_WIFI_BT and CY8CKIT_062S2_43012 that have special configuration for PPP thread size. Removed pppInterface() helper call from network interface. It causes binary compatibility break with precompiled network interface classes. Call is helper function to check network interface type in case it is unknown, and is not mandatory or used with PPP.

0xc0170 · 2019-08-20T11:03:21Z

Recent merges caused the conflict, please rebase and we shall restart CI asap

artokin · 2019-08-20T12:15:12Z

CI started

mbed-ci · 2019-08-20T16:10:14Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

mikaleppanen mentioned this pull request Jul 5, 2019

Added support for Cellular PPP interface PelionIoT/nanostack-border-router#182

Merged

ciarmcom requested review from a team July 5, 2019 15:00

ciarmcom added the needs: review label Jul 5, 2019

0xc0170 added the do not merge label Jul 9, 2019

kivaisan reviewed Aug 7, 2019

View reviewed changes

artokin reviewed Aug 9, 2019

View reviewed changes

artokin requested review from mikter, kjbracey, AnttiKauppila and AriParkkila August 9, 2019 10:40

artokin removed the do not merge label Aug 9, 2019

kjbracey approved these changes Aug 13, 2019

View reviewed changes

mikter approved these changes Aug 13, 2019

View reviewed changes

VeijoPesonen reviewed Aug 14, 2019

View reviewed changes

VeijoPesonen approved these changes Aug 14, 2019

View reviewed changes

kivaisan approved these changes Aug 14, 2019

View reviewed changes

AnttiKauppila approved these changes Aug 16, 2019

View reviewed changes

0xc0170 added the release-version: 5.14.0-rc1 label Aug 19, 2019

0xc0170 requested changes Aug 19, 2019

View reviewed changes

KariHaapalehto and others added 11 commits August 20, 2019 13:27

Moved PPP from lwip directory to under netsocket

c518319

Moved PPP and renamed files and constants.

Created netsocket classes for PPP service and PPP interface

c9cb8f8

PPP service encapsulates the PPP protocol. PPP interface can be used as helper class to bind PPP protocol with network stack (similar to EMAC and L3IP interface). Added PPP interface to onboard network stack class.

Nanostack changes for PPP service

a2df462

Created PPP interface for PPP service. Re-used the ethernet tasklet and PHY driver structure for PPP.

Removed pbuf operations that were commented out

3102bb1

Added PPP directory to astyle ignore and added some spellchecker igno…

e3f2477

…re words

lwIP adds now PPP DNS servers to default interface

3951422

If PPP interface is the lwIP default interface, adds the PPP DNS servers to default DNS server storage. If PPP is not default interface, then adds DNS servers to interface specific storage.

Increased PPP stack size based on testing

3b20d9e

Increased stack size from 768 to 816.

Added missing header to wisun interface

ab3a469

0xc0170 approved these changes Aug 20, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Aug 20, 2019

artokin added needs: CI and removed needs: work labels Aug 21, 2019

0xc0170 added ready for merge and removed needs: CI labels Aug 21, 2019

0xc0170 merged commit 399b517 into ARMmbed:master Aug 21, 2019

0xc0170 removed the ready for merge label Aug 21, 2019

Created PPP interface and PPP service classes to netsocket #10974

Created PPP interface and PPP service classes to netsocket #10974

Uh oh!

Conversation

mikaleppanen commented Jul 5, 2019 • edited by 0xc0170 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

mikaleppanen commented Jul 5, 2019

Uh oh!

ciarmcom commented Jul 5, 2019

Uh oh!

mikaleppanen commented Aug 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kivaisan Aug 7, 2019 • 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!

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!

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!

mikaleppanen commented Aug 9, 2019

Uh oh!

kjbracey 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaleppanen commented Aug 14, 2019

Uh oh!

mikaleppanen commented Aug 15, 2019

Uh oh!

mikaleppanen commented Aug 16, 2019

Uh oh!

artokin commented Aug 19, 2019

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

mbed-ci commented Aug 19, 2019

mikaleppanen commented Jul 5, 2019 •

edited by 0xc0170

Loading

kivaisan Aug 7, 2019 •

edited

Loading