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

resolve lwip init twice issue #4444

Merged
merged 3 commits into from Jun 19, 2017

Conversation

Projects
None yet
9 participants
@Archcady
Contributor

Archcady commented Jun 5, 2017

resolve wifi example test fail before lwip is init
We find for REALTEK_RTL8195AM target, the wifi example will hang at mbed_lwip_bringup().

@0xc0170 0xc0170 requested review from geky and kjbracey-arm Jun 5, 2017

features/FEATURE_LWIP/lwip-interface/lwip_stack.c Outdated
@@ -469,6 +469,7 @@ nsapi_error_t mbed_lwip_emac_init(emac_interface_t *emac)
eth_arch_enable_interrupts();
#endif
netif_inited = true;

This comment has been minimized.

@0xc0170

0xc0170 Jun 5, 2017

Member

Tab used? please fix alignment

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 5, 2017

As this change is in the generic, how was this tested? It touches wifi, what about the ethernet? any other platform?

This variable is set in mbed_lwip_bringup, isnt that correct place?

@Archcady Archcady force-pushed the Archcady:lwip_issue branch Jun 6, 2017

@Archcady

This comment has been minimized.

Contributor

Archcady commented Jun 6, 2017

We set this netif_inited here is because there's multiple place doing init of the lwip. The first time is by our RTWInterface constructor, the second time is during mbed_lwip_bringup in RTWInterface.connect().
That's why we submit this PR to discuss.

Archcady added some commits Jun 5, 2017

resolve lwip init twice issue
resolve wifi example test fail before lwip is init

@Archcady Archcady force-pushed the Archcady:lwip_issue branch to a812696 Jun 7, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 9, 2017

@geky @kjbracey-arm Can you please review?

@tung7970

This comment has been minimized.

Contributor

tung7970 commented Jun 9, 2017

RTWInterface calls mbed_lwip_init() in constructor, and calls mbed_lwip_bringup() in its connect() when a wifi connection is requested.

// Backwards compatibility with people using DEVICE_EMAC
nsapi_error_t mbed_lwip_init(emac_interface_t *emac)
{
    mbed_lwip_core_init();
    return mbed_lwip_emac_init(emac);
}

// Backwards compatibility with people using DEVICE_EMAC
nsapi_error_t mbed_lwip_bringup(bool dhcp, const char *ip, const char *netmask, const char *gw)
{
    return mbed_lwip_bringup_2(dhcp, false, ip, netmask, gw);
}

The problem is that mbed_lwip_init() calls mbed_lwip_emac_init() without setting netif_inited to true, so mbed_lwip_emac_init() ends up called twice.

Hope this helps.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 12, 2017

@geky @kjbracey-arm Can you please review?

bump

@tung7970

This comment has been minimized.

Contributor

tung7970 commented Jun 12, 2017

For the EMAC case, I think the proper place to set netif_inited is in mbed_lwip_init, iff mbed_lwip_emac_init() returns OK.

diff --git a/features/FEATURE_LWIP/lwip-interface/lwip_stack.c b/features/FEATURE_LWIP/lwip-interface/lwip_stack.c
index a1fa5848a..de5278bd7 100644
--- a/features/FEATURE_LWIP/lwip-interface/lwip_stack.c
+++ b/features/FEATURE_LWIP/lwip-interface/lwip_stack.c
@@ -468,8 +468,14 @@ nsapi_error_t mbed_lwip_emac_init(emac_interface_t *emac)
 // Backwards compatibility with people using DEVICE_EMAC
 nsapi_error_t mbed_lwip_init(emac_interface_t *emac)
 {
 +    nsapi_error_t ret;
 + 
      mbed_lwip_core_init();
 -    return mbed_lwip_emac_init(emac);
 +    ret = mbed_lwip_emac_init(emac);
 +    if (ret == NSAPI_ERROR_OK) {
 +        netif_inited = true;
 +    }
 +    return ret;
 }
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 13, 2017

As I understand, this is still not completely fixed @tung7970 ?

@tung7970

This comment has been minimized.

Contributor

tung7970 commented Jun 13, 2017

@0xc0170 The problem is mbed_lwip_emac_int is called twice, once in mbed_lwip_init(), and the other in mbed_lwip_bringup_2(). The bug was introduced in the EMAC backward fix.

As long as netif_inited is correctly set to true after it's inited, wifi example should work properly. Either @Archcady's fix or the patch I posted should work.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 13, 2017

I prefer @tung7970's version - but @Archcady's would be okay.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 14, 2017

/morph test-nightly

andreaslarssonublox pushed a commit to u-blox/mbed-os that referenced this pull request Jun 14, 2017

@geky

geky approved these changes Jun 14, 2017

Looks good to me

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 14, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 550

Test failed!

@sg-

This comment has been minimized.

Member

sg- commented Jun 15, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 15, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 561

Test failed!

@adbridge

This comment has been minimized.

Contributor

adbridge commented Jun 16, 2017

ARCH_PRO-GCC_ARM.features-feature_lwip-tests-mbedmicro-net-gethostbyname.DNS query

[1497558667.02][CONN][RXD] DNS: query "connector.mbed.com" => "(null)"
[1497558667.06][CONN][RXD] :49::FAIL: Expected 0 Was -3009

ARCH_PRO-GCC_ARM.features-feature_lwip-tests-mbedmicro-net-tcp_hello_world.TCP hello world

[1497559442.79][CONN][RXD] TCP client IP Address is 10.118.13.255
[1497559442.83][CONN][RXD] HTTP: Connection to developer.mbed.org:80
[1497559456.78][CONN][RXD] HTTP: ERROR
[1497559456.81][CONN][RXD] :88::FAIL: Expression Evaluated To FALSE

K64F-GCC_ARM.features-feature_lwip-tests-mbedmicro-net-gethostbyname.DNS query

[1497564549.07][CONN][RXD] DNS: query "connector.mbed.com" => "(null)"
[1497564549.10][CONN][RXD] :49::FAIL: Expected 0 Was -3009

K64F-GCC_ARM.features-feature_lwip-tests-mbedmicro-net-tcp_hello_world.TCP hello world

[1497565521.38][CONN][RXD] TCP client IP Address is 10.118.13.255
[1497565521.43][CONN][RXD] HTTP: Connection to developer.mbed.org:80
[1497565534.86][CONN][RXD] HTTP: ERROR
[1497565534.90][CONN][RXD] :88::FAIL: Expression Evaluated To FALSE

So looks like all the failures were LWIP network failures when trying to communicate with developer.mbed.org !

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 17, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 17, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 578

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 19, 2017

Seems like network issue was back again the last weekend ? I restarted one job to confirm, waiting for a result
@geky @studavekar

@adbridge

This comment has been minimized.

Contributor

adbridge commented Jun 19, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 19, 2017

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 587

All builds and test passed!

@theotherjimmy theotherjimmy merged commit c08864b into ARMmbed:master Jun 19, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment