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
Merged

Feature public headers #11073

merged 10 commits into from
Aug 7, 2019

Conversation

evedon
Copy link
Contributor

@evedon evedon 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 a team July 19, 2019 09:00
@ciarmcom
Copy link
Member

@evedon, thank you for your changes.
@toyowata @maclobdell @bulislaw @ARMmbed/mbed-os-core @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-test @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@SeppoTakalo
Copy link
Contributor

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

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

evedon 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.

@0Grit
Copy link

0Grit 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
Copy link
Contributor Author

evedon 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
Copy link
Contributor Author

evedon 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
Copy link

mbed-ci 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 */
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@geky fyi

Copy link
Contributor

@geky geky Jul 19, 2019

Choose a reason for hiding this comment

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

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

Are we doing the same for littlefs?

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@bulislaw bulislaw Jul 23, 2019

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@geky geky Jul 23, 2019

Choose a reason for hiding this comment

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

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

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

evedon 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
Copy link

mbed-ci commented Jul 26, 2019

Test run: SUCCESS

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

@gpsimenos
Copy link
Contributor

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

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

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

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

@evedon
Copy link
Contributor Author

evedon 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
Copy link
Contributor

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
Copy link

mbed-ci commented Aug 1, 2019

Test run: SUCCESS

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

evedon pushed 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
Copy link
Contributor Author

evedon commented Aug 1, 2019

Rebased and triggered CI.

@mbed-ci
Copy link

mbed-ci 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 August 2, 2019 12:23
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"
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
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
Copy link
Contributor Author

evedon commented Aug 2, 2019

Rebased again due to recent conflicting commits on master

@mbed-ci
Copy link

mbed-ci commented Aug 2, 2019

Test run: SUCCESS

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

@jamesbeyond
Copy link
Contributor

Overnight tests and examples tests all passed on this PR

@SeppoTakalo
Copy link
Contributor

@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
Copy link
Contributor Author

evedon 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.

@SeppoTakalo SeppoTakalo merged commit 7d74165 into master Aug 7, 2019
@adbridge adbridge deleted the feature-public-headers branch August 20, 2019 11:00
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.