Cordio: Add tests that validates a cordio port. #7221

pan- · 2018-06-14T17:27:24Z

Description

The first test focuse on the transport by testing that the reset sequence is
correctly sent to the controller and properly received.

The second test validates that the reset sequence fullfill the right state
of the stack and ensure that bluetooth initialization succeed.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

The first test focuse on the transport by testing that the reset sequence is correctly sent to the controller and properly received. The second test validates that the reset sequence fullfill the right state of the stack and ensure that bluetooth initialization succeed.

donatieng

Thanks a lot Vincent, a few issues as described

donatieng · 2018-06-19T11:47:33Z

features/FEATURE_BLE/targets/TARGET_CORDIO/TESTS/cordio_hci/driver/main.cpp

+    if (params->error == BLE_ERROR_NONE) {
+        initialization_state = INITIALIZATION_SUCCESS;
+    } else {
+        initialization_state = INITIALIZATION_SUCCESS;


The other one ;)

donatieng · 2018-06-19T11:49:32Z

features/FEATURE_BLE/targets/TARGET_CORDIO/TESTS/cordio_hci/driver/main.cpp

+using namespace utest::v1;
+using mbed::callback;
+
+#define INITIALIZATION_TIMEOUT (10 * 1000)


Is it actually acceptable for the initialisation process to take this long?

Yes, at first I was thinking of setting it to 30 seconds; if the bitrate is low and the controller require a large service pack then initialization can takes a loooooot of time.

donatieng · 2018-06-19T12:01:02Z

features/FEATURE_BLE/targets/TARGET_CORDIO/TESTS/cordio_hci/driver/main.cpp

+    TEST_ASSERT_NOT_EQUAL(0, hciCoreCb.availBufs);
+
+    uint8_t invalid_le_states[HCI_LE_STATES_LEN] = { 0 };
+    TEST_ASSERT_NOT_EQUAL(0, memcmp(invalid_le_states, hciCoreCb.leStates, HCI_LE_STATES_LEN));


Can you just add a quick comment (as hciCoreCb is an internal Cordio struct, so not immediately obvious what you're testing here)

donatieng · 2018-06-19T12:03:05Z

features/FEATURE_BLE/targets/TARGET_CORDIO/TESTS/cordio_hci/transport/main.cpp

+                }
+                break;
+        }
+


You should break from the for loop here if test_result is TEST_RESULT_FAILURE

doesn't the if do that? it'll break (return) on any result change

Yes, the if does that whenever a result is present (success or failure).

OK - there are a few things that confused me, now I understand what you're doing. Few questions/remarks:

Could you rename TEST_RESULT_TIMEOUT_FAILURE to something more explicit (like 'in progress'); it hasn't timed out till proven otherwise

At the moment, if you don't know the HCI OpCode's value, you skip to the next byte - which would be parameters' length; what's the rationale behind this? Skipping padding bytes? You could interpret the length field (and subsequent parameters) as OpCodes in the current implementation

donatieng · 2018-06-19T12:04:17Z

features/FEATURE_BLE/targets/TARGET_CORDIO/TESTS/cordio_hci/transport/main.cpp

+                break;
+        }
+
+        if (test_result != TEST_RESULT_TIMEOUT_FAILURE) {


Presumably that should be outside of the for loop?

Why ? there's no guarantee that data in input is at the event boundary and it may fail early.

donatieng · 2018-06-19T12:05:49Z

features/FEATURE_BLE/targets/TARGET_CORDIO/TESTS/cordio_hci/transport/main.cpp

+}
+
+Case cases[] = {
+    Case("Test reset command", test_reset_command),


Could you add a second test that issues multiple reset commands + initialize()/terminate()?

donatieng · 2018-06-20T08:20:23Z

features/FEATURE_BLE/targets/TARGET_CORDIO/TESTS/cordio_hci/transport/main.cpp

+                }
+                break;
+        }
+


OK - there are a few things that confused me, now I understand what you're doing. Few questions/remarks:

Could you rename TEST_RESULT_TIMEOUT_FAILURE to something more explicit (like 'in progress'); it hasn't timed out till proven otherwise

At the moment, if you don't know the HCI OpCode's value, you skip to the next byte - which would be parameters' length; what's the rationale behind this? Skipping padding bytes? You could interpret the length field (and subsequent parameters) as OpCodes in the current implementation

… init.

The existing test was expecting that the acknowledgement of the reset command would be the first and only event receive. This assumption is false. As a consequence, the new code parse all incoming packets and raise a flag in the following circumstances: * a reset packet has been successfully received. * RX stream is not synchronized * The status of the reset command is an error. Another test has been added that send a serie of reset commands.

pan- · 2018-06-20T14:28:03Z

@donatieng @paul-szczepanek-arm I've rewritten the logic in the transport test: all incoming packet are parsed and a flag is raised whenever:

A reset packet has been received
RX is synced with the controller
The status of the reset packet is incorrect.

In the process I've added a second test that sends a serie of reset commands and waits for response events.

donatieng

Thanks a lot @pan-, this addresses all of my issues 👍

cmonr · 2018-06-21T16:21:28Z

/morph build

mbed-ci · 2018-06-21T18:07:05Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-06-21T19:42:23Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-06-22T01:34:20Z

Test : SUCCESS

Build number : 2204
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7221/2204

pan- requested review from donatieng and paul-szczepanek-arm June 14, 2018 17:27

pan- added the component: ble label Jun 14, 2018

cmonr added the needs: review label Jun 14, 2018

paul-szczepanek-arm previously approved these changes Jun 19, 2018

View reviewed changes

donatieng suggested changes Jun 19, 2018

View reviewed changes

donatieng suggested changes Jun 20, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jun 20, 2018

pan- added 3 commits June 20, 2018 12:00

Cordio BLE: Fix test failure when controller initialization failed.

5b7f7aa

Cordio BLE: Document in test what fields are expected to be set after…

4f8a006

… init.

pan- dismissed paul-szczepanek-arm’s stale review via d2ecdb8 June 20, 2018 14:25

donatieng approved these changes Jun 21, 2018

View reviewed changes

cmonr added needs: review needs: CI and removed needs: work needs: review labels Jun 21, 2018

paul-szczepanek-arm approved these changes Jun 21, 2018

View reviewed changes

cmonr added the release-version: 5.9.2 label Jun 21, 2018

cmonr merged commit 34513f2 into ARMmbed:master Jun 22, 2018

0xc0170 removed the needs: CI label Jun 22, 2018

adbridge added release-version: 5.10.0-rc1 and removed release-version: 5.9.2 labels Jun 29, 2018

Cordio: Add tests that validates a cordio port. #7221

Cordio: Add tests that validates a cordio port. #7221

Uh oh!

Conversation

pan- commented Jun 14, 2018

Description

Pull request type

Uh oh!

donatieng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-szczepanek-arm Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donatieng Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donatieng Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- commented Jun 20, 2018

Uh oh!

donatieng left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Jun 21, 2018

Uh oh!

mbed-ci commented Jun 21, 2018

Triggering tests

Uh oh!

mbed-ci commented Jun 21, 2018

Uh oh!

mbed-ci commented Jun 22, 2018

Uh oh!

Uh oh!

paul-szczepanek-arm Jun 19, 2018 •

edited

Loading

donatieng Jun 20, 2018 •

edited

Loading

donatieng Jun 20, 2018 •

edited

Loading