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

Greentea testing wifi connect nonblocked #11194

Merged
merged 1 commit into from Aug 21, 2019

Conversation

@dmaziec1
Copy link
Contributor

commented Aug 9, 2019

Description

Added test is implemented for testing Wifi in nonblocked mode. However, it is commented in Test cases TESTS/network/wifi/main.cpp because most boards do not use this functionality.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[x] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@VeijoPesonen
@michalpasztamobica

Release Notes

@dmaziec1 dmaziec1 force-pushed the dmaziec1:Wifi_test_connect_nonblock branch 2 times, most recently from af00970 to b294d60 Aug 9, 2019

@ciarmcom ciarmcom requested review from michalpasztamobica, SeppoTakalo, VeijoPesonen and ARMmbed/mbed-os-maintainers Aug 9, 2019

@ciarmcom

This comment has been minimized.

@VeijoPesonen
Copy link
Contributor

left a comment

Would you please add this to the test case documentation - TESTS/network/wifi/README.md. Otherwise this looks good to me.

@@ -0,0 +1,72 @@
/*
* Copyright (c) 2017, ARM Limited, All Rights Reserved

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Aug 12, 2019

Contributor

2019

@@ -74,6 +74,7 @@ Case cases[] = {
Case("WIFI-GET-RSSI", wifi_get_rssi),
Case("WIFI-CONNECT-PARAMS-VALID-UNSECURE", wifi_connect_params_valid_unsecure),
Case("WIFI-CONNECT", wifi_connect),
//Case("WIFI-CONNECT-NONBLOCKING", wifi_connect_nonblock),

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Aug 12, 2019

Member

this should not be in here (can be added anytime).

Why is it commented out ? The implementation is being added in this PR.

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Aug 12, 2019

Contributor

In fact, none of the Wi-Fi class implementations we checked is able to pass this test right now. We want the test to be available, however, in order to determine the expected behaviour and be able to ask Wi-Fi stack developers (Ublox, ST, etc.) to implement the missing functionality or at least return NSAPI_ERROR_UNSUPPORTED, so the test can be skipped.
We thought it would be easiest for someone starting the implementation with a test-driven-development to uncomment this line, run the test and try to make it pass.
Removing the test is also fine - most developers will probably be able to figure out how to run it ;-).
I would say, however, that leaving a commented test will not let us (and anyone developing a W-Fi driver) forget about this function.
@0xc0170 , how about we add a comment explaining the situation, like:

// Most boards are not passing this test, but they should if they support non-blocking API.

?

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Aug 12, 2019

Member

The comment would be better than it is now.

@dmaziec1 dmaziec1 force-pushed the dmaziec1:Wifi_test_connect_nonblock branch 3 times, most recently from affc80d to 07aa23b Aug 12, 2019

} else {
while ((connection_status != NSAPI_STATUS_GLOBAL_UP)
&& (connection_status != NSAPI_STATUS_LOCAL_UP)) {
connection_status = wifi->get_connection_status();

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Aug 13, 2019

Contributor

You are effectively polling in busy-loop.
Please add some kind of delay here, or use callbacks that report connection status.

This comment has been minimized.

Copy link
@dmaziec1

dmaziec1 Aug 20, 2019

Author Contributor

Asynchronous execution added, @SeppoTakalo , please review again.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Aug 19, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@dmaziec1 Please respond to reviews.

@dmaziec1 dmaziec1 force-pushed the dmaziec1:Wifi_test_connect_nonblock branch 4 times, most recently from 1cce2de to 88b00fb Aug 20, 2019

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Aug 20, 2019

@dmaziec1 dmaziec1 force-pushed the dmaziec1:Wifi_test_connect_nonblock branch from 88b00fb to 6aca4ab Aug 20, 2019

@dmaziec1 dmaziec1 force-pushed the dmaziec1:Wifi_test_connect_nonblock branch from 6aca4ab to 8f4a865 Aug 20, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 21, 2019

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit 590ce2c into ARMmbed:master Aug 21, 2019

18 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/greentea-test Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8590 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
Projects
None yet
7 participants
You can’t perform that action at this time.