Skip to content

Add missing wifi parameters to test configs #9778

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

Merged

Conversation

michalpasztamobica
Copy link
Contributor

Description

The point is to follow the configs as described in the documentation .

Pull request type

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

Reviewers

@SeppoTakalo

@ciarmcom ciarmcom requested review from SeppoTakalo and a team February 20, 2019 16:00
@ciarmcom
Copy link
Member

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

@cmonr
Copy link
Contributor

cmonr commented Feb 20, 2019

@michalpasztamobica What happens if WIFI_MAX_SCAN_SIZE is undefined? Wouldn't the json then be invalid?

Copy link
Contributor

@bridadan bridadan left a comment

Choose a reason for hiding this comment

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

My feedback echos @SeppoTakalo

@michalpasztamobica
Copy link
Contributor Author

@cmonr , @SeppoTakalo , you are right, I might have overinterpreted my task... PR amended to reflect your input, thanks for that.
@SeppoTakalo, before approving, please take a look into internal JIRA ticket ONME-4147 - it seems the task's goals have changed, but let's make sure we can close that JIRA with this PR...

@michalpasztamobica
Copy link
Contributor Author

Noticed some missing network parameters documentation, so added that in a separate commit, but I think it's related enough to merge both in one PR.

Copy link
Contributor

@bridadan bridadan left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. The configuration values are up to the folks who maintain those, so if it works for you it works for me.

@michalpasztamobica michalpasztamobica force-pushed the add_missing_wifi_config_params branch from 903fdba to 08b06e2 Compare February 22, 2019 08:13
@@ -26,7 +62,7 @@
"target.network-default-interface-type": "WIFI",
"nsapi.default-wifi-ssid": "\"WIFI_SSID\"",
"nsapi.default-wifi-password": "\"WIFI_PASSWORD\"",
"nsapi.default-wifi-security": "WIFI_SECURITY",
"nsapi.default-wifi-security": "WPA2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To keep thos configs compileable out of the box I had to change this setting.
Otherwise we get:

[Error] NetworkInterfaceDefaults.cpp@86,0:  #20: identifier "NSAPI_SECURITY_WIFI_SECURITY" is undefined

@michalpasztamobica
Copy link
Contributor Author

@VeijoPesonen , thanks for your feedback. I just pushed a commit with "Wi-Fi" written correctly :)

@cmonr
Copy link
Contributor

cmonr commented Feb 25, 2019

The point is to follow the configs as described in the documentation .

🎉

@cmonr
Copy link
Contributor

cmonr commented Feb 25, 2019

CI started

@cmonr cmonr added the risk: G label Feb 25, 2019
@mbed-ci
Copy link

mbed-ci commented Feb 25, 2019

Test run: SUCCESS

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

@cmonr cmonr merged commit 044d0c9 into ARMmbed:master Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants