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

Fix greentea test mbed-os-tests-netsocket-connectivity #5649

Merged
merged 5 commits into from Dec 20, 2017

Conversation

Projects
None yet
8 participants
@Archcady
Contributor

Archcady commented Dec 4, 2017

Fix in lwip_stack to pass greentea test for Realtek RTL8195A platform

Notes:

Description

This is a fix in the lwip stack to ensure that the netif is not being initialized twice. In the earlier version the netif was being inialized twice and hence the test "bring the network up and down twice" was faling.

Status

READY

Migrations

No other changes other than in lwip_stack.c

Related PRs

None

Todos

  • [X ] Tests. (Tested with latest mbed-os and mbedgt)
  • Documentation [N/A]

Deploy notes

No change in build/test environment needed.

Steps to test or reproduce

download and run the greentea test corresponding to the test using the command

mbed test -n mbed-os-tests-netsocket-connectivity --test-config Realtek_wifi

Before running the command make sure to input the wi-fi credentials in mbed-os\tools\test_configs\RealtekInterface.json

@0xc0170 0xc0170 requested review from kjbracey-arm and mikaleppanen Dec 4, 2017

ret = mbed_lwip_emac_init(emac);
if (ret == NSAPI_ERROR_OK) {
netif_inited = true;
if(netif_inited == false){

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Dec 4, 2017

Contributor

Formatting - missing spaces around ().

I'd kind of like a sanity check on the other case where we are already inited, rather than always just returning OK. Something like

if (netif_is_ppp || emac != lwip_netif.state) {
    ret = NSAPI_ERROR_PARAMETER;
}

to catch someone trying to register a different EMAC, which we couldn't cope with.

Mind you, thinking about it, if you're making this change because you're calling this on every connect to your driver, you shouldn't be doing that - this should be a one-off operation - if (!inited) in your driver's connect, or do it on construction. That will match better the coming multiple-interface world (#5558).

So I'm wondering if we should actually be faulting any second init call with NSAPI_ERROR_PARAMETER - effectively saying "we don't (yet) support a second interface".

This comment has been minimized.

@Archcady

Archcady Dec 12, 2017

Contributor

I tried modifying my driver to call the mbed_lwip_init function only once but it still fails connectivity test for bring the network up and down twice. I tried putting the call in constructor, I also made the check as you mentioned but the test still fails.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Dec 12, 2017

Contributor

I'm not familiar with this test - is there a possibility that the mbed_lwip_xxx stuff is fundamentally bugged in some way that prevents this passing? Do any other EMAC drivers pass this test?

@SeppoTakalo - do we have an equivalent test?

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Dec 12, 2017

Contributor

Can't see any obvious reason for failure - the bringup/bringdown logic in lwip_stack.c for a 2nd go around is much the same between native lwIP drivers and EMAC drivers, and I know native lwIP drivers are passing.

Calling mbed_lwip_bringdown (in your disconnect) followed by mbed_lwip_bringup (in your connect) should work.

This comment has been minimized.

@Archcady

Archcady Dec 12, 2017

Contributor

Ok I called the bringdown in my disconnect, it worked, shall I update the changes in this PR or create a new one? The new changes are only in my device driver and not in lwip_stack.

@samchuarm

This comment has been minimized.

samchuarm commented Dec 7, 2017

prashantrar added some commits Dec 12, 2017

Revert "Fixing changes to make greentea test mbed-os-tests-netsocket-…
…connectivity pass for target Realtek AMEBA"

This reverts commit 1fcfced.
@Archcady

This comment has been minimized.

Contributor

Archcady commented Dec 12, 2017

Have pushed the latest changes as decided to this PR. Latest changes in commit
01808db

@kjbracey-arm

Logic looks correct to me now, but formatting is a bit messed up - tabs instead of spaces.

@SeppoTakalo

Fix the formatting.
For indent, configure your editor to insert spaces instead of tabs. Tab width varies, depending on your editor configuration, we cannot rely on it to be standard 8 spaces.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 14, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Dec 14, 2017

Build : SUCCESS

Build number : 700
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5649/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@Archcady

This comment has been minimized.

Contributor

Archcady commented Dec 18, 2017

Is this PR merged yet, Shall I delete the branch?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Dec 18, 2017

Is this PR merged yet, Shall I delete the branch?

It has not been merged, it's now ready for integration, will be merged soon.

@0xc0170 0xc0170 merged commit 13dbb67 into ARMmbed:master Dec 20, 2017

10 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2 Local mbed2 testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@0xc0170 0xc0170 changed the title from Fixing for greentea test mbed-os-tests-netsocket-connectivity to Fix greentea test mbed-os-tests-netsocket-connectivity Dec 20, 2017

@mbed-ci

This comment has been minimized.

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