USB basic and hid greentea test cleanup and enable to the bare metal profile #12916

rajkan01 · 2020-05-04T17:52:06Z

Summary of changes

Enabled USB basic and hid test to bare metal profile.
Removed the partially (The green tea test host side implementation is missing on pyusb_basic.py ) implemented USB control transfer VENDOR_TEST_CTRL_NONE_DELAY and VENDOR_TEST_CTRL_NONE request type as it is becoming obsolete.
Removed the USB control request type VENDOR_TEST_CTRL_IN_DELAY, VENDOR_TEST_CTRL_OUT_DELAY, VENDOR_TEST_CTRL_IN_STATUS_DELAY, VENDOR_TEST_CTRL_OUT_STATUS_DELAY macros as did not have any test.
Removed high priority event queue which is used by VENDOR_TEST_CTRL_NONE_DELAY request type test.

Impact of changes

With these changes, USB basic and hid greentea tests build and run successfully.

Migration actions required

None.

Documentation

None.

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@evedon @fkjagodzinski @maciejbocianski

ciarmcom · 2020-05-04T19:00:36Z

@rajkan01, thank you for your changes.
@evedon @ARMmbed/mbed-os-maintainers please review.

TESTS/usb_device/basic/USBTester.h

kjbracey · 2020-05-07T07:32:04Z

TESTS/usb_device/basic/USBTester.cpp

@@ -174,7 +177,7 @@ void USBTester::callback_request(const setup_packet_t *setup)
        }
    }

-    if (delay) {
+    if (delay != std::chrono::duration<int, std::milli>::zero()) {


If you want to use the method, better to just say delay.zero(). But you may as well just say 0s in normal life, I think. Using the zero() method is only really worth it where it's important to maintain the specific type.

Yes 0s is 64-bit, so it does make it a != comparison of a 32-bit millisecond count to a 64-bit zero, but the compiler should, I hope, realise it doesn't actually need to do any 64-bit work for that. Maybe you could check if != 0s is bigger than != delay.zero() on each toolchain? Put my mind at rest.

(But as this is test code, I wouldn't even sweat 32-bit/64-bit anyway. I'd have just made delay be milliseconds (ie 64-bit) to save typing.)

And you already used 0s above when you initialised it, so you're not being consistent!

kjbracey · 2020-05-07T07:37:54Z

TESTS/usb_device/basic/USBTester.cpp

    queue = mbed::mbed_highprio_event_queue();
+#else


Is there any record in the logs of why it was using high priority specifically? I guess it's working as normal.

If there's no downside, I'd make it normal priority always. If there is a downside, put a comment about it here.

kjbracey · 2020-05-07T07:38:33Z

TESTS/usb_device/basic/USBTester.cpp

@@ -45,6 +43,7 @@

 #define EVENT_READY (1 << 0)

+using namespace std::chrono_literals;


Don't forget you can using namespace std::chrono instead to save typing std::chrono::duration. or std::chrono::milliseconds.

0xc0170 · 2020-05-08T08:40:40Z

TESTS/usb_device/basic/USBEndpointTester.cpp

@@ -173,7 +171,6 @@ USBEndpointTester::USBEndpointTester(USBPhy *phy, uint16_t vendor_id, uint16_t p
        MBED_ASSERT(_endpoint_buffs[i] != NULL);
    }
    MBED_ASSERT(resolver.valid());
-    queue = mbed::mbed_highprio_event_queue();


by removing the queue here, the test does not change - it has not been used at all?

yes. This queue initially added by Russ to create a utility function but the scope of this work became obsolete. This queue is not used any of the test cases on full profile.

0xc0170 · 2020-05-08T08:41:24Z

TESTS/usb_device/basic/USBTester.cpp

@@ -32,7 +30,6 @@
 #define VENDOR_TEST_CTRL_NONE               3
 #define VENDOR_TEST_CTRL_IN_DELAY           4
 #define VENDOR_TEST_CTRL_OUT_DELAY          5
-#define VENDOR_TEST_CTRL_NONE_DELAY         6


why is this delay removed? Shall this be a series of commits rather than "enable usb basic" - does not provide a reason for some changes I am commenting now on

Removed the USB endpoint test class unused variable and partially (The greentea test host side implementation is missed on pyusb_basic.py ) implemented USB request control transfer of VENDOR_TEST_CTRL_NONE_DELAY so it can run with the bare metal profile.

This would explain some changes, should be in the commit msg as well.

@0xc0170 Done. Please check

I don't think it is worth deleting VENDOR_TEST_CTRL_NONE_DELAY (here and in the .py file) because it breaks the defines and it is not the only one not implemented. For example I don't see any code calling VENDOR_TEST_CTRL_IN_STATUS_DELAY or VENDOR_TEST_CTRL_OUT_STATUS_DELAY

After discussion with @evedon have decided to remove all unused USB control transfer request type and also partially implemented test.

Pull request has been modified.

- Enable the USB hid and basic test for bare metal profile. - Remove the partially (The green tea test host side implementation is missing on pyusb_basic.py ) implemented USB control transfer VENDOR_TEST_CTRL_NONE_DELAY request type test case as it is becoming obsolete. - Remove high priority event queue which is used by that request type test case.

Pull request has been modified.

- Remove the partially implemented VENDOR_TEST_CTRL_NONE USB control request type. - Remove the USB control request type VENDOR_TEST_CTRL_IN_DELAY, VENDOR_TEST_CTRL_OUT_DELAY, VENDOR_TEST_CTRL_IN_STATUS_DELAY, VENDOR_TEST_CTRL_OUT_STATUS_DELAY macros as did not have any test.

TESTS/usb_device/basic/USBEndpointTester.cpp

evedon · 2020-05-12T11:06:05Z

@adbridge This PR is for Mbed 6.0

mbed-ci · 2020-05-12T11:56:28Z

Test run: SUCCESS

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

ciarmcom requested review from evedon and a team May 4, 2020 19:00

ciarmcom added the needs: review label May 4, 2020

rajkan01 force-pushed the usb_basic_hid_greentea branch from 4b2fcd6 to db8f07e Compare May 5, 2020 17:15

evedon reviewed May 6, 2020

View reviewed changes

TESTS/usb_device/basic/USBTester.h Outdated Show resolved Hide resolved

rajkan01 force-pushed the usb_basic_hid_greentea branch from db8f07e to 5bdd2c1 Compare May 6, 2020 17:40

kjbracey reviewed May 7, 2020

View reviewed changes

rajkan01 force-pushed the usb_basic_hid_greentea branch 2 times, most recently from a14eeae to e6195ef Compare May 7, 2020 17:09

0xc0170 previously requested changes May 8, 2020

View reviewed changes

mergify bot added needs: work and removed needs: review labels May 8, 2020

rajkan01 force-pushed the usb_basic_hid_greentea branch from e6195ef to 772c7bd Compare May 11, 2020 10:46

rajkan01 force-pushed the usb_basic_hid_greentea branch from 772c7bd to f7f0681 Compare May 11, 2020 10:51

rajkan01 changed the title ~~Bare metal profile: Enable USB basic and hid greentea test~~ USB basic and hid greentea test cleanup and enable to the bare metal profile May 11, 2020

0xc0170 previously approved these changes May 11, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: work labels May 11, 2020

rajkan01 force-pushed the usb_basic_hid_greentea branch from 353d263 to 9e657c6 Compare May 11, 2020 19:54

Cleanup

61c70e0

- Remove the partially implemented VENDOR_TEST_CTRL_NONE USB control request type. - Remove the USB control request type VENDOR_TEST_CTRL_IN_DELAY, VENDOR_TEST_CTRL_OUT_DELAY, VENDOR_TEST_CTRL_IN_STATUS_DELAY, VENDOR_TEST_CTRL_OUT_STATUS_DELAY macros as did not have any test.

rajkan01 force-pushed the usb_basic_hid_greentea branch from 9e657c6 to 61c70e0 Compare May 11, 2020 19:55

evedon reviewed May 12, 2020

View reviewed changes

TESTS/usb_device/basic/USBEndpointTester.cpp Show resolved Hide resolved

evedon approved these changes May 12, 2020

View reviewed changes

0xc0170 added the release-version: 6.0.0-rc1 label May 12, 2020

mergify bot added ready for merge and removed needs: CI labels May 12, 2020

0xc0170 merged commit 631e81e into ARMmbed:master May 12, 2020

mergify bot removed the ready for merge label May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

USB basic and hid greentea test cleanup and enable to the bare metal profile #12916

USB basic and hid greentea test cleanup and enable to the bare metal profile #12916

Uh oh!

rajkan01 commented May 4, 2020 •

edited

Loading

Uh oh!

ciarmcom commented May 4, 2020

Uh oh!

Uh oh!

kjbracey May 7, 2020

Uh oh!

kjbracey May 7, 2020

Uh oh!

kjbracey May 7, 2020

Uh oh!

0xc0170 May 8, 2020

Uh oh!

rajkan01 May 11, 2020

Uh oh!

0xc0170 May 8, 2020

Uh oh!

0xc0170 May 8, 2020

Uh oh!

rajkan01 May 11, 2020

Uh oh!

evedon May 11, 2020

Uh oh!

rajkan01 May 12, 2020

Uh oh!

Uh oh!

evedon commented May 12, 2020

Uh oh!

mbed-ci commented May 12, 2020

Uh oh!

Uh oh!

		@@ -45,6 +43,7 @@

		#define EVENT_READY (1 << 0)

		using namespace std::chrono_literals;

USB basic and hid greentea test cleanup and enable to the bare metal profile #12916

USB basic and hid greentea test cleanup and enable to the bare metal profile #12916

Uh oh!

Conversation

rajkan01 commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented May 4, 2020

Uh oh!

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!

Uh oh!

evedon commented May 12, 2020

Uh oh!

mbed-ci commented May 12, 2020

Uh oh!

Uh oh!

rajkan01 commented May 4, 2020 •

edited

Loading