Skip to content

Reorganize vendor specific USB Device/Host code into targets folders #4277

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
merged 1 commit into from
Jun 27, 2017
Merged

Reorganize vendor specific USB Device/Host code into targets folders #4277

merged 1 commit into from
Jun 27, 2017

Conversation

screamerbg
Copy link
Contributor

@screamerbg screamerbg commented May 5, 2017 •
edited
Loading

Description

Reorganize vendor specific USB Device and USB Host code into targets folders, following the mbed-os/targets/ design pattern.

Status

READY

Migrations

NO

Related PRs

A continuation of #4248

Todos

  • Testing
    @ashok-rao Will test this PR against physical targets from each vendor.

@screamerbg screamerbg requested a review from TomoYamanaka May 5, 2017 13:25
@ccli8
Copy link
Contributor

ccli8 commented May 8, 2017

@screamerbg Please re-organize Nuvoton USB device/host files as below:
USBEndpoints_NUC472.h to mbed-os/targets/TARGET_NUVOTON/TARGET_NUC472
USBHAL_NUC472.cpp to mbed-os/targets/TARGET_NUVOTON/TARGET_NUC472
USBEndpoints_M453.h to mbed-os/targets/TARGET_NUVOTON/TARGET_M451
USBHAL_M453.cpp to mbed-os/targets/TARGET_NUVOTON/TARGET_M451
USBHALHost_NUC472.cpp to mbed-os/targets/TARGET_NUVOTON/TARGET_NUC472
USBHALHost_M451.cpp to mbed-os/targets/TARGET_NUVOTON/TARGET_M451

@0xc0170
Copy link
Contributor

0xc0170 commented May 11, 2017

@screamerbg Please re-organize Nuvoton USB device/host files as below:
USBEndpoints_NUC472.h to mbed-os/targets/TARGET_NUVOTON/TARGET_NUC472
USBHAL_NUC472.cpp to mbed-os/targets/TARGET_NUVOTON/TARGET_NUC472
USBEndpoints_M453.h to mbed-os/targets/TARGET_NUVOTON/TARGET_M451
USBHAL_M453.cpp to mbed-os/targets/TARGET_NUVOTON/TARGET_M451
USBHALHost_NUC472.cpp to mbed-os/targets/TARGET_NUVOTON/TARGET_NUC472
USBHALHost_M451.cpp to mbed-os/targets/TARGET_NUVOTON/TARGET_M451

Any update?

@screamerbg
Copy link
Contributor Author

screamerbg commented May 11, 2017 •
edited
Loading

@ccli8 The USB related files cannot reside under mbed-os/targets due to USB being unsupported feature in mbed OS.

As I mentioned in a different thread, we are currently in a process of consolidating the differences of the USB Device on mbed.org and here in mbed-os on Github, after which we will start mirroring the mbed-os/unsupported/USBDevice to USBDevice on mbed.org and provide a way for regular updates of USBDevice.

Currently if USBDevice files are in mbed-os/targets/, then they won't be backwards compatible with the USBDevice library on mbed.org as it exists today. E.g. All USBDevice support files live under mbed-os/features/unsupported/USBDevice, not under mbed-os/targets.

Note that once USBDevice is officially introduced in mbed OS 5, then:

  • The USBDevice support files will be migrated back to the mbed-os/targets; and also
  • The USBDevice library on mbed.org will be deprecated as it will be natively provided as a feature of mbed-os.

@ccli8
Copy link
Contributor

ccli8 commented May 12, 2017

@screamerbg Thanks for comment.

@screamerbg
Copy link
Contributor Author

@ccli8 @cyliangtw @adustm @jeromecoutant @jessexm @jamike @TomoYamanaka @mmahadevan108 Could you please approve the pull request if you're happy with it? (you've been assigned as approver for this PR)

ccli8
ccli8 approved these changes May 15, 2017
View reviewed changes
Copy link
Contributor

@ccli8 ccli8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@screamerbg
As you decide to move USB files to mbed-os/targets, please note to move Nuvoton USB files to
mbed-os/targets/TARGET_NUVOTON/TARGET_NUC472 and mbed-os/targets/TARGET_NUVOTON/TARGET_M451.

@jamike
Copy link
Contributor

jamike commented May 15, 2017

#4291 is fixing issue for USBHOST on target STM : This patch keeps the USBHALHost file for target STM still in the directory USBDevice , instead of beeing placed in USBHOST directory as done in #4291.
As in current version on master ,USBHost build for TARGET_STM is failed, it is still failed with this patch. Can we merged 1st 4291, and then 4277 reworked after having test USBHOST Build for TARGET_STM.,

@0xc0170
Copy link
Contributor

0xc0170 commented May 15, 2017

#4291 is fixing issue for USBHOST on target STM : This patch keeps the USBHALHost file for target STM still in the directory USBDevice , instead of beeing placed in USBHOST directory as done in #4291.

Merged, this PR should be now rebased (=conflicts), thanks

@screamerbg
Copy link
Contributor Author

@0xc0170 is that a "please rebase"?

@theotherjimmy
Copy link
Contributor

Yes @screamerbg Please rebase this PR so that we can re-run testing and merge it.

TomoYamanaka
TomoYamanaka reviewed May 17, 2017
View reviewed changes
Copy link
Contributor

@TomoYamanaka TomoYamanaka left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for this late reply.

LGTM.
But, I have one question.
Can I think that the folder structures of USB Host and USB Device for mbed classic will not change?

@mmahadevan108
Copy link
Contributor

@screamerbg LGTM

@ashok-rao
Copy link
Contributor

@mmahadevan108 , can you please take a look at this : #4250 . Thanks!

@adbridge
Copy link
Contributor

@screamerbg bump...

@mmahadevan108 mmahadevan108 mentioned this pull request Jun 2, 2017
Merged
1 task
@sg- sg- removed the needs: review label Jun 3, 2017
@sg-
Copy link
Contributor

sg- commented Jun 15, 2017

@screamerbg bump

@screamerbg
Copy link
Contributor Author

@sg- Rebased

@theotherjimmy
Copy link
Contributor

@jeromecoutant @jessexm @mmahadevan108 @jamike Could you review?

@jeromecoutant
Copy link
Collaborator

Hi
Maybe, we could apply #4400 into this pull request ?

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 20, 2017

Maybe, we could apply #4400 into this pull request ?

What would be the reason? can we do it as 2 step process? First this one then the latter one

@jeromecoutant
Copy link
Collaborator

OK, no problem

@theotherjimmy
Copy link
Contributor

@screamerbg Are you still waiting on some more reviews?

@screamerbg
Copy link
Contributor Author

@0xc0170 can you stop accepting PRs that create merge issues with this??

@screamerbg screamerbg mentioned this pull request Jun 27, 2017
Merged
@theotherjimmy
Copy link
Contributor

@screamerbg @0xc0170 I broke this one recently. I'm going to rebase it to remove the conflicts I introduced.

@theotherjimmy
Copy link
Contributor

Alrighty. Rebase done. I'm going to merge this when the three CI's return back success.

@theotherjimmy theotherjimmy mentioned this pull request Jun 27, 2017
Closed
@theotherjimmy
Copy link
Contributor

@screamerbg Travis passed! LGTM!

@theotherjimmy
Copy link
Contributor

@0xc0170 Care to merge? I'll merge it in a bit if you don't.

@theotherjimmy theotherjimmy merged commit b89fd03 into ARMmbed:master Jun 27, 2017
@theotherjimmy
Copy link
Contributor

@screamerbg It's in!

@jeromecoutant
Copy link
Collaborator

It's in!

But you have removed #4400.... :-(
I will re-push it....

@jeromecoutant jeromecoutant mentioned this pull request Jul 11, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TomoYamanaka TomoYamanaka TomoYamanaka left review comments

@cyliangtw cyliangtw cyliangtw approved these changes

@ccli8 ccli8 ccli8 approved these changes

@adbridge adbridge Awaiting requested review from adbridge

@jeromecoutant jeromecoutant Awaiting requested review from jeromecoutant

@jessexm jessexm Awaiting requested review from jessexm

@mmahadevan108 mmahadevan108 Awaiting requested review from mmahadevan108

@jamike jamike Awaiting requested review from jamike

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.