Add USB endpoint tests #6880

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.

Sign up for GitHub

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

Jump to bottom

Merged

0xc0170 merged 10 commits into ARMmbed:feature-hal-spec-usb-device from fkjagodzinski:test-feature-hal-spec-usb-device

Jul 18, 2018

Member

fkjagodzinski commented May 11, 2018 •

edited

Loading

Description

Extend tests-usb_device-basic with:

endpoint test abort,
endpoint test data correctness,
endpoint test data toggle reset,
endpoint test halt,
endpoint test parallel transfers,
endpoint test parallel transfers ctrl.

Pull request type

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

CC @c1728p9 @maciejbocianski @jamesbeyond

Member Author

fkjagodzinski commented May 11, 2018

The comments with test description are missing. I'll add them here or in the consecutive PR.

Member Author

fkjagodzinski commented May 11, 2018 •

edited

Loading

~~The endpoint test abort test case needs #6884 to work.~~

cmonr requested review from maciejbocianski and c1728p9

May 14, 2018 03:59

cmonr added the needs: review label

0xc0170 added the needs: preceding PR label

Member Author

fkjagodzinski commented May 15, 2018

I added the missing docstrings.

maciejbocianski approved these changes

View reviewed changes

c1728p9 reviewed

View reviewed changes

TESTS/host_tests/pyusb_basic.py Outdated

    
                      while time.time() < end_ts and not ctrl_error.is_set():

                          loopback_ep_test(ep_out, ep_in, ep_out.wMaxPacketSize)

                  except usb.core.USBError as err:

                      if err.errno not in (32, 110):

Contributor

c1728p9 May 14, 2018

On windows this error is None when the endpoint is stalled. You might need to remove this check if the error codes aren't portable.

Member Author

fkjagodzinski May 22, 2018

Done.

TESTS/host_tests/pyusb_basic.py Outdated

    
                      while time.time() < end_ts:

                          halt_ep_test(dev, bulk_out, bulk_in, bulk_out, log)

                          bulk_out.clear_halt()

                          intf.set_altsetting()  # Force the device to start reading data from all OUT endpoints again.

Contributor

c1728p9 May 14, 2018

I'm not sure if setting the alternate again is the right way to handle this, but I would like to get your thoughts on this @fkjagodzinski. Some possible alternatives

Require that if a transfer was started then the transfer is resumed when halt is cleared (USBPhy responsible for this)
Have the class driver handle the clear halt request and start the transfer again

Member Author

fkjagodzinski May 15, 2018

It was the simplest solution I was thinking about. I haven't thought about the test class callback for a clear request though. I'll update the code this way.

Member Author

fkjagodzinski May 22, 2018

I added VENDOR_TEST_READ_START device request. It's used to explicitly start reading on given OUT endpoint.

TESTS/host_tests/pyusb_basic.py Outdated

    
                  delayed_halt = Timer(delay, timer_handler)

                  delayed_halt.start()

                  end_ts = time.time() + 1.5 * delay

Contributor

c1728p9 May 14, 2018

This delay wasn't long enough for my machine and caused intermittent failures. It may be good to set this to a really long delay (1 second?), as it will break from the loop early due to the stall anyway.

Member Author

fkjagodzinski May 22, 2018

Done, updated code does not use end_ts at all.

TESTS/host_tests/pyusb_basic.py Outdated

    
                          log('Testing aborting an in progress transfer for IN endpoints.')

                      for ep_in in (bulk_in, interrupt_in):

                          payload_size = (NUM_PACKETS_UNTIL_ABORT + NUM_PACKETS_AFTER_ABORT) * ep_in.wMaxPacketSize

                          payload_in = ep_in.read(payload_size)

Contributor

c1728p9 May 14, 2018

On my setup, Windows 10 using libusb-win32, this raises a timeout exception so payload_in never gets returned:

USBError: [Errno None] libusb0-dll:err [_usb_reap_async] timeout error

Maybe instead of aborting and stopping the transfer, abort and start a new transfer with a buffer with different contents. Alternatively, this could be broken up into two reads, where the first one should complete fully, and the second one should timeout without any data.

Member Author

fkjagodzinski May 22, 2018

Done. Now reading data in chunks of wMaxPacketSize each.

c1728p9 reviewed

View reviewed changes

TESTS/host_tests/pyusb_basic.py Outdated

    
                      if verbose:

                          log('Testing OUT/IN data correctness for bulk endpoint pair.')

                      for payload_size in range(1, bulk_out.wMaxPacketSize + 1):

Contributor

c1728p9 May 16, 2018

Zero is a valid size, so it might not hurt to test that value as well. If you were seeing errors with this earlier #6922 should fix the errors.

Member Author

fkjagodzinski May 22, 2018

Updated; thanks for the reminder!

0xc0170 added needs: work and removed needs: review labels

Contributor

cmonr commented May 21, 2018

Umm... Is this PR still ongoing?

#6884 is now closed...

cmonr removed the needs: preceding PR label

Member Author

fkjagodzinski commented May 21, 2018

@cmonr, Yes, I'm updating the code according to @c1728p9 comments.

fkjagodzinski force-pushed the test-feature-hal-spec-usb-device branch from 6464312 to 8fb7922 Compare

May 22, 2018 16:05

Member Author

fkjagodzinski commented May 22, 2018

Update:

rebased to current feature-hal-spec-usb-device, 675b24e,
fixed host script issues present on Windows machines,
added 0 B payload size to bulk endpoints test,
updated halt and abort tests according to PR comments,
added an explicit request to start reading on OUT endpoints.

c1728p9 mentioned this pull request

Fix USB problems on the LPC1768 #6922

Merged

0xc0170 added needs: review and removed needs: work labels

Contributor

cmonr commented May 31, 2018

@c1728p9 Mind re-reviewing?

c1728p9 force-pushed the feature-hal-spec-usb-device branch from 675b24e to 565ec75 Compare

June 6, 2018 19:48

Contributor

c1728p9 commented Jun 7, 2018

This needs to be rebased but after that I will re-review. cc @fkjagodzinski

cmonr added needs: work and removed needs: review labels

fkjagodzinski force-pushed the test-feature-hal-spec-usb-device branch from 8fb7922 to ea32ceb Compare

June 13, 2018 09:17

Member Author

fkjagodzinski commented Jun 13, 2018

rebased to current feature-hal-spec-usb-device, c753780,
added endpoint test data toggle reset.

cmonr added needs: CI needs: review and removed needs: work labels

fkjagodzinski and others added 4 commits

July 3, 2018 14:16


          Tests: USB: Update ep callbacks

060a800

Endpoint callbacks no longer have endpoint as a param. This update was
introduced in ARMmbed#7267.


          Update test to restart reads after unstall

c44c697

After an endpoint is unstalled any pending transfers are terminated.
This patch re-starts any reads that were ongoing.


          Tests: USB: Decrease the max data size for iso ep

dfeb332

Although the USB spec sets the upper limit on FS isochronous endpoint
payloads to 1023 B, this value is hard to test in practice. Moreover,
not all the targets Mbed OS supports (like NUCLEO_F207ZG) are able to
handle all the endpoints set to max.


          Tests: USB: Update endpoint halt test

9a8b276

Wait for a locally created Timer thread to finish before returning.

fkjagodzinski force-pushed the test-feature-hal-spec-usb-device branch from 1de5b2c to 4759a20 Compare

July 3, 2018 12:57

Member Author

fkjagodzinski commented Jul 3, 2018

rebased to current feature-hal-spec-usb-device, 9165547
updated the test code according to @c1728p9 feedback

c1728p9 approved these changes

View reviewed changes

fkjagodzinski force-pushed the test-feature-hal-spec-usb-device branch from 4759a20 to 5b501e1 Compare

July 17, 2018 08:41


          Tests: USB: Check ep buffer is released by abort

b1fdfd8

Validate that endpoint buffer is not used after a transfer has been
aborted.

fkjagodzinski force-pushed the test-feature-hal-spec-usb-device branch from 5b501e1 to b1fdfd8 Compare

July 17, 2018 09:33

Contributor

0xc0170 commented Jul 17, 2018

/morph build

0xc0170 added needs: CI and removed needs: review labels

mbed-ci commented Jul 17, 2018

Build : SUCCESS

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

Triggering tests

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

mbed-ci commented Jul 17, 2018

Exporter Build : SUCCESS

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

mbed-ci commented Jul 17, 2018

Test : FAILURE

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

cmonr added needs: work and removed needs: CI labels

Member Author

fkjagodzinski commented Jul 18, 2018

@cmonr @0xc0170 I can see that quite a few random test cases timed out. This PR only extends tests-usb_device-basic, so shouldn't have such a huge impact on other test suites. Can we retrigger ci-morph-test?

Contributor

0xc0170 commented Jul 18, 2018

/morph test

mbed-ci commented Jul 18, 2018

Test : FAILURE

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

Contributor

0xc0170 commented Jul 18, 2018 •

edited

Loading

Test results same. We had similar failures 2 weeks ago (no space left on device error), @cmonr please review

Contributor

cmonr commented Jul 18, 2018

@studavekar FYI.

Devices power cycled. Restarting.
/morph test

mbed-ci commented Jul 18, 2018

Test : SUCCESS

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

cmonr added ready for merge and removed needs: work labels

0xc0170 approved these changes

View reviewed changes

Contributor

0xc0170 left a comment

Note, please run astyle for this feature branch once the work is done (might need to tweak astyleignore file))

0xc0170 merged commit 5fa3cb9 into ARMmbed:feature-hal-spec-usb-device

0xc0170 removed the ready for merge label

fkjagodzinski deleted the test-feature-hal-spec-usb-device branch

July 19, 2018 13:39

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet