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

USB public APIs cleanup #11034

Merged
merged 2 commits into from Jul 18, 2019
Merged

USB public APIs cleanup #11034

merged 2 commits into from Jul 18, 2019

Conversation

gpsimenos
Copy link
Contributor

@gpsimenos gpsimenos commented Jul 12, 2019

Description

This PR refactors the usb directory according to https://jira.arm.com/browse/IOTCLOUDPR-3296.

  • The contents of the usb directory were moved to appropriate locations and the usb directory removed.
  • Public USB headers moved under drivers/
  • Internal USB headers moved under drivers/internal/
  • USB Source code moved under drivers/source/usb/
  • Moved usb/device/hal/ under hal/usb/
  • Moved usb/device/USBPhy/ under hal/usb/
  • Merged usb/device/targets/ into targets/
  • Separated public and private USB API documentation under Doxygen groups drivers-public-api and drivers-internal-api.

Pull request type

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

Reviewers

@evedon
@hugueskamba

@40Grit
Copy link

40Grit commented Jul 12, 2019

@AGlass0fMilk

@ciarmcom ciarmcom requested review from evedon, hugueskamba and a team July 12, 2019 11:00
@ciarmcom
Copy link
Member

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

@evedon
Copy link
Contributor

evedon commented Jul 12, 2019

@gpsimenos ResetReason.cpp needs to move to source
Please also update 3rd bullet in PR description

Copy link
Contributor

@evedon evedon left a comment

Choose a reason for hiding this comment

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

Some rework needed.

In drivers/internal/Task.h, there is #include "events/TaskBase.h"
TaskBase.h is under drivers/source/usb/device/utilities/events/. It should be in drivers/internal

Same for TaskQueue.h AsyncOp.h etc.

@hugueskamba
Copy link
Collaborator

hugueskamba commented Jul 12, 2019

@gpsimenos
The doxy-spellcheck CI test fails because some words in the content now part of Doxygen has incorrect spelling/case.

The incorrect words are:

  • sdcard
  • leds
  • async
  • Unstall
  • Unstalling
  • irq
  • deinit

You can fix the issue by replacing the first six words with:

  • SD card
  • LEDs
  • asynchronous
  • Un-stall
  • Un-stalling
  • IRQ

Note: You cannot just do a replace all as it might change content that is not part of the Doxygen comments (i.e c/cpp code).

For deinit, you cannot change it as the Doxygen comment refers to a function name deinit(). Instead, do what the script suggests at the bottom of the failure.

If any of the failed words should be considered valid please add them to the ignore.en.pws file
found in tools/test/scripts/doxy-spellchecker between the _code_ and _doxy_ tags.

So add deinit to that file.

@gpsimenos
Copy link
Contributor Author

@gpsimenos ResetReason.cpp needs to move to source
Please also update 3rd bullet in PR description

That's not part of USB. Should I move it regardless?

@evedon
Copy link
Contributor

evedon commented Jul 12, 2019

@gpsimenos ResetReason.cpp needs to move to source
Please also update 3rd bullet in PR description

That's not part of USB. Should I move it regardless?

Hugues already did it. Rebase your PR to pick up the change.

@gpsimenos
Copy link
Contributor Author

Some rework needed.

In drivers/internal/Task.h, there is #include "events/TaskBase.h"
TaskBase.h is under drivers/source/usb/device/utilities/events/. It should be in drivers/internal

Same for TaskQueue.h AsyncOp.h etc.

Fixed

@hugueskamba
Copy link
Collaborator

hugueskamba commented Jul 17, 2019

@gpsimenos
8e84d9a does not fix the Doxygen spelling. You missed out async and irq.
Run ./tools/test/travis-ci/doxy-spellchecker/spell.sh drivers/ to make sure all is fixed.

@gpsimenos
The doxy-spellcheck CI test fails because some words in the content now part of Doxygen has incorrect spelling/case.

The incorrect words are:

  • sdcard
  • leds
  • async
  • Unstall
  • Unstalling
  • irq
  • deinit

You can fix the issue by replacing the first six words with:

  • SD card
  • LEDs
  • asynchronous
  • Un-stall
  • Un-stalling
  • IRQ

Note: You cannot just do a replace all as it might change content that is not part of the Doxygen comments (i.e c/cpp code).

For deinit, you cannot change it as the Doxygen comment refers to a function name deinit(). Instead, do what the script suggests at the bottom of the failure.

If any of the failed words should be considered valid please add them to the ignore.en.pws file
found in tools/test/scripts/doxy-spellchecker between the _code_ and _doxy_ tags.

So add deinit to that file.

@hugueskamba
Copy link
Collaborator

The unit tests fails with the following error:

[Fatal Error] TaskQueue.h@21,10: events/TaskBase.h: No such file or directory
[DEBUG] Return: 1
[DEBUG] Output: In file included from ./drivers/internal/PolledQueue.h:21,
[DEBUG] Output:                  from ./drivers/USBMSD.h:25,
[DEBUG] Output:                  from ./drivers/source/usb/device/USBMSD.cpp:20:
[DEBUG] Output: ./drivers/internal/TaskQueue.h:21:10: fatal error: events/TaskBase.h: No such file or directory
[DEBUG] Output:  #include "events/TaskBase.h"
[DEBUG] Output:           ^~~~~~~~~~~~~~~~~~~
[DEBUG] Output: compilation terminated.
Failed to build library
Memory map breakdown for built projects (values in Bytes):
| name | target | toolchain | static_ram | total_flash |
|------|--------|-----------|------------|-------------|


Build failures:
  * K64F::GCC_ARM::MBED-BUILD
        
[mbed] ERROR: "/usr/bin/python" returned error.
       Code: 1
       Path: "/home/hugkam01/projects/mbed-os-projects/mbed-os"
       Command: "/usr/bin/python -u /home/hugkam01/projects/mbed-os-projects/mbed-os/tools/test.py -D MBED_TEST_MODE -t GCC_ARM -m k64f -c --source . --build ./BUILD/tests/K64F/GCC_ARM --test-spec ./BUILD/tests/K64F/GCC_ARM/test_spec.json --build-data ./BUILD/tests/K64F/GCC_ARM/build_data.json -n mbed-os-tests-usb* -v --greentea"
       Tip: You could retry the last command with "-v" flag for verbose output

Basically there is still references to events/TaskBase.h instead of drivers/internal/TaskBase.h

$ grep -ir 'TaskBase.h' .
./drivers/internal/TaskQueue.h:#include "events/TaskBase.h"
./drivers/internal/Task.h:#include "drivers/internal/TaskBase.h"
./drivers/source/usb/device/utilities/events/TaskBase.cpp:#include "events/TaskBase.h"

Copy link
Contributor

@evedon evedon left a comment

Choose a reason for hiding this comment

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

Please make sure that all greentea tests are passing.
And check with tools team if they need some refactoring due to removal of usb/mbed_lib.json file.

Other than that I am okay with the PR.

usb/mbed_lib.json Show resolved Hide resolved
@hugueskamba
Copy link
Collaborator

Please make sure that all greentea tests are passing.
And check with tools team if they need some refactoring due to removal of usb/mbed_lib.json file.

Other than that I am okay with the PR.

The Greentea tests are failing.

@hugueskamba
Copy link
Collaborator

The copyright year for modified files need to be updated.

@evedon evedon force-pushed the feature-public-headers branch 2 times, most recently from 9f60f9f to 1a6fa7c Compare July 18, 2019 11:41
* Separate internal APIs from public APIs
* Move source files to source subdirs
* Move internal headers to internal subdirs
* Add Doxygen comments for documenting internal and public APIs
@evedon evedon merged commit d17fec5 into ARMmbed:feature-public-headers Jul 18, 2019
hugueskamba pushed a commit to hugueskamba/mbed-os that referenced this pull request Jul 18, 2019
The contents of the usb directory were moved to appropriate locations and the usb directory removed.

* Public USB headers moved under drivers/
* Internal USB headers moved under drivers/internal/
* USB Source code moved under drivers/source/usb/
* Moved usb/device/hal/ under hal/usb/
* Moved usb/device/USBPhy/ under hal/usb/
* Merged usb/device/targets/ into targets/
* Separated public and private USB API documentation under Doxygen groups drivers-public-api and drivers-internal-api.
evedon pushed a commit that referenced this pull request Jul 22, 2019
The contents of the usb directory were moved to appropriate locations and the usb directory removed.

* Public USB headers moved under drivers/
* Internal USB headers moved under drivers/internal/
* USB Source code moved under drivers/source/usb/
* Moved usb/device/hal/ under hal/usb/
* Moved usb/device/USBPhy/ under hal/usb/
* Merged usb/device/targets/ into targets/
* Separated public and private USB API documentation under Doxygen groups drivers-public-api and drivers-internal-api.
@mark-edgeworth
Copy link
Contributor

Looks like there are some issues with tools/tests.py at least: the USB tests at line 672- in this file refer to a directory TESTS/usb/device/... but this seems to be TESTS/usb_device/... However, this change does not appear to have been introduced here, so I question the testing...

@hugueskamba
Copy link
Collaborator

Looks like there are some issues with tools/tests.py at least: the USB tests at line 672- in this file refer to a directory TESTS/usb/device/... but this seems to be TESTS/usb_device/... However, this change does not appear to have been introduced here, so I question the testing...

@jamesbeyond Any comment?

evedon pushed a commit that referenced this pull request Aug 1, 2019
The contents of the usb directory were moved to appropriate locations and the usb directory removed.

* Public USB headers moved under drivers/
* Internal USB headers moved under drivers/internal/
* USB Source code moved under drivers/source/usb/
* Moved usb/device/hal/ under hal/usb/
* Moved usb/device/USBPhy/ under hal/usb/
* Merged usb/device/targets/ into targets/
* Separated public and private USB API documentation under Doxygen groups drivers-public-api and drivers-internal-api.
evedon pushed a commit that referenced this pull request Aug 2, 2019
The contents of the usb directory were moved to appropriate locations and the usb directory removed.

* Public USB headers moved under drivers/
* Internal USB headers moved under drivers/internal/
* USB Source code moved under drivers/source/usb/
* Moved usb/device/hal/ under hal/usb/
* Moved usb/device/USBPhy/ under hal/usb/
* Merged usb/device/targets/ into targets/
* Separated public and private USB API documentation under Doxygen groups drivers-public-api and drivers-internal-api.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants