Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Created PPP interface and PPP service classes to netsocket #10974

Merged
merged 11 commits into from Aug 21, 2019

Conversation

@mikaleppanen
Copy link
Contributor

commented Jul 5, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

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

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Jul 5, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

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

@0xc0170 0xc0170 added the do not merge label Jul 9, 2019
@mikaleppanen mikaleppanen force-pushed the mikaleppanen:nano_ppp branch from 1ac1765 to 45d3d4f Aug 6, 2019
@mikaleppanen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

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

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

This comment has been minimized.

Copy link
@kivaisan

kivaisan Aug 7, 2019

Contributor

Are these commented codes needed?

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Aug 12, 2019

Author Contributor

Removed the write function since not valid for ppp.

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

This comment has been minimized.

Copy link
@kivaisan

kivaisan Aug 7, 2019

Contributor

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

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Aug 12, 2019

Author Contributor

Added the constant to phy_link_type_e.

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

This comment has been minimized.

Copy link
@kivaisan

kivaisan Aug 7, 2019

Contributor

Cellular has used set_credentials name for these.

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Aug 12, 2019

Author Contributor

Renamed.

@@ -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);

This comment has been minimized.

Copy link
@kivaisan

kivaisan Aug 7, 2019

Contributor

Why commented out?

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Aug 12, 2019

Author Contributor

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


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

This comment has been minimized.

Copy link
@artokin

artokin Aug 9, 2019

Contributor

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

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Aug 9, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@artokin

artokin Aug 9, 2019

Contributor

Please update copyrigth year in all new files

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Aug 12, 2019

Author Contributor

Corrected


void PPPInterface::set_as_default()
{
#if 0

This comment has been minimized.

Copy link
@artokin

artokin Aug 9, 2019

Contributor

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

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Aug 12, 2019

Author Contributor

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

@mikaleppanen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Updated pull request with latest changes. Ready for review.

@mikaleppanen mikaleppanen force-pushed the mikaleppanen:nano_ppp branch 3 times, most recently from 679c1f2 to 122c2db Aug 12, 2019
Copy link
Contributor

left a comment

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...?

@mikter
mikter approved these changes Aug 13, 2019
@@ -338,6 +339,14 @@ class NetworkInterface: public DNS {
return 0;
}

/** Return pointer to a MeshInterface.

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Aug 14, 2019

Contributor

PPPInterface

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Aug 14, 2019

Author Contributor

Corrected

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

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Aug 14, 2019

Contributor

Does this actually have a default argument?

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Aug 14, 2019

Author Contributor

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


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

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Aug 14, 2019

Contributor

Should a define be used instead?

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Aug 14, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Aug 14, 2019

Contributor

Should be removed?

This comment has been minimized.

Copy link
@mikaleppanen

mikaleppanen Aug 14, 2019

Author Contributor

Removed.

@mikaleppanen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

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

@mikaleppanen mikaleppanen force-pushed the mikaleppanen:nano_ppp branch from a38c0a4 to e75cdf0 Aug 15, 2019
Copy link
Member

left a comment

[x] Functionality change

Please add release notes section describing this in detail for users

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Corrected compilation errors.

@artokin

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 20, 2019

Test run: SUCCESS

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

KariHaapalehto and others added 11 commits May 7, 2019
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.
Increased stack size from 768 to 816.
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.
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 0xc0170 added needs: work and removed needs: review labels Aug 20, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

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

@mikaleppanen mikaleppanen force-pushed the mikaleppanen:nano_ppp branch from f90a2f2 to 4e60d2f Aug 20, 2019
@artokin

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 20, 2019

Test run: SUCCESS

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

@artokin artokin added needs: CI and removed needs: work labels Aug 21, 2019
@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 21, 2019
@0xc0170 0xc0170 merged commit 399b517 into ARMmbed:master Aug 21, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8692 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.