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

USBDevice: Avoid forcing end device to be derived from USBDevice class #11136

Merged
merged 2 commits into from Aug 23, 2019

Conversation

@facchinm
Copy link
Contributor

commented Jul 31, 2019

Description

By replacing the existing callbacks with proper mbed::callback we can use the USBDevice infrastructure without deriving directly from USBDevice.

The patch could bring a (tiny, big ?) performance hit due to the indirection, but would make the USBDevice class usable as a singleton.

A usage example can be found here https://github.com/arduino/ArduinoCore-nRF528x-mbedos/tree/master/cores/arduino/USB and https://github.com/bcmi-labs/ArduinoCore-nRF528x-mbedos/tree/master/libraries/USBHID

PluggableUSBDevice implements USBDevice and calls phy init() just once; all device drivers (derived from mbed ones) plug on it to create a composite device but they don't derive from USBDevice.

Pull request type

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

Reviewers

Release Notes

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-maintainers Jul 31, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

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

@0xc0170 0xc0170 changed the title [USBDevice] Avoid forcing end device to be derived from USBDevice class USBDevice: Avoid forcing end device to be derived from USBDevice class Aug 8, 2019
@0xc0170 0xc0170 requested a review from maciejbocianski Aug 8, 2019
@0xc0170
0xc0170 approved these changes Aug 8, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

I can see there are lot of conflicts, please review

@0xc0170 0xc0170 added needs: work and removed needs: review labels Aug 8, 2019
@facchinm facchinm force-pushed the facchinm:avoid_derive_needed_usbdevice branch from a0c6d9c to 811098f Aug 8, 2019
@facchinm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Rebased upon master, thanks.

@0xc0170 0xc0170 added needs: review and removed needs: work labels Aug 8, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@maciejbocianski please review

Copy link
Member

left a comment

Generally looks good,
tests-usb_device-basic needs adaptation to your changes, you can use my code maciejbocianski@092b26b

@fkjagodzinski @c1728p9 what do you think

@@ -141,21 +142,6 @@ class USBDevice: public USBPhyEvents {
*/
bool endpoint_add(usb_ep_t endpoint, uint32_t max_packet, usb_ep_type_t type, ep_cb_t callback = NULL);

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 12, 2019

Contributor

If you're changing to Callback, could you please avoid giving it NULL - change to nullptr. Will avoid issues with some optimisation work I'm doing on Callback. Will save me touching this again later.

(That does make it a 5.14 change - we're not putting any C++11-isms into 5.13 patches. But this doesn't look like a patch anyway).

@fkjagodzinski

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Yeah, I think this patch OK but can't really comment on performance aspect.

The patch could bring a (tiny, big ?) performance hit due to the indirection, but would make the USBDevice class usable as a singleton.

@c1728p9 could you comment on that, please?

Copy link
Contributor

left a comment

This PR seems reasonable to me. Does the use of callback increase or decrease RAM used? Also, you'll need a few more changes if you want to make USBDevice usable without inheriting from it:

  • Protected functions need to be made public so they can be called

Those changes could be made in a separate PR though as this change look good on its own merits.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 20, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

CI started

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

@facchinm Is this breaking change or functionality change? If yes, Release Notes should be added to your first comment above.

@facchinm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@0xc0170 the only breaking change is the removal of template<typename T> bool endpoint_add(usb_ep_t endpoint, uint32_t max_packet, usb_ep_type_t type, void (T::*callback)() overload that could affect 3rd party, out of tree USB classes (I didn't find any in the wild but they could exist of course).
I'm amending the first commit and add this information

@0xc0170 0xc0170 requested a review from bulislaw Aug 21, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Aug 21, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
@0xc0170 0xc0170 added needs: work and removed needs: CI labels Aug 21, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Please review build logs, there are failures

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

CI restarted

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Aug 21, 2019
@facchinm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

By fixing the tests I came up with another implementation, much cleaner and, more important, without breaking changes.
Should I close this PR and open another or forcepushing is ok?

In this way we can use the USBDevice infrastructure without deriving directly from USBDevice.
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

You can force push

@facchinm facchinm force-pushed the facchinm:avoid_derive_needed_usbdevice branch from f433881 to 84e228a Aug 21, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Once you do force push , please update PR with the current status to notify us - what has changed.

@facchinm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Sorry, I updated the description on the top but probably this didn't trigger any notification.
Anyway, the new implementation leave everything in its place and adds the possibility to use a regular Callback as argument.

@0xc0170 0xc0170 requested a review from maciejbocianski Aug 22, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Ci restarted

@maciejbocianski Please review

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 22, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 added needs: review and removed needs: CI labels Aug 23, 2019
Copy link
Member

left a comment

LGTM

@0xc0170 0xc0170 merged commit 17fd795 into ARMmbed:master Aug 23, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
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 RTOS ROM(+0 bytes) RAM(+0 bytes)
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 Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8562 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@fkjagodzinski

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

Does the use of callback increase or decrease RAM used?

FYI, this patch increased the size of the USBDevice class by 240 B. It is now 824 B.

@fkjagodzinski

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

@donatieng, @ARMmbed/mbed-os-hal, this information ^^^ might be useful for future reference. I think there's room for improvement if needed.

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