Add new WiFi tests #3147

bulislaw · 2016-10-27T13:02:56Z

Description

The tests try to:

scan for available networks and check whether specified networks
are present in the results.
connect to and disconnect from the specified network.
repeats the scan tests while connected to a network.
connect to a network and perform simple HTTP query.

Status

HOLD

Deploy notes

There are some issues to be solved for the tests to work in CI:

WIFI_TEST_NETWORKS, WIFI_TEST_PASS and WIFI_TEST_NETWORKS need to be defined. Is there a way of doing that in CIs?
For UBLOX Odin board the DEVICE_EMAC macro needs to be defined, but just for this test (it will break Ethernet tests)
For other platforms, platform needs to be Arduino FF with ESP2866 connected to D0/D1 pins and the ESP826xx-driver need to be compiled in.

@mazimkhan @bridadan

geky · 2016-10-27T15:38:03Z

TESTS/network/wifi/main.cpp

+ */
+#if TARGET_UBLOX_EVK_ODIN_W2
+#include "OdinWiFiInterface.h"
+SingletonPtr<OdinWiFiInterface> wifi;


This is a bit concerning, SingletonPtr was intended only for gc-optimizations internally in libraries.

We're you able to track down the exact issue that was caused by declaring this interface globally?

Sort of, the greentea sync timeouts waiting for the sync, I'm guessing we're stuck in the initialization of the Odin and miss it.

Oh, that's an interesting situation. @bridadan, there doesn't happen to be a test-specific configuration for delaying the sync timeout is there?

Target can always sync with Greentea first and then init Odin. Time taken by Odin init can be added to the test case timeout.

geky · 2016-10-27T15:38:57Z

TESTS/network/wifi/main.cpp

+WiFiInterface *get_wifi()
+{
+#if TARGET_UBLOX_EVK_ODIN_W2
+    return wifi.get();


If we do need a SingletonPtr, perhaps we should use it consistently for all interfaces?

As far as i can tell our singleton can't handle constructors with parameters. Required for ESP, sure we could change the code, but not sure it's worth it.

Oh that's very interesting. Perhaps we should dynamically allocate the WiFi interfaces?

Yeah we could. We could just use the ESP-style-logic there without bothering to use Singleton and just allocate odin as well.

Sounds reasonable

geky · 2016-10-27T15:42:00Z

TESTS/network/wifi/main.cpp

+SingletonPtr<OdinWiFiInterface> wifi;
+#else
+#if !TARGET_FF_ARDUINO
+#error [NOT_SUPPORTED] Only Arduino form factor devices supported


Has this become a requirement for ease of use? The esp8266 should work fine on any serial connection.

It's either that or long list of pin names for platforms that are not Arduino FF.

You make a good point, that wouldn't be the greatest. Could we at least have an option to provide pins that would avoid the unsupported error? It would be nice to be able to use this test with esp8266s on boards locally.

If it's too difficult, we can just modify the test directly if we need to test other boards.

geky · 2016-10-27T15:42:38Z

A few noted questions, but looks great! excited to have some solid wifi tests.

bulislaw · 2016-10-27T16:33:51Z

On hold due to #3152

bridadan · 2016-10-27T16:39:23Z

@bulislaw Yay for more tests! This is awesome.

Addressing a few of your points:

WIFI_TEST_NETWORKS, WIFI_TEST_PASS and WIFI_TEST_NETWORKS need to be defined. Is there a way of doing that in CIs?

You could use an mbed_app.json to specify those macros. Should be harmless, but still feels kinda funny since those macros will be specified for all test builds.

You can use mbed_app.json for tests by doing the following:

mbed test --app-config mbed_app.json

For UBLOX Odin board the DEVICE_EMAC macro needs to be defined, but just for this test (it will break Ethernet tests)

You can use the #error [NOT_SUPPORTED] functionality for this. Just #ifdef on DEVICE_EMAC.

For other platforms, platform needs to be Arduino FF with ESP2866 connected to D0/D1 pins and the ESP826xx-driver need to be compiled in.

mbed OS tests can't rely on any code that doesn't exist in the mbed-os tree. So this test should be specialized to just platforms that use the EMAC or something. It can't rely on ESP8266Interface since that doesn't live in the OS.

bridadan · 2016-10-27T16:41:41Z

TESTS/network/wifi/main.cpp

+/* We use singletons as some of the WiFi modules don't like to be initialized multiple times and using global objects
+   causes greentea serial to timeout.
+ */
+#if TARGET_UBLOX_EVK_ODIN_W2


I'm not really crazy about having to do this for each platform that would be added to this test. Is there a more generic method planned for this? Something like WifiInterface *wifi = get_default_wifi_interface() (syntax not important here, just that the target knows its default wifi/network interface)

Yeah, we could have a 'factory' for built in wifis, but it would be basically just moving the ifdefs somewhere else. And that would only work for built in WiFi as the external modules can be connected to different pins and what not. I'm not sure how useful would that be, we can think about it.

@bulislaw Yes, it'd only be useful for platforms with built-in network interfaces. This is definitely for another discussion and (series of) PRs, but I just wanted to be sure we don't get in the habit of doing this kind of thing in tests. This is the kind of thing in mbed 2 that I think we should avoid in mbed 5: https://github.com/ARMmbed/mbed-os/blob/master/features/unsupported/tests/mbed/digitalinout/main.cpp#L3

bulislaw · 2016-10-28T08:17:45Z

WIFI_TEST_NETWORKS, WIFI_TEST_PASS and WIFI_TEST_NETWORKS need to be defined. Is there a way of doing that in CIs?
You could use an mbed_app.json to specify those macros. Should be harmless, but still feels kinda funny since those macros will be specified for all test builds.

You can use mbed_app.json for tests by doing the following:

mbed test --app-config mbed_app.json

I'd rather not have our ssid/password in the github even if it's just a test network, that should be done for each CI location I guess.

For UBLOX Odin board the DEVICE_EMAC macro needs to be defined, but just for this test (it will break Ethernet tests)
You can use the #error [NOT_SUPPORTED] functionality for this. Just #ifdef on DEVICE_EMAC.

I can, it's a good point I'll add it to make the error clearer, but my point is we want to actually test it in the farm, so we need a solution that would set that there for this test. Possibly we can overwrite it in mbed_app.json as well?

For other platforms, platform needs to be Arduino FF with ESP2866 connected to D0/D1 pins and the ESP826xx-driver need to be compiled in.
mbed OS tests can't rely on any code that doesn't exist in the mbed-os tree. So this test should be specialized to just platforms that use the EMAC or something. It can't rely on ESP8266Interface since that doesn't live in the OS.

That's a pity, would be nice to test ESP2866 as well.

bridadan · 2016-10-28T15:41:26Z

TESTS/network/wifi/main.cpp

+ * - WIFI_TEST_NETWORKS - List of network that presence will be asserted e.g. "net1", "net2", "net3"
+ */
+#if !defined(WIFI_TEST_SSID) || !defined(WIFI_TEST_PASS) || !defined(WIFI_TEST_NETWORKS)
+#error WIFI_TEST_NETWORKS, WIFI_TEST_PASS and WIFI_TEST_NETWORKS have to be defined for this test.


I'd recommend using the #error [NOT_SUPPORTED] macro here as well. That way it doesn't show up as a failure if the WIFI_TEST_* defines are missing, which is up to the user/CI to provide and not the code base. This way it will just show up as a build skip.

bridadan · 2016-10-28T15:44:03Z

I'd rather not have our ssid/password in the github even if it's just a test network, that should be done for each CI location I guess.

@bulislaw Oh yeah definitely, each CI system would provide a custom mbed_app.json, it wouldn't live in the code base. Sorry this is what I meant, should have been more clear 😄

I can, it's a good point I'll add it to make the error clearer, but my point is we want to actually test it in the farm, so we need a solution that would set that there for this test. Possibly we can overwrite it in mbed_app.json as well?

Yes, I think we should be able to use mbed_app.json for this, though I haven't tried it. If you can't use the device_has attribute in the config files you could always just emit it as a macro (DEVICE_EMAC)

0xc0170 · 2016-11-09T16:12:47Z

@bulislaw Can we progress with this PR ? The issue #3152 was resolved 2 days ago.

The tests try to: * scan for available networks and check whether specified networks are present in the results. * connect to and disconnect from the specified network. * repeats the scan tests while connected to a network. * connect to a network and perform simple HTTP query.

bulislaw · 2016-11-10T10:17:38Z

@bridadan @mazimkhan what do you think guys? I also added template mbed_app.json that the CIs would need to provide.

bridadan

Just two little things, please see below.

bridadan · 2016-11-10T16:37:39Z

TESTS/network/wifi/main.cpp

+ * - WIFI_TEST_NETWORKS - List of network that presence will be asserted e.g. "net1", "net2", "net3"
+ */
+#if !defined(MBED_CONF_APP_WIFI_SSID) || !defined(MBED_CONF_APP_WIFI_PASSWORD) || !defined(MBED_CONF_APP_WIFI_NETWORKS)
+#error MBED_CONF_APP_WIFI_SSID, MBED_CONF_APP_WIFI_PASSWORD and MBED_CONF_APP_WIFI_NETWORKS have to be defined for this test.


@bulislaw Could you please change this to a [NOT_SUPPORTED] macro? This will show up as a build failure for anyone building these tests locally for the UBLOX_EVK_ODIN_W2. If you add the [NOT_SUPPORTED] it'll just show up as a skip and they can continue with testing.

bridadan · 2016-11-10T16:38:48Z

TESTS/network/wifi/mbed_app.json

@@ -0,0 +1,21 @@
+{


Would you mind renaming this file to template_mbed_app.txt? This will prevent the file from being picked up at all by the build system, but I think that having this reference is still extremely useful.

bulislaw · 2016-11-10T17:03:51Z

Done.

bridadan · 2016-11-10T17:30:19Z

/morph test

mbed-bot · 2016-11-10T21:30:00Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1052

Build failed!

bridadan · 2016-11-11T01:27:31Z

Lots of CI issues today, sorry again. Had to restart the system. Restarting now.

/morph test

mbed-bot · 2016-11-11T04:55:12Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1065

All builds and test passed!

bridadan · 2016-11-14T15:24:02Z

While I'm still a little uneasy bringing in a test that only works on one target at the moment (not even a feature), it's ok for now considering we don't have any other targets in the tree with this capability. We just need to be EXTREMELY careful that we don't fall into the same behavior of bringing target-specific config into each test case as we did in mbed 2, otherwise we'll end up with this kind of stuff: https://github.com/ARMmbed/mbed-os/blob/master/features/unsupported/tests/mbed/interruptin_2/main.cpp#L7

bridadan · 2016-11-14T15:24:25Z

@0xc0170 One last review?

bulislaw · 2016-11-14T15:44:22Z

One thing I was thinking to add to wifi would be a factory for 'built in' wifi modules. So instead of doing OdinWiFiInterface wifi(); you could do something likeWiFiInterface *wifi = BuiltinWifi::get();, that wouldn't fix the problem, but would at least shift it further away from the users (at least for built in modules).
But that's separate discussion we need to have some other time.

bridadan · 2016-11-14T16:01:46Z

From above (outdated diff) :)

I'm not really crazy about having to do this for each platform that would be added to this test. Is there a more generic method planned for this? Something like WifiInterface *wifi = get_default_wifi_interface() (syntax not important here, just that the target knows its default wifi/network interface)

I definitely agree! And not just for WiFi, any Network interface (Ethernet, BLE, Mesh, etc).

@bulislaw Maybe you can start an issue on GitHub about this to start the discussion?

bridadan · 2016-11-14T20:06:41Z

After a discussion with @sg-, it sounds like we may not want to bring this kind of test in without having the "get default wifi interface" function. Otherwise other WiFi targets coming in will start adding there own <TARGET>Interface.h includes to the test.

geky · 2016-11-15T23:37:00Z

After another discussion, we will not be able to provide a forward-compatible "get default wifi interface" function for 5.3. These tests are valuable enough that we should probably go ahead and merge as a temporary solution.

0xc0170 · 2016-11-16T17:06:35Z

/morph test-nightly

bridadan · 2016-11-16T22:32:22Z

We were moving some of the CI machines around and the server cable got bumped, so the nightly job got stopped unfortunately. However, there's no need to run a nightly since the test will be skipped until I add the mbed_app.json to the build, and that won't be added until this PR is merged. And the ODIN board isn't in CI yet. So it should be good to go!

bulislaw · 2016-11-22T09:28:40Z

ping

0xc0170 added the do not merge label Oct 27, 2016

geky reviewed Oct 27, 2016

View reviewed changes

bridadan reviewed Oct 27, 2016

View reviewed changes

bulislaw mentioned this pull request Oct 28, 2016

Odin WiFi broken by DNS API change #3152

Closed

bridadan reviewed Oct 28, 2016

View reviewed changes

bulislaw added 3 commits November 10, 2016 10:04

WiFi tests: Increase the timeout for ESP module

34cc61f

WiFi tests: Only support Odin board

e8b55ca

bulislaw force-pushed the wifi_test branch from a4c933a to 1a3f6c9 Compare November 10, 2016 10:15

WiFi test Add support for mbed_app.json

e116a66

bulislaw force-pushed the wifi_test branch from 1a3f6c9 to e116a66 Compare November 10, 2016 10:15

bridadan suggested changes Nov 10, 2016

View reviewed changes

WiFi tests: Make tests CI friendly

3c06cda

bridadan added needs: CI and removed do not merge labels Nov 10, 2016

bridadan approved these changes Nov 14, 2016

View reviewed changes

bridadan added needs: review and removed needs: CI labels Nov 14, 2016

0xc0170 removed the needs: review label Nov 16, 2016

0xc0170 added the needs: CI label Nov 16, 2016

bridadan added ready for merge and removed needs: CI labels Nov 16, 2016

bulislaw mentioned this pull request Nov 24, 2016

u-blox ODIN-W2 drivers update #3323

Merged

0xc0170 added release-version: 5.3.0 and removed ready for merge labels Nov 24, 2016

0xc0170 merged commit 464f04c into ARMmbed:master Nov 24, 2016

bulislaw deleted the wifi_test branch December 20, 2016 16:20

Add new WiFi tests #3147

Add new WiFi tests #3147

Conversation

bulislaw commented Oct 27, 2016

Description

Status

Deploy notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bulislaw Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geky commented Oct 27, 2016 • edited Loading

bulislaw commented Oct 27, 2016

bridadan commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bulislaw commented Oct 28, 2016

Choose a reason for hiding this comment

bridadan commented Oct 28, 2016

0xc0170 commented Nov 9, 2016

bulislaw commented Nov 10, 2016

bridadan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bulislaw commented Nov 10, 2016

bridadan commented Nov 10, 2016

mbed-bot commented Nov 10, 2016

Output

bridadan commented Nov 11, 2016

mbed-bot commented Nov 11, 2016

Output

bridadan commented Nov 14, 2016

bridadan commented Nov 14, 2016

bulislaw commented Nov 14, 2016

bridadan commented Nov 14, 2016

bridadan commented Nov 14, 2016

geky commented Nov 15, 2016

0xc0170 commented Nov 16, 2016

bridadan commented Nov 16, 2016

bulislaw commented Nov 22, 2016

bulislaw Oct 27, 2016 •

edited

Loading

geky commented Oct 27, 2016 •

edited

Loading