Add USB CDC and Serial tests #7755

fkjagodzinski · 2018-08-10T12:39:51Z

Description

Add tests-usb_device-serial with a few test cases:

CDC RX multiple bytes,
CDC RX multiple bytes concurrent,
CDC RX single bytes,
CDC RX single bytes concurrent,
CDC USB reconnect,
CDC loopback,
Serial USB reconnect,
Serial getc,
Serial printf/scanf,
Serial line coding change,
Serial terminal reopen.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Feature
[ ] Breaking change

CC @c1728p9 @jamesbeyond

cmonr · 2018-08-14T01:58:59Z

@c1728p9 @jamesbeyond Mind taking a look?

c1728p9

@fkjagodzinski were these tests passing for you? When I run tests-usb_device-serial the test case CDC loopback consistently fails.

c1728p9 · 2018-08-15T20:49:34Z

TESTS/host_tests/usb_device_serial.py

+    """Exception raised by retry_fun_call()."""
+
+
+def retry_fun_call(fun, num_retries=3, retry_delay=0.0):


Why are retries needed?

Operations like finding the serial port and opening it have to be retried due to variable delays on the host side. You can check how this works on your machine after setting verbose in line https://github.com/fkjagodzinski/mbed-os/blob/082c63e16a11b14a25b9b327843d2e8b1d7db408/TESTS/host_tests/usb_device_serial.py#L55

c1728p9 · 2018-08-15T20:59:20Z

TESTS/host_tests/usb_device_serial.py

+        Search is based on the id received from the device itself.
+        Raises RuntimeError if the device is not found.
+        """
+        vid, pid = (int(i, base=16) for i in str(usb_id_str).split(','))


Can the device serial number be used instead? The serial number can be made unique per device. These tests will likely be running in an environment with many boards connected. VID/PID may not be sufficient to determine the device you are connected to.

I'll look into this. This should be rather straight forward to implement.

Changed as requested -- using USB SN to identify the correct device on the host system.

c1728p9 · 2018-08-15T21:03:09Z

TESTS/usb_device/serial/main.cpp

+    Case("Serial USB reconnect", test_serial_usb_reconnect),
+    Case("Serial terminal reopen", test_serial_term_reopen),
+    Case("Serial getc", test_serial_getc),
+    Case("Serial printf/scanf", test_serial_printf_scanf),


It may also be good to test line_coding_changed if you are testing USBCDC in this PR.

New test case added -- Serial line coding change.

0xc0170 · 2018-08-22T12:18:26Z

@fkjagodzinski Please respond to the latest review

jamesbeyond · 2018-08-22T13:14:01Z

@0xc0170, Filip is on holiday, he will continue this PR when back to the office next week

cmonr · 2018-08-27T16:05:39Z

Is @fkjagodzinski back?

fkjagodzinski · 2018-08-28T12:44:28Z

@cmonr, yes I'm back and will update this PR soon.

fkjagodzinski · 2018-09-06T11:56:11Z

@c1728p9, thanks for your feedback. 👍 These 2 new commits update the code according to your comments.

I'm now moving to CDC loopback failures.

fkjagodzinski · 2018-09-20T13:49:17Z

The CDC loopback test case is fixed now. @c1728p9 could you take a look?

Needs new review

c1728p9 · 2018-10-02T18:58:13Z

TESTS/usb_device/serial/main.cpp

+    while (!usb_serial.connected()) {
+        wait_ms(1);
+    }
+    wait_ms(TX_DELAY_MS);


Why is there a delay here?

The first usb_serial.printf() call from the for loop has to be delayed. More details can be found in a comment for TX_DELAY_MS macro.

Is this a problem with the host machine or the mbed board? If this is an mbed problem then it needs to be root caused.

This behaviour is Linux specific, when tests run on Win host, I do not see characters echoed with ^ prefix.

c1728p9 · 2018-10-02T19:31:15Z

TESTS/usb_device/serial/main.cpp

+{
+    USBCDC usb_cdc(false, USB_CDC_VID, USB_CDC_PID);
+    char usb_cdc_sn[USB_SN_MAX_LEN] = { };
+    usb_desc2str(usb_cdc.string_iserial_desc(), usb_cdc_sn, USB_SN_MAX_LEN);


This will return the same serial number for all connected devices so the host won't be able to determine the right one to connect to. If testing is running concurrently on CI it may get these boards mixed up.

Instead, could the host test create and send a serial number to use in the test?

Ok, I'll look into this.

Done. The USB device now uses a SN (UUID) received from host on test suite setup.

c1728p9 · 2018-10-02T20:07:48Z

TESTS/usb_device/serial/main.cpp

+    TEST_ASSERT_TRUE(usb_cdc.configured());
+    TEST_ASSERT_TRUE(usb_cdc.ready());
+
+    wait_ms(USB_DISCONNECT_DELAY_MS);


Why is USB_DISCONNECT_DELAY_MS needed? usb_cdc.wait_ready() should be enough to ensure that the host opened the serial port.

Correct, no issue on the device side. Additional delay USB_DISCONNECT_DELAY_MS helps Windows hosts handle this test case correctly. My Linux machine does not need it. I'll look into this and add comments for clarification.

c1728p9 · 2018-10-02T20:11:38Z

TESTS/usb_device/serial/main.cpp

+    TEST_ASSERT_FALSE(usb_cdc.configured());
+    TEST_ASSERT_FALSE(usb_cdc.ready());
+
+    wait_ms(USB_RECONNECT_DELAY_MS);


Why does this need to be 200ms? Only a 1ms delay should be needed between disconnect and reconned as explained in the basic test.

Good catch! 1 ms delay works as expected.

c1728p9 · 2018-10-02T20:26:30Z

TESTS/usb_device/serial/main.cpp

+{
+    USBCDC usb_cdc(false, USB_CDC_VID, USB_CDC_PID);
+    char usb_cdc_sn[USB_SN_MAX_LEN] = { };
+    usb_desc2str(usb_cdc.string_iserial_desc(), usb_cdc_sn, USB_SN_MAX_LEN);


You might want to send lots of data to the device as part of this test. Event if its more data then the device can hold at once, the built in flow control should ensure that nothing is dropped.

cmonr · 2018-10-18T16:11:51Z

@c1728p9 @jamesbeyond All good?

cmonr · 2018-10-25T01:25:24Z

@c1728p9 @jamesbeyond Ping

fkjagodzinski · 2018-10-25T15:29:13Z

@c1728p9 thanks for your feedback.

Although the TX_DELAY_MS value should matter only for Linux hosts, when I removed this delay, I found another issue present on Windows hosts. I suspect this to be a host side issue and think this PR should proceed. I'll update this test suite in another PR if it is needed.

c1728p9 · 2018-10-26T00:37:15Z

Hi @fkjagodzinski I looked into the TX_DELAY_MS failures on Windows and found them to be a race-condition in pyserial. As part of pyserial's serial port open call it first opens the physical serial port and then does a flush of TX and RX buffers. Any data that is received between the open and flush is lost, causing tests like the loopback test to hang and timeout.

As a fix for this DTR (data terminal ready) can be set to False before opening the serial port. This prevents the device from sending any data. Once the port is opened and ready DTR can be re-enabled to allow testing like normal.

With this change I don't need any of the calls to wait_ms(TX_DELAY_MS); and I don't see any failures even across multiple runs. I pushed my changes https://github.com/c1728p9/mbed-os/tree/increase_stability. Feel free to make use of this in this or a subsequent PR.

fkjagodzinski · 2018-11-02T16:37:12Z

cherry-picked a few fixes from @c1728p9,
added a fix to handle the Linux hosts correctly (regarding the DTR line)

c1728p9 · 2018-11-02T23:27:02Z

TESTS/host_tests/usb_device_serial.py

+
+    def port_open_wait(self):
+        """Open the serial and wait until it's closed by the device."""
+        mbed_serial = serial.Serial(dsrdtr=False)


Why is dsrdtr set to false here and below?

This is just setting the default dsrdtr value explicitly. It has to stay False to allow setting the DTR line manually, i.e. setting DTR True after the call to open().

cmonr · 2018-11-09T03:58:26Z

/morph build

mbed-ci · 2018-11-09T05:13:04Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-11-09T06:26:00Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-11-09T11:09:44Z

Test : FAILURE

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

fkjagodzinski · 2018-11-09T12:16:19Z

The USB device tests require an additional USB cable connected to the host. These tests should not run on the CI until additional cables are provided.

@cmonr, @0xc0170, @studavekar, could you update the CI config

https://github.com/ARMmbed/austin-mbed-ci/blob/9d0b962742d574cacb1b7c2cc1a4364d8819a3c0/jobs/test_matrix_raas/config.xml#L344-L345

and replace tests-usb_device-basic with tests-usb_device-*, or should I create a PR to that repo?

0xc0170 · 2018-11-09T12:25:58Z

Please send PR, and notify @studavekar

studavekar · 2018-11-09T15:35:34Z

@c1728p9, @fkjagodzinski HW test CI config is updated to filter out new USB tests.

fkjagodzinski · 2018-11-09T16:10:32Z

Great! Thanks @studavekar.

cmonr · 2018-11-09T19:21:23Z

/morph test

cmonr · 2018-11-09T23:40:11Z

Will restart in a bit. Figured we had a spurious network issue, when this is only waiting on a test.

mbed-ci · 2018-11-10T03:40:59Z

Test : FAILURE

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

cmonr · 2018-11-10T04:26:25Z

/morph test

mbed-ci · 2018-11-10T06:37:24Z

Test : FAILURE

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

cmonr · 2018-11-10T23:47:43Z

/morph test

mbed-ci · 2018-11-11T02:49:13Z

Test : FAILURE

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

0xc0170 · 2018-11-12T11:14:26Z

/morph test

mbed-ci · 2018-11-12T14:15:58Z

Test : FAILURE

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

0xc0170 · 2018-11-14T08:20:48Z

/morph test

mbed-ci · 2018-11-14T11:41:32Z

Test : FAILURE

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

adbridge · 2018-11-14T14:38:18Z

/morph test

mbed-ci · 2018-11-14T22:42:35Z

Test : SUCCESS

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

c1728p9 · 2018-11-27T01:36:34Z

@fkjagodzinski the test case Serial printf/scanf fails when IAR is used. Can you investigate and correct this failure?

fkjagodzinski · 2018-12-10T13:44:31Z

@fkjagodzinski the test case Serial printf/scanf fails when IAR is used. Can you investigate and correct this failure?

This should be fixed with #9033.

TomoYamanaka · 2018-12-17T02:55:42Z

@fkjagodzinski @c1728p9
Thank you so much. I confirmed that the above test case is success with IAR by adopting #9033 .

I'm preparing to submit a PR for Renesas' s USB Device feature, so I'll be glad if you could rebase feature-hal-spec-usb-device repository to merge #9033 .

0xc0170 added the needs: review label Aug 10, 2018

0xc0170 requested review from c1728p9 and jamesbeyond August 10, 2018 13:19

c1728p9 previously requested changes Aug 15, 2018

View reviewed changes

cmonr added needs: work and removed needs: review labels Aug 16, 2018

cmonr added needs: review and removed needs: work labels Sep 6, 2018

c1728p9 reviewed Oct 2, 2018

View reviewed changes

c1728p9 force-pushed the feature-hal-spec-usb-device branch from 9e06f07 to 05d77ab Compare October 3, 2018 20:15

fkjagodzinski force-pushed the test-usb-serial branch from 9d5082c to 8533c0d Compare October 18, 2018 16:00

cmonr added the release-version: feature branch label Oct 25, 2018

c1728p9 mentioned this pull request Oct 30, 2018

Ignore disabled Kinetis USB endpoint interrupts #8596

Merged

c1728p9 reviewed Nov 2, 2018

View reviewed changes

c1728p9 force-pushed the feature-hal-spec-usb-device branch from 05d77ab to aa0cdea Compare November 2, 2018 23:30

Filip Jagodzinski added 2 commits November 5, 2018 13:44

Tests: USB: Add tests for CDC class.

e30cc39

Tests: USB: Add tests for Serial class.

56b928f

cmonr added needs: CI and removed needs: review labels Nov 8, 2018

0xc0170 mentioned this pull request Nov 12, 2018

netsocket: networkinterface status failures #8715

Closed

cmonr merged commit 7e866c4 into ARMmbed:feature-hal-spec-usb-device Nov 15, 2018

cmonr removed the needs: CI label Nov 15, 2018

fkjagodzinski deleted the test-usb-serial branch November 15, 2018 09:41

		"""Exception raised by retry_fun_call()."""


		def retry_fun_call(fun, num_retries=3, retry_delay=0.0):

Add USB CDC and Serial tests #7755

Add USB CDC and Serial tests #7755

Uh oh!

Conversation

fkjagodzinski commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

cmonr commented Aug 14, 2018

Uh oh!

c1728p9 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!

Choose a reason for hiding this comment

Uh oh!

fkjagodzinski Sep 6, 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!

0xc0170 commented Aug 22, 2018

Uh oh!

jamesbeyond commented Aug 22, 2018

Uh oh!

cmonr commented Aug 27, 2018

Uh oh!

fkjagodzinski commented Aug 28, 2018

Uh oh!

fkjagodzinski commented Sep 6, 2018

Uh oh!

fkjagodzinski commented Sep 20, 2018

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Oct 18, 2018

Uh oh!

cmonr commented Oct 25, 2018

Uh oh!

fkjagodzinski commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c1728p9 commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fkjagodzinski commented Nov 2, 2018

Uh oh!

fkjagodzinski commented Aug 10, 2018 •

edited

Loading

fkjagodzinski Sep 6, 2018 •

edited

Loading

fkjagodzinski commented Oct 25, 2018 •

edited

Loading

c1728p9 commented Oct 26, 2018 •

edited

Loading

studavekar commented Nov 9, 2018 •

edited

Loading