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

Feature public headers #11073

Merged
merged 10 commits into from Aug 7, 2019

Conversation

@evedon
Copy link
Contributor

commented Jul 19, 2019

Description

Separated platform, usb, drivers, events, and rtos internal APIs from public APIs

  • Moved source files under source subdirs
  • Moved header files included by public headers under internal subdirs
  • Added Doxygen comments for documenting internal and public APIs
  • Removed source code from header files in order to remove include pre-processor directives
  • Used explicit include header files instead of implicit inclusions via third-party header files

Refactored the usb directory

  • 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/
    • Source code moved under drivers/source/usb/device
    • usb/device/hal/ moved under hal/usb/
    • usb/device/USBPhy/ moved under hal/usb/
    • Merged usb/device/targets/ into targets/

Pull request type

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

Reviewers

@bulislaw

Release Notes

This will break user code that was using an internal API using a prefixed path, for example #include "foo/bar.h" instead of #include "bar.h".

@ciarmcom ciarmcom requested review from bulislaw, maclobdell, toyowata and ARMmbed/mbed-os-maintainers Jul 19, 2019
@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

Does it make sense to move all sources under the source/ folder as that actually makes build commands larger, and trips this kind of problems earlier #7129

I don't mind having public header and .cpp in same folder, as long as we are clear on API perspective which are public, and which are internal.

Copy link
Member

left a comment

Could we change the doxygen groups structure a bit: having two main groups public-api and internal-api without split per directory seems like something that is easy to reference to and also should be easier to browse. Then having each of the main directories as it's own group (drivers, platforms, etc). Also it would be good to group related APIs together. So having SPI group for spi, spislave and qspi; i2c for i2c and i2cslave; usb for all usb classes (without group per class); timers for all normal and low power timer related classes; uart for all uart and serial; gpio and analog. There probably some other groupings that would make sense so please add them if you spot something.

Is there a way we can check how much we are affecting the character limit as pointed out by Seppo?

Before that goes in could someone have a look at hand generated doxygen and see if there's something obviously broken? Also would be good to run all the nightiles and examples, Qinghao will be able to help here.

drivers/source/DigitalIn.cpp Show resolved Hide resolved
@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

Does it make sense to move all sources under the source/ folder as that actually makes build commands larger, and trips this kind of problems earlier #7129

I don't mind having public header and .cpp in same folder, as long as we are clear on API perspective which are public, and which are internal.

I don't think it is unreasonnable to move source files under source. This makes for a cleaner directory structure.
Now in order to mitigate the issue with mbed-cli build path length, I would be in favour of flattening the directory structure in other areas, for example under the source directory itself.
For example, mbed-os/drivers/source/usb/device/utilities/events/ is quite a long path.

@loverdeg-ep

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

@evedon I've never understood the reasoning for having headers separate from source except to make it slightly easier to to release them with a precompiled binary.

Is there any other reasoning i'm missing?

@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

Could we change the doxygen groups structure a bit: having two main groups public-api and internal-api without split per directory seems like something that is easy to reference to and also should be easier to browse. Then having each of the main directories as it's own group (drivers, platforms, etc).

We discuss this and I decided that it woud be less confusing if the documentation follows the directory structure.
In the directory structure we have first a split of mbed-os per module and then a split between public and internal api mbed-os\module & mbed-os\module\internal.

Also it would be good to group related APIs together. So having SPI group for spi, spislave and qspi; i2c for i2c and i2cslave; usb for all usb classes (without group per class); timers for all normal and low power timer related classes; uart for all uart and serial; gpio and analog. There probably some other groupings that would make sense so please add them if you spot something.

Agreed that this would be a good improvement.

Is there a way we can check how much we are affecting the character limit as pointed out by Seppo?

I suggest to flatten the directory structure under source

Before that goes in could someone have a look at hand generated doxygen and see if there's something obviously broken?

We will check the hand generated doxygen (we checked for each PR but not for this final one). I have also been in touch with Amanda so she can assess the impact on docs and update the internal links.

Also would be good to run all the nightiles and examples, Qinghao will be able to help here.

Agreed.

@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

@evedon I've never understood the reasoning for having headers separate from source except to make it slightly easier to to release them with a precompiled binary.

Is there any other reasoning i'm missing?

This also allows us to move "private" header files under source.
So this makes for a cleaner directory structure with public headers, internal headers (headers included by public headers) and private headers well separated.

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 19, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR

#include <stddef.h>
#include <stdint.h>

/** \ingroup events */

This comment has been minimized.

Copy link
@bulislaw

bulislaw Jul 19, 2019

Member

I think that should be a public header. I know it's sort of underlying concept, but in some situations (eg bare metal) people may not want to use C++. We also need to do something smart about the both README files, I would say merge them into one. Starting with the C++ description and then introduce concept of underlying C APIs and that they can be used in bare metal c only projects.

This comment has been minimized.

Copy link
@bulislaw

This comment has been minimized.

Copy link
@geky

geky Jul 19, 2019

Member

Well, opinions aside, this just made all the subtrees quite a bit harder to maintain :(

Are we doing the same for littlefs?

This comment has been minimized.

Copy link
@loverdeg-ep

loverdeg-ep Jul 19, 2019

Contributor

I can't even imagine sub-trees being any harder to maintain than they already were when I last tried to use sub trees for dependency management.

This comment has been minimized.

Copy link
@geky

geky Jul 20, 2019

Member

Oh really? I'm actually a big fan of subtrees :) Though as I understand the quality of the command has changed quite a bit over the last half-decade.

It works really well at separating version control from other programmer debates such as monorepo/multirepo, build/dependency systems.

It can get complicated, but one of the best things it has going for it is that, all the effort is on the developer making the subtree merge. After that the user never needs to know (unlike submodules) and even other developers can work on merged directory as though it was part of the repo.

Every so often I'll come back through and create another subtree merge pr to bring the code up to date with what's outside the tree (though I've been slow because of littlefs v2 stuff, sorry about that): #7713

Subtrees really seem like a realization of the idea of truly distributed version control. Because of it repos can exist as copies in a bunch of different places with their own quirks and patches.

This comment has been minimized.

Copy link
@geky

geky Jul 22, 2019

Member

@evedon, good point. I'll move this to a separate issue.

My vote for equeue public vs internal is to leave it as internal for now. Are we worried this will break code?

This comment has been minimized.

Copy link
@geky

geky Jul 22, 2019

Member

Let's not throw the baby out with the bathwater. There's some things the EventQueue does better than other event systems present in mbed-os tree :

To be clear I'm not suggesting we remove EventQueue. Or even remove any features yet.

Just that we rename equeue -> mbed_events (or similar), remove separate between equeue and EventQueue where it makes sense, and let the projects naturally diverge as their features grow/shrink to match their requirements.

How so ? Queuing event will always be relevant and if it can run on x86 it can help for integration and unit testing as well.

I was more referring specifically to the portability/OS-agnostic aspect of equeue.

You're right x86 support has been useful for testing, but IMO x86 testing should somehow be provided for all of Mbed OS. It's the only way to keep this shindig scalable.

FreeRTOS and Windows support have already been removed from the Mbed OS version because it makes less sense here. And there have been several cases of the terminology differences between equeue and the RTOS confusing users.

This comment has been minimized.

Copy link
@bulislaw

bulislaw Jul 23, 2019

Member

I agree :D So how about for this PR we:

  • Make queue public, as we need C API for bare metal builds
  • Merge the readmes so it all still makes sense
  • Do not rename anything public so that we don't break compatibility

Going forward:

  • @kjbracey-arm is looking at getting all the requirements from all the components so we can finaly have one queue to rule them all (mandatory https://xkcd.com/927/)
  • @donatieng @pan- are looking at 2 priorities for the queue and at static allocations

For mbed 6 we will try to force the unification and possible changes to the existing APIs.

I'll catch up with y'all at some point to discuss the plans further.

Does it sound like a plan?

This comment has been minimized.

Copy link
@loverdeg-ep

loverdeg-ep Jul 23, 2019

Contributor

👍
@bulislaw @kjbracey-arm @donatieng @pan
I am looking forward to the discussion of this topic further in the related public issue/issues

This comment has been minimized.

Copy link
@geky

geky Jul 23, 2019

Member

I'm a bit confused. My conclusion was that there shouldn't be a single queue. Is there plans to maintain a version outside of mbed-os?

@donatieng @pan- are looking at 2 priorities for the queue and at static allocations

FYI, I am also looking at static allocations, (though intentionally not priorities). A part that plan was to break API compatibility to introduce better error codes. Let me know if you want me to create a PR here as well though.

Copy link
Member

left a comment

We also should deal a bit better with the rtos directory, there's source which matches our current model, but there's also TARGET_CORTEX which probably should go under the sources. I don't have strong opinion what to do with the files from TARGET_CORTEX. And similiar for TARGET_CORTEX_M under platform/

@evedon evedon added needs: work and removed needs: review labels Jul 22, 2019
@evedon evedon force-pushed the feature-public-headers branch from d17fec5 to 24b3ca0 Jul 22, 2019
@evedon evedon added needs: CI and removed needs: work labels Jul 26, 2019
@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

@bulislaw We have address all comments, except for the following:

  • Moving rtos/TARGET_CORTEX & platform/TARGET_CORTEX_M to a differrent directory. @gpsimenos will do the refactoring in a different PR as I don't want to further increase the scope of this one.
  • Renaming equeue -> mbed_events (or similar): we agreed that this will be differed to another PR

We also need to check how much we are affecting the character limit as pointed out by Seppo.

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 26, 2019

Test run: SUCCESS

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

@gpsimenos

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

We also need to check how much we are affecting the character limit as pointed out by Seppo.

Compared to master, the command line length when building this branch is reduced by about 400 bytes mostly thanks to the flattening of the USB folder.

.includes file size (mbed-cloud-client-example, K64F, GCC_ARM, master) = 26281 bytes
.includes file size (mbed-cloud-client-example, K64F, GCC_ARM, feature-public-headers) = 25890 bytes

diff

@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@jamesbeyond ran the nightly tests before the last rework and all tests passed.
I will rebase this PR and we could have a final run for verification.

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Looks good, can we run the automated example to verify we don't break our own use cases? @jamesbeyond

Yeah, I'll trigger the test again on this

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 1, 2019

Test run: SUCCESS

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

@evedon evedon force-pushed the feature-public-headers branch from e68557e to 1031304 Aug 1, 2019
evedon added a commit that referenced this pull request Aug 1, 2019
* Modify Doxygen grouping of `drivers` Public/Internal APIs
* Correct classification of `mbed_events.h`
* Amend name of Doxygen group containing Device Key API
* Classify `CallChain.h` as public API and relocate file
* Remove Doxygen group from `equeue_platform.h` as it has no Doxygen compliant documentation
* Move USB target specific code back to `usb/device/targets`
@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Rebased and triggered CI.

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 1, 2019

Test run: SUCCESS

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

hugueskamba and others added 10 commits Jun 27, 2019
Also includes:
* rename `mbed_sleep_manager.c` to `mbed_power_mgmt.c` to match its
  header file
* create Doxygen groups for public and internal APIs
* use relative path to include header files where inconsistent
* update references to internal APIs throughout libraries
* update the copyright year for all modified files
Separate drivers, events, and rtos 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
* Remove source code from header files in order to remove include pre-processor directives
that included header files not directly used by said header files
* Explicitly include header files instead of implicit inclusions via third-party header files.

Release Notes

This will break user code that was using an internal API as the internal header files have been moved.
This will only break if the user was including the header file using a namespace (i.e #include "foo/bar.h" instead of #include "bar.h"
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.
Include the missing header file inclusion to find the added
MBED_DEPRECATED macro.
…11105)

* Change Doxygen groups structure, splitting first by Public/Internal

This commit also does the following:
* groups the documentation of related API
* moves `events/internal/equeue.h` to `events/equeue.h`
* merges `events/source/README.md` to `events/README.md`
* Fix rtos include path in NRFCordioHCIDriver
* Flatten USB driver directory structure
* Add missing include for us_ticker
* Add more missing includes for us_ticker
* Fix mbed_hal_fpga_ci_test_shield/uart test
* Fix bare-metal build
* Fix Watchdog UNITTEST
* Fix Mbed OS 2 build for Public/Internal headers relocating
…11114)


* Modify compilation API to provide a list of paths to exclude from the build.
* `_exclude_files_from_build` becomes a static method
* Replace ternary expression with simple  `if/else` statement
* Make unit test case for dirs exclusion independent of system files
* Modify Doxygen grouping of `drivers` Public/Internal APIs
* Correct classification of `mbed_events.h`
* Amend name of Doxygen group containing Device Key API
* Classify `CallChain.h` as public API and relocate file
* Remove Doxygen group from `equeue_platform.h` as it has no Doxygen compliant documentation
* Move USB target specific code back to `usb/device/targets`
@evedon evedon force-pushed the feature-public-headers branch from 1031304 to df85e8e Aug 2, 2019
@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Rebased again due to recent conflicting commits on master

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 2, 2019

Test run: SUCCESS

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

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Overnight tests and examples tests all passed on this PR

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

@evedon This passes a CI test and is approved by some of the reviewers. Is this ready to go in?

Maybe @ARMmbed/mbed-os-core team should also approve.

I would not wait too long for all pending reviews, as file renames tend to cause merge conflicts, if we wait too long.

@evedon

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

The code changes for this PR were made collaboratively by mbed-os-core team.
It is ready to go now. So please merge at your convenience.

@evedon evedon added ready for merge and removed needs: CI labels Aug 6, 2019
@SeppoTakalo SeppoTakalo merged commit 7d74165 into master Aug 7, 2019
28 checks passed
28 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push 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 RTOS ROM(+8 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 8525 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 8448B.
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
@adbridge adbridge deleted the feature-public-headers branch Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.