Skip to content
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

Add USB HID tests #10096

Merged
merged 7 commits into from Apr 17, 2019

Conversation

Projects
None yet
8 participants
@fkjagodzinski
Copy link
Member

commented Mar 14, 2019

Description

Add tests-usb_device-hid test suite for USBHID, USBMouse & USBKeyboard.

Pull request type

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

Reviewers

@c1728p9, @jamesbeyond, @maciejbocianski

Release Notes

requirements.txt Outdated
@@ -23,4 +23,5 @@ git+https://github.com/armmbed/manifest-tool.git@v1.4.6
icetea>=1.2.1,<1.3
pycryptodome>=3.7.2,<=3.7.3
pyusb>=1.0.0,<2.0.0
hidapi>=0.7.99,<0.8.0

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Mar 14, 2019

Author Member

@c1728p9, @jamesbeyond I'd like to clarify the licensing of this module. Based on its LICENSE file it can be used under one of 3 licenses. Should I specify, which one we want to use? If so, how do I do that?

This comment has been minimized.

Copy link
@cmonr

cmonr Mar 15, 2019

Contributor

@0xc0170 Might be a question for you as well

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Mar 15, 2019

Member

I need more context here, one of 3 licenses? Does this module hidapi have dual license or?

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Mar 15, 2019

Author Member

Yes 3

cython-hidapi may be used by one of three licenses as outlined in LICENSE.txt

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Mar 15, 2019

Member

Got it . This is missing in our license file (capturing python dependencies there!) , can you fix that ? IT would be added there as well - hidapi under BSD license

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Mar 15, 2019

Member

Created #10123

@ciarmcom ciarmcom requested review from c1728p9, jamesbeyond, maciejbocianski and ARMmbed/mbed-os-maintainers Mar 14, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

@fkjagodzinski

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

I can see Travis CI failed when installing the hidapi Python module added to requirements.txt in this PR.

@0xc0170 @cmonr How can I add the missing prerequisites to the travis config? Or should we avoid introducing such modification just to be able to run one test case? (Currently, all USB tests are skipped on the CI anyway.) I can update the python code to handle the import failure and skip this test case.

@fkjagodzinski fkjagodzinski force-pushed the fkjagodzinski:test-usb-hid branch to a845863 Mar 14, 2019

@fkjagodzinski

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

OK, with a temporary fix, tests also work on Windows hosts. I'll prepare a proper fix shortly.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

How can I add the missing prerequisites to the travis config?

Oh jeez, this just got really nasty really quick.

if I understand correctly, the new python module also requires additional system packages?
Not sure I want to force users to install something else before even doing a pip install.

@fkjagodzinski

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

if I understand correctly, the new python module also requires additional system packages?
Not sure I want to force users to install something else before even doing a pip install.

Yeah, this is not ideal, but applies only to Linux users. I can see two possible solutions here:

  1. make that single test case optional (run only if the host machine has hidapi already installed), and possibly add the TESTS/usb_device/hid/{requirements.txt,README} files,
  2. rewrite the test case to use pyusb -- the transmitted data is treated as raw anyway, no HID specific parsing here.

The second option seems better, but has a drawback -- the host side HID driver will be excluded, making the test less valuable. USB transfers through pyusb are already tested with basic USB tests.

@c1728p9 what do you think?

usb_hid.connect();
greentea_send_kv(MSG_KEY_TEST_RAW_IO, REPORT_SIZE);
usb_hid.wait_ready();
wait_ms(500);

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Mar 15, 2019

Contributor

Why the delay?

This comment has been minimized.

Copy link
@fkjagodzinski

fkjagodzinski Mar 22, 2019

Author Member

I added that temporarily to handle host side driver setup. It turned out to take much longer on Windows than Linux. It is replaced with a greentea_parse_kv() now.

@c1728p9

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Hi @fkjagodzinski I would prefer to go with option 1 - make the test optional on linux. As you mentioned, I don't think pyusb will add much testing value as it bypasses the host's driver. @donatieng what do you think?

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

@fkjagodzinski fkjagodzinski force-pushed the fkjagodzinski:test-usb-hid branch from a845863 to 8c6f63e Mar 22, 2019

@cmonr cmonr added needs: work and removed needs: review labels Mar 25, 2019

@fkjagodzinski

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

The PR is ready for review again. Latest updates provide:

  • a proper fix for variable setup time of the host HID driver -- this was exposed on Windows; now the device simply waits for a greentea message from the host,
  • update for the requirements.txt file -- hidapi is skipped on Linux because of its dependencies; the module can still be easily installed manually; if the module is missing on the host machine, the one test case that requires it is skipped,
  • update for LICENSE.md providing info about hidapi and pyusb licenses.
@adbridge

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

@c1728p9, @jamesbeyond, @maciejbocianski Can we get this re-reviewed please?

fkjagodzinski added some commits Mar 13, 2019

USBHID: Fix the initial HID report read operation
The first 4 bytes received were lost due to a wrong address.
Read the output report into HID_REPORT.data.
Tests: USBHID: Reuse VID & PID from basic test
To successfully use pyusb on Windows hosts, a Zadig configuration has to
be performed. Since config for basic tests has already been provided,
use it again.
Tests: USBHID: Handle variable driver setup time
Wait for the host driver to finish setup before sending any HID reports
from the device.

USBHID::wait_ready() blocks until the device reaches 'configured' state,
but the state of the host HID driver remains unknown to the device.
Tests: USBHID: Remove unnecessary wait_ready call
Every test case waits at greentea_parse_kv() anyway.
Tests: USBHID: Make report test optional on Linux
This test case uses `hidapi` -- a cross-platform Python module.
To keep the initial Mbed setup as simple as possible, the `hidapi`
module is skipped on Linux hosts because of its external dependancies
for this platform.

The module can be easily installed following instructions from the
README file.
The test case is skipped if the host machine lacks `hidapi` module.

@fkjagodzinski fkjagodzinski force-pushed the fkjagodzinski:test-usb-hid branch from e7021b2 to 491ea55 Apr 9, 2019

@fkjagodzinski

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

I've rebased the commits onto current master and fixed the LICENSE.md conflict.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 9, 2019

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_exporter
@fkjagodzinski

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

I've checked the errors; they're unrelated to this PR.

  • jenkins-ci/mbed-os-ci_cloud-client-test
ExampleAppCloudTest01_Rel1_2.K66F
Dut Initialization failed, reason(s): DUT index 1 - RaaS DUT flashing failed
  • jenkins-ci/mbed-os-ci_exporter
# Failed build combinations
################################################################################
# mbed-os-example-blinky NUCLEO_F429ZI make_armc6
armclang: error: Failed to check out a license.
@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

Tests restarted

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

CI job restarted: jenkins-ci/cloud-client-test

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 17, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 merged commit 6b9d157 into ARMmbed:master Apr 17, 2019

26 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10321 cycles (+1157 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@fkjagodzinski fkjagodzinski deleted the fkjagodzinski:test-usb-hid branch Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.