Lwip 2.0 #2767

mikaleppanen · 2016-09-21T10:06:37Z

Description

LWIP stack 2.0, LWIP stack adaptation code and mac driver updates.

Status

READY

Migrations

Updated freescale drivers to do or operation for multicast group filters:
hal/targets/hal/TARGET_Freescale/TARGET_KSDK2_MCUS/TARGET_K66F/drivers/fsl_enet.c
hal/targets/hal/TARGET_Freescale/TARGET_KSDK2_MCUS/TARGET_MCU_K64F/drivers/fsl_enet.c
(commit 92baa85b7b07dbcdfcb1422be03127d88ddded91)

Related PRs

List related PRs against other branches:
None

Todos

[X ] Tests
Documentation

Deploy notes

None

Steps to test or reproduce

None

0xc0170 · 2016-09-21T10:49:50Z

cc @bulislaw @sg- @geky @kjbracey-arm

@mikaleppanen Can you share any tests run for this patch? how many targets were tested?

bridadan · 2016-09-21T13:50:01Z

/morph test-nightly

kjbracey · 2016-09-21T13:56:38Z

Testing so far has been limited to one particular end-to-end system test - we have got mbed-client-example to register with the device server using both IPv4 and IPv6 on K64F hardware, using UDP.

More directed targeted IP testing and testing on other platforms is required. The other platforms' drivers are compile-tested only.

kjbracey · 2016-09-21T14:01:27Z

Note that the LWIP code that is being added is from the reference repository fork here:

https://github.com/ARMmbed/lwip

coming from the branch "mbed-os-prefixed", based on STABLE-2.0.0-rc2, with 2 bug fixes cherry-picked from master.

mbed-bot · 2016-09-21T14:13:07Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 909

Build failed!

bridadan · 2016-09-21T14:16:22Z

@kjbracey-arm and @mikaleppanen, you should have access to the CI report by following the bot's posted link. But here's a quick summary of one of the build failures for the RZ_A1H when built with ARM Compiler 5:

...
08:53:20 Compile: Ethernet.cpp
08:53:20 [Error] Ethernet.cpp@24,0:  #276: name followed by "::" must be a class or namespace name
08:53:20 [Error] Ethernet.cpp@24,0:  #260-D: explicit type is missing ("int" assumed)
08:53:20 [Warning] Ethernet.cpp@26,0:  #940-D: missing return statement at end of non-void function "mbed::Ethernet"
08:53:20 [Error] Ethernet.cpp@28,0:  #276: name followed by "::" must be a class or namespace name
08:53:20 [Error] Ethernet.cpp@32,0:  #276: name followed by "::" must be a class or namespace name
08:53:20 [Error] Ethernet.cpp@36,0:  #276: name followed by "::" must be a class or namespace name
08:53:20 [Error] Ethernet.cpp@40,0:  #276: name followed by "::" must be a class or namespace name
08:53:20 [Error] Ethernet.cpp@44,0:  #276: name followed by "::" must be a class or namespace name
08:53:20 [Error] Ethernet.cpp@48,0:  #276: name followed by "::" must be a class or namespace name
08:53:20 [Error] Ethernet.cpp@52,0:  #276: name followed by "::" must be a class or namespace name
08:53:20 [Error] Ethernet.cpp@56,0:  #276: name followed by "::" must be a class or namespace name
08:53:20 [Error] Ethernet.cpp@56,0:  #20: identifier "Mode" is undefined
08:53:20 [Error] Ethernet.cpp@61,0:  #20: identifier "AutoNegotiate" is undefined
08:53:20 [Error] Ethernet.cpp@62,0:  #20: identifier "HalfDuplex10" is undefined
08:53:20 [Error] Ethernet.cpp@63,0:  #20: identifier "FullDuplex10" is undefined
08:53:20 [Error] Ethernet.cpp@64,0:  #20: identifier "HalfDuplex100" is undefined
08:53:20 [Error] Ethernet.cpp@65,0:  #20: identifier "FullDuplex100" is undefined

EDIT: for further convenience, here's a direct link to the build matrix: http://10.118.12.43:8081/job/build_matrix/547/

mikaleppanen · 2016-09-21T15:00:12Z

Corrected dual stack ipv6 preferred mode in 91c9f98

bulislaw · 2016-09-21T15:05:21Z

CC: @andreaslarssonublox

bridadan · 2016-09-21T15:11:52Z

@mikaleppanen Will the patch you pushed fix the build error I mentioned? Is this good to go for another test run?

geky · 2016-09-21T19:23:31Z

This has a relatively large number of changes in a single pr, including conflicts with other prs that are currently under review (for example IPv6 support in SocketAddress #2652).

Could we limit the scope of this pr to only lwip and split other changes (especially changes to the network-socket API) into separate prs? This would allow us to properly review changes that impact the user API and other network-socket implementations.

geky · 2016-09-21T19:26:54Z

features/net/network-socket/NetworkStack.cpp

@@ -20,11 +20,29 @@
 #include "stddef.h"
 #include <new>

+static bool stackHasIPV4(NetworkStack *stack)


Please stick with the existing naming convention: stack_has_ipv4
https://docs.mbed.com/docs/getting-started-mbed-os/en/latest/Full_Guide/Code_Style/

geky · 2016-09-21T19:28:40Z

features/net/network-socket/NetworkStack.cpp

+        return 0;
+    }
+
+    if (stackHasIPV4(this)) {


I believe checking for literals should actually be handled in nsapi_dns_query (similarly to the POSIX gethostbyname)

Doesn't make sense to me. Surely nsapi_dns_query should do what it name says - a DNS query. Having to add literal parsing to the DNS code too doesn't make sense - it has to be outside the DNS code anyway.

It's gethostbyname that has to handle literals before attemping a DNS query.

geky · 2016-09-21T19:29:50Z

features/net/network-socket/Socket.h

@@ -182,6 +182,8 @@ class Socket {
        attach(mbed::callback(obj, method));
    }

+    NetworkStack *stack() const { return _stack; }


Why is this function exposed publicly in the Socket class? The internal NetworkStack is intentionally hidden from the user API.

The reason it was added is no longer valid, so it no longer needs to be added for this commit. (It was needed to implement the DNS work in the first draft.)

But I remain unconvinced of the merit of hiding NetworkStack.

geky · 2016-09-21T19:33:40Z

features/net/network-socket/SocketAddress.h

     */
-    void set_ip_address(const char *addr);
+    bool set_ip_address(const char *addr);


Not sure this is needed. If the set_ip_address call fails, the resulting SocketAddress will be null. Failure can be checked like so:

SocketAddress address; address.set_ip_address("1.2.3.4"); if (!address) { // handle invalid address }

We should avoid changing existing APIs unless there is evidence that the current API is insufficient or conflicts with the current coding guidelines.

The SocketAddress will also be "null" after a valid parse of "0.0.0.0". Need to distinguish - otherwise we'd request a DNS lookup of "0.0.0.0". (Or "::").

Ah interesting, that is a valid point.

geky · 2016-09-21T19:34:32Z

features/net/network-socket/nsapi_dns.cpp


 // DNS options
-#define DNS_BUFFER_SIZE 256 
+#define DNS_BUFFER_SIZE 512


Just checking, was there a test that confirmed the original buffer size was too small?

The DNS spec says 512 bytes. We reduced it from 1024 to 512 during devlopment, but when we saw that someone else had been touching the DNS code (aargh), we saw you'd reduced it further to 256. Left it at 512 during conflict resolution.

Not seen any failures, but I'm not totally confident about using IPv6 DNS with half-sized buffers.

(We also made the original DNS reply parsing resilient to truncated results, but had to abandon it - wasn't straightforward to refit to the new code, which doesn't consider packet length at all).

Ah gotcha, wasn't aware of the DNS spec 👍

geky · 2016-09-21T19:38:00Z

features/net/FEATURE_IPV4/lwip-interface/.mbedignore

+lwip/src/apps/*
+lwip/src/netif/ppp/*
+lwip/src/inlude/lwip/apps/*
+lwip/src/inlude/posix/*


If the lwip codebase is being mirrored, shouldn't we just avoid adding these directories into mbed-os?

None of these seem to add a benefit except maybe documentation, which shouldn't present a compilation issue.

Would mean writing special scripts and tools to cope with the variation, and more error-prone tracking of the upstream. This is attempting to work with the source control system rather than against it. (As much as we can given the limitations of mbed-os).

This way means it's just a single "git subtree add".

Looks like you're right, removing the directories would be annoying.

It is possible to amend the subtree add commit to remove the directories, and this removal works cleanly with future subtree merges. But if the removed files are modified, it will introduce a merge conflict for every modification.

geky · 2016-09-21T19:42:36Z

features/net/network-socket/SocketAddress.cpp

@@ -43,16 +43,21 @@ static bool ipv4_is_valid(const char *addr)
 static bool ipv6_is_valid(const char *addr)
 {
    // Check each digit for [0-9a-fA-F:]
+    // Must also have at least 2 colons
+    int colons = 0;


+1, nice catch

geky · 2016-09-21T20:10:25Z

features/net/FEATURE_IPV4/lwip-interface/lwip_stack.c

+        addr_type = NETCONN_DNS_IPV4;
+    }
+#endif
+    err_t err = netconn_gethostbyname_addrtype(host, &lwip_addr, addr_type);


It looks like addr_type was declared conditionally based on LWIP_IPV4 && LWIP_IPV6. Has this been tested with other IP version configurations?

That actually does work, because the call is a macro that ignores its 3rd parameter unless both IPv4 and IPv6 are enabled. Should at least be commented, but probably better to not use the addrtype form unless in the if.

mikaleppanen · 2016-09-22T12:09:19Z

@bridadan ethernet.h header collision is now corrected. Could you trigger tests again.

bridadan · 2016-09-22T12:17:31Z

@mikaleppanen It sounds like there's still a discussion going on for this PR, maybe we should hold off until that has a chance to be resolved.

sg- · 2016-09-22T14:01:53Z

/morph test-nightly

mbed-bot · 2016-09-23T00:16:46Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 928

Test failed!

geky · 2016-09-23T07:56:25Z

I've tried to merge the DNS changes into the pr over here: #2652

Let me know if I missed anything

kjbracey · 2016-09-23T08:05:40Z

Thanks - looks like we should be able to drop those bits from here. Have commented over there.

mikaleppanen · 2016-09-23T13:55:09Z

TCP corrected in: 84194f04df688f403e19887d0357a29207fd16f6 could you retest

bridadan · 2016-09-23T16:42:37Z

/morph test-nightly

mbed-bot · 2016-09-23T21:31:45Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 935

Test failed!

mazimkhan · 2016-09-27T12:31:27Z

retest uvisor

mazimkhan · 2016-09-27T12:36:37Z

retest uvisor

mikaleppanen · 2016-09-27T13:30:51Z

Updated to master and corrected inspection defect. Could you retest this.

geky · 2016-09-27T14:21:16Z

/morph test-nightly

mbed-bot · 2016-09-27T18:14:16Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 964

Test failed!

…d" when multicast group address is added to filter.

mikaleppanen · 2016-09-28T06:02:06Z

Updated commit message 1d2130f

kjbracey · 2016-09-28T06:23:24Z

When this does go in, please make sure it's a normal merge, not a squash or rebase, to preserve the lwIP subtree.

0xc0170 · 2016-09-28T07:46:11Z

@0xc0170 @geky @kjbracey-arm @bridadan @bogdanm any last comments?

LGTM, let's get this in soon, as we will need to rebase wifi feature branch.

mikaleppanen · 2016-09-28T09:51:47Z

Corrected TCP keep alive interval 7d770366d17500ea50eea03fdf94e6a890a559b1

kjbracey · 2016-09-28T10:18:26Z

On the TCP interval - it's not actually clear what is "correct".

NSAPI documentation doesn't specify the units of the interval, and it actually has always been milliseconds for lwIP - it's not a 1.4->2.0 change. So suggest that last commit moves to a separate pull request.

mbed client appears to have a default interval setting of 300 - it's assuming seconds. Not unreasonably, as that tends to be the unit used by most APIs (eg Linux sysctl, lwIP's own setsockopt). But it ends up getting a keepalive every half-second.

mikaleppanen · 2016-09-28T10:43:22Z

Removed the TCP interval commit.

bogdanm · 2016-09-28T13:52:38Z

While I'm not very familiar with all the PR details, it looks good overall 👍

geky · 2016-09-28T14:34:53Z

I'm still uncomfortable with the network-socket API changes in this pr. Given the size of the diff and the impact of the network-socket API, I don't believe these changes have had acceptable review. I'm not saying the changes won't be accepted, but we should have separate prs to track the network-socket API changes.

kjbracey · 2016-09-29T10:25:57Z

Agree that it was an excessive lump, given that we're stuck with Github.

But the set of changes made sense as a single topic branch - they're all related to one aim - making IPv4+IPv6 work. Problem is Github PRs are a rubbish review tool. If this was Gerrit it would have been easy to review the individual patches.

Having a pull request review system but no proper patch review system is just sad. :(

geky reviewed Sep 21, 2016

View reviewed changes

sg- added the needs: work label Sep 22, 2016

kjbracey mentioned this pull request Sep 22, 2016

nsapi - Add explicit DNS interface to the network socket API #2652

Merged

sg- added needs: CI and removed needs: work labels Sep 22, 2016

Mika Leppänen added 15 commits September 28, 2016 08:48

Added json configuration file and mbed ignore file to lwip stack.

98c452d

Updated K64F mac for lwip 2.0 and IPv6

2f9c5e9

K64F: Use older multicast filtering API

5f7a953

Updated lwip API and configuration files to lwip 2.0

bb50717

Corrected K66F and K64F drivers to make "or" operation instead of "an…

2bad43d

…d" when multicast group address is added to filter.

Updated NUC472 mac for lwip 2.0 and IPv6

f4e76bb

Updated LPC17 mac for lwip 2.0 and IPv6

f270fcb

Updated RZ_A1H mac for lwip 2.0 and IPv6

c61973c

Updated VK_RZ_A1H mac for lwip 2.0 and IPv6.

a349c11

Updated STM mac for lwip 2.0 and IPv6.

1d2130f

Declare heap ourselves to specify section

0a88251

Added lwip status parameter

283ee52

Added ipv6 support to lwip dns adaptation and updated dchp functionality

95383dd

Corrected tcp socket send

1f98cc3

Added version to nsapi get host by name

e5c291d

sg- added ready for merge and removed needs: CI labels Sep 28, 2016

sg- merged commit 3d1531f into ARMmbed:master Sep 28, 2016

sg- added release-version: 5.2.0 and removed ready for merge labels Sep 28, 2016

geky mentioned this pull request Oct 3, 2016

nsapi - Standardize support of NSAPI_UNSPEC #2897

Merged

Lwip 2.0 #2767

Lwip 2.0 #2767

Uh oh!

Conversation

mikaleppanen commented Sep 21, 2016

Description

Status

Migrations

Related PRs

Todos

Deploy notes

Steps to test or reproduce

Uh oh!

0xc0170 commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bridadan commented Sep 21, 2016

Uh oh!

kjbracey commented Sep 21, 2016

Uh oh!

kjbracey commented Sep 21, 2016

Uh oh!

mbed-bot commented Sep 21, 2016

Output

Uh oh!

bridadan commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaleppanen commented Sep 21, 2016

Uh oh!

bulislaw commented Sep 21, 2016

Uh oh!

bridadan commented Sep 21, 2016

Uh oh!

geky commented Sep 21, 2016

Uh oh!

geky Sep 21, 2016 • 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!

kjbracey Sep 22, 2016 • 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!

geky Sep 21, 2016 • 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!

mikaleppanen commented Sep 22, 2016

Uh oh!

0xc0170 commented Sep 21, 2016 •

edited

Loading

bridadan commented Sep 21, 2016 •

edited

Loading

geky Sep 21, 2016 •

edited

Loading

kjbracey Sep 22, 2016 •

edited

Loading

geky Sep 21, 2016 •

edited

Loading

kjbracey commented Sep 23, 2016 •

edited

Loading