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.

Sign up for GitHub

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

Jump to bottom

Greentea testing wifi connect nonblocked #11194

Merged
merged 1 commit into from
Aug 21, 2019
Merged

Greentea testing wifi connect nonblocked #11194

merged 1 commit into from
Aug 21, 2019

Conversation

dmaziec
Copy link
Contributor

@dmaziec dmaziec 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

@dmaziec dmaziec force-pushed the Wifi_test_connect_nonblock branch 2 times, most recently from af00970 to b294d60 Compare August 9, 2019 13:57
@ciarmcom
Copy link
Member

ciarmcom commented Aug 9, 2019

@dmaziec1, thank you for your changes.
@VeijoPesonen @SeppoTakalo @michalpasztamobica @ARMmbed/mbed-os-test @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

VeijoPesonen
VeijoPesonen reviewed Aug 12, 2019
View reviewed changes
Copy link
Contributor

@VeijoPesonen VeijoPesonen left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

VeijoPesonen
VeijoPesonen reviewed Aug 12, 2019
View reviewed changes
TESTS/network/wifi/wifi_connect_nonblock.cpp Outdated
@@ -0,0 +1,72 @@
/*
* Copyright (c) 2017, ARM Limited, All Rights Reserved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2019

0xc0170
0xc0170 requested changes Aug 12, 2019
View reviewed changes
TESTS/network/wifi/main.cpp
@@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment would be better than it is now.

@dmaziec dmaziec force-pushed the Wifi_test_connect_nonblock branch 3 times, most recently from affc80d to 07aa23b Compare August 12, 2019 13:39
SeppoTakalo
SeppoTakalo reviewed Aug 13, 2019
View reviewed changes
TESTS/network/wifi/wifi_connect_nonblock.cpp Outdated
} else {
while ((connection_status != NSAPI_STATUS_GLOBAL_UP)
&& (connection_status != NSAPI_STATUS_LOCAL_UP)) {
connection_status = wifi->get_connection_status();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asynchronous execution added, @SeppoTakalo , please review again.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 20, 2019

@dmaziec1 Please respond to reviews.

@dmaziec dmaziec force-pushed the Wifi_test_connect_nonblock branch 4 times, most recently from 1cce2de to 88b00fb Compare August 20, 2019 13:38
@0xc0170
Copy link
Contributor

0xc0170 commented Aug 21, 2019

CI started

@michalpasztamobica michalpasztamobica mentioned this pull request Aug 21, 2019
Closed
@mbed-ci
Copy link

mbed-ci 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SeppoTakalo SeppoTakalo SeppoTakalo left review comments

@VeijoPesonen VeijoPesonen VeijoPesonen left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@michalpasztamobica michalpasztamobica michalpasztamobica approved these changes

Assignees
No one assigned
Labels
release-version: 5.13.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dmaziec @ciarmcom @0xc0170 @mbed-ci @SeppoTakalo @VeijoPesonen @michalpasztamobica