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

Support cellular IPv4v6 dual stack in LWIP #4911

Merged
merged 10 commits into from Sep 4, 2017

Conversation

Projects
None yet
9 participants
@mikaleppanen
Contributor

mikaleppanen commented Aug 15, 2017

Description

Cellular IPv4v6 dual stack support to lwip.

  • Merged ipv6 address auto negotiation corrections from lwip master branch, which prevented ipv6 cellular connection from working (lwip did not work correctly, if RA prefix had auto negotiation flag set, but on-link flag was not set).
  • Made correction to lwip to disable ipv6 neighbor solidification probes, if link layer address is not available.
  • Disabled duplicated address detection for cellular ipv6.
  • If cellular dual stack support is enabled, tries first cellular context activation for both ipv4v6, and if context activation is not successful, falls back to ipv4 context.
  • If context is activated for ipv4v6, makes PPP negotiation with both IPCP and IPV6CP protocols, and configures both ipv4 and ipv6 addresses.
  • If context is only for ipv4, makes PPP negotiation with IPCP protocol, and configures ipv4 address.
  • If both ipv4 and ipv6 addresses are configured, when DNS address resolution is made, tries to resolve first the preferred stacks address. If address resolution fails, tries to resolve non-preferred stack address.
  • Added trace dump function that can be used to trace PPP, ipv4 and ipv6 packets in format that can be imported to Wireshark.
  • Moved lwip memory pools to second memory bank for UBLOX_C027.
  • Corrected lwip random and tcp isn not to include configuration headers.

Status

READY

Migrations

YES

nsapi_ppp.h function nsapi_ppp_connect(): Added "const nsapi_ip_stack_t stack" parameter
-> can be used in dual stack case to set either ipv4v6 or ipv4 stack, for non-dual
stack case use value DEFAULT_STACK.

ppp_lwip.h function ppp_lwip_if_init(): Added "const nsapi_ip_stack_t stack" parameter
-> can be used in dual stack case to set either ipv4v6 or ipv4 stack, for non-dual
stack case use value DEFAULT_STACK.

lwip_stack.h function mbed_lwip_bringup_2(): Added "const nsapi_ip_stack_t stack" parameter
-> can be used in dual stack case to set either ipv4v6 or ipv4 stack, for non-dual
stack case use value DEFAULT_STACK.

Related PRs

None

Todos

  • Tests
  • Documentation

Deploy notes

None

Steps to test or reproduce

None

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 15, 2017

ip6_addr_set_allnodes_linklocal(&ip6_allnodes_ll);
lwip_netif.mld_mac_filter(&lwip_netif, &ip6_allnodes_ll, NETIF_ADD_MAC_FILTER);
}
/*

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 15, 2017

Contributor

Formatting

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 18, 2017

Contributor

just moved the indentation when if was added, no problem there.

features/netsocket/cellular/generic_modem_driver/PPPCellularInterface.cpp Outdated
@@ -262,6 +262,7 @@ PPPCellularInterface::PPPCellularInterface(FileHandle *fh, bool debug)
_pwd = NULL;
_fh = fh;
_debug_trace_on = debug;
_stack = IPV4_STACK;

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 15, 2017

Contributor

Shouldn't this be IPV4V6_STACK ?

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 18, 2017

Contributor

I set the value to default stack. In that case flag configuration defines the stack.

features/netsocket/nsapi_ppp.h Outdated
*
* @return 0 on success, negative error code on failure
*/
nsapi_error_t nsapi_ppp_connect(FileHandle *stream, Callback<void(nsapi_error_t)> status_cb=0, const char *uname=0, const char *pwd=0);
nsapi_error_t nsapi_ppp_connect(FileHandle *stream, Callback<void(nsapi_error_t)> status_cb=0, const char *uname=0, const char *pwd=0, const nsapi_ip_stack_t stack=IPV4_STACK);

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 15, 2017

Contributor

Why default to IPv4 ? Isn't the whole point to have the stack dual configurable and choose at runtime which one is available ?

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 18, 2017

Contributor

Set also this to default stack.

#define PRINTPKT_SUPPORT 0
#define PAP_SUPPORT 0
#define VJ_SUPPORT 0
#define PRINTPKT_SUPPORT 0

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 15, 2017

Contributor

Formatting

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 18, 2017

Contributor

moved the indentation when longer define was added, no problem there.

#define IP_SOF_BROADCAST 0
#define IP_SOF_BROADCAST_RECV 0
#define IP_SOF_BROADCAST 0
#define IP_SOF_BROADCAST_RECV 0

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 15, 2017

Contributor

formating

#define MAXNAMELEN 64 /* max length of hostname or name for auth */
#define MAXSECRETLEN 64
#define MAXNAMELEN 64 /* max length of hostname or name for auth */
#define MAXSECRETLEN 64

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 15, 2017

Contributor

Formatting

@@ -16,7 +16,11 @@
"addr-timeout": {

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 15, 2017

Contributor

Is this left out for backward compatibility ? Why and how ? Isn't the lwipopts.h also updated ?

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 16, 2017

Contributor

"addr-timeout" is the amount of time we wait for preferred system address in case we have only non-preferred system address available.
"both-addr-timeout" is the time to wait that both systems addresses are available.

So the flags have different meaning and both are valid.

#define MAXNAMELEN 64 /* max length of hostname or name for auth */
#define MAXSECRETLEN 64
#define MAXNAMELEN 64 /* max length of hostname or name for auth */

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 15, 2017

Contributor

Formatting

#define PAP_SUPPORT 0
#define VJ_SUPPORT 0
#define PRINTPKT_SUPPORT 0
#define PAP_SUPPORT 0

This comment has been minimized.

@hasnainvirk

hasnainvirk Aug 15, 2017

Contributor

Formatting

@hasnainvirk

There will be issues while using dual stack config, as 'add_dns_addr()' function is still not changed in relation to this PR. This particular function calls dns_setserver() internally with same index , so essentially you will overwrite your previous config.

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 16, 2017

"Hasnain: there will be issues while using dual stack config, as 'add_dns_addr()".

Yes, this could be improved maybe so that in dual stack system the DNS resolution could fallback to non-preferred system if it does not for some reason work in more preferred. I'll test this a little to see
how it works.

There is no absolute need to modify the function since the IP address get function (mbed_lwip_get_ip_addr()) fetches either the preferred systems IP address or if not available the non-preferred. So a either ipv4 or ipv6 DNS server (google) will be always added to list if network has not provided DNS server.

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 16, 2017

Jenkins is failing in interface up/down/up/down test since variable holding the address status is not reset on bring down. I'll correct that.

@0xc0170 0xc0170 added the needs: work label Aug 17, 2017

@mikaleppanen mikaleppanen force-pushed the mikaleppanen:lwip_ipv4v6_ppp branch Aug 18, 2017

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 18, 2017

Review defects corrected.

I improved the DNS address addition functionality in case network does not provide DNS server addresses. Now in dual mode both ipv4 and ipv6 DNS addresses are added in order of preference defined by flag configuration.

Corrected IAR memory pools relocation for UBLOX_C027.

Corrected interface up/down/up/down error detected by Jenkins.

@mikaleppanen mikaleppanen force-pushed the mikaleppanen:lwip_ipv4v6_ppp branch Aug 18, 2017

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Aug 18, 2017

@mikaleppanen I like the changes you did to place some lwip stuff on .ethusbram section for IAR compiler. Great work.
Can you please check why CI is failing ? Is this related to RAAS unavailability or a genuine case ?

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 18, 2017

Yes, ble raas is down. Tools team is aware about it.

@theotherjimmy theotherjimmy changed the title from lwip cellular IPv4v6 dual stack support to Support cellular IPv4v6 dual stack in LWIP Aug 21, 2017

mikaleppanen added some commits Jul 28, 2017

Corrections to lwip ipv6 neighbor discovery
Merged lwip 2.0.2 stable path 1 from https://github.com/ARMmbed/lwip

Patch allows lwip to do autonomous address configuration for prefixes
that are not on-link.

ad7cf16 Add David's IPv6 improvements to CHANGELOG
af9d783 Fix (bogus) MSVC 2010 warning about uninitialized variable usage in ip6.c It's wrong because the variables are initialized during first loop iteration due to best_addr == NULL
68358d7 nd6: cull destination cache on router removal
7323fa1 nd6: some work on basic RFC 4861 compliance
10eb2ca ip6: improve source address selection
6c06ecd ip6/nd6: route using on-link prefixes, not addresses
9f1714d nd6: improve router selection
2486b41 netif: more ip6 state changes invoke status callback
519d809 nd6: fix Duplicate Address Detection
9f3c6dd nd6: check link status before sending packets
8c761a2 nd6: improve address autoconfiguration support

@mikaleppanen mikaleppanen force-pushed the mikaleppanen:lwip_ipv4v6_ppp branch Aug 22, 2017

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 22, 2017

Updated lwip not to add neighbor cache entry based on router advertisement if link layer address is not present (PPP case)

features/FEATURE_LWIP/lwip-interface/mbed_lib.json Outdated
@@ -16,7 +16,11 @@
"addr-timeout": {
"help": "On dual-stack system how long to wait preferred stack's address in seconds",
"value": 5
},
},
"both-addr-timeout": {

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 22, 2017

Contributor

Not clear from help how this relates to the preferred wait - is it "parallel" or "series"? Could these defaults wait a total of 10, or 5? Having it be an additional wait (if it isn't) might be easier to conceptualise.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 22, 2017

Contributor

Maybe comment to say that it's not terribly meaningful to set both options. How about a boolean that changes what addr-timeout means - extra wait for preferred, or extra wait for all addresses. Make the help clear that is the extra wait on top of the initial wait for any address.

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 22, 2017

Contributor

Corrected.

features/FEATURE_LWIP/lwip-interface/lwip/src/netif/ppp/lwip_auth.c Outdated
@@ -751,6 +751,10 @@ void link_established(ppp_pcb *pcb) {
if (!doing_multilink) {
for (i = 0; (protp = protocols[i]) != NULL; ++i)
if (protp->protocol != PPP_LCP
#if PPP_IPV4_SUPPORT && PPP_IPV6_SUPPORT

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 22, 2017

Contributor

This feels like it's in the wrong layer. Shouldn't a suppression check be in ip[v6]cp_lowerup?

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 22, 2017

Contributor

Corrected.

neighbor_cache[neighbor_index].state = ND6_INCOMPLETE;
neighbor_cache[neighbor_index].counter.probes_sent = 1;
nd6_send_neighbor_cache_probe(&neighbor_cache[neighbor_index], ND6_SEND_FLAG_MULTICAST_DEST);
if (netif->hwaddr_len) {

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Aug 22, 2017

Contributor

Suggested simple alternative - set state to STALE if interface has no HW address. I believe that should stay in STALE forever, without triggering probes. (Because the entries are not used, so don't transition to DELAY)

This comment has been minimized.

@mikaleppanen

mikaleppanen Aug 22, 2017

Contributor

Corrected.

mikaleppanen added some commits Aug 3, 2017

Added dual Ipv4/Ipv6 stack support to lwip address configuration
Added new configuration option "both-addr-timeout" that defines
how long to wait that addresses from both systems become available.
Added Ipv4/Ipv6 stack stack support to lwip DNS queries
If both IPv4 and IPv6 are available, and DNS query fails for first system,
second system is tried.

@mikaleppanen mikaleppanen force-pushed the mikaleppanen:lwip_ipv4v6_ppp branch Aug 22, 2017

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 22, 2017

@kjbracey-arm corrected defects.

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 23, 2017

I think all has approved this. @hasnainvirk can you update status review status. @theotherjimmy @0xc0170 can this progress?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 23, 2017

/morph test-nightly

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 23, 2017

@mikaleppanen How this can be tested ? Is it referenced PR above to cliapp? Any new example (will cellular example get an update) ?

cc @MarceloSalazar @adbridge

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 23, 2017

I have tested this manually using cliapp tests (above pr) and mbed-os-example-client with MTS_DRAGONFLY_F411RE. Cellular example will be updated but currently we do not have echo server for ipv6 connectivity (ublox server works only for ipv4).

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 23, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1094

Build failed!

mikaleppanen added some commits Aug 15, 2017

Improved manual DNS address addition for dual stack case
If DHCP or PPP does not provide DNS addresses, for dual stack, adds both ipv4
and ipv6 DNS addresses to DNS list.

@mikaleppanen mikaleppanen force-pushed the mikaleppanen:lwip_ipv4v6_ppp branch to 32131b6 Aug 23, 2017

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 23, 2017

Updated pull request. Changed memory pool areas that caused nightly tests to fail. Tested to compile with failing targets and tested Ethernet connectivity with LPC1768 and UBLOX_C027.

@adbridge adbridge added needs: CI and removed needs: review labels Aug 24, 2017

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Aug 24, 2017

@0xc0170 Please restart the test.

@mikaleppanen

This comment has been minimized.

Contributor

mikaleppanen commented Aug 25, 2017

Could you retest this.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Aug 28, 2017

@0xc0170 @adbridge Please, somebody restart the tests.

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Aug 28, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 29, 2017

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1134

All builds and test passed!

@adbridge adbridge added ready for merge and removed needs: CI labels Aug 30, 2017

@0xc0170 0xc0170 merged commit c0fe3b5 into ARMmbed:master Sep 4, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test-nightly Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@0xc0170 0xc0170 removed the ready for merge label Sep 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment