Public and internal APIs cleanup #10955

hugueskamba · 2019-07-03T13:59:14Z

Description

Separate drivers, events, and rtos internal APIs from public APIs

Move source files and internal headers to source subdirs
Amend test files and other modules include pre-processor directives that include the header files
moved to source subdirs
Add Doxygen comments for documenting internal and public drivers 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.

Pull request type

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

Reviewers

@evedon @gpsimenos

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"

* Moved source files and internal headers to internal/ * Added doxygen commands

* Move implementations from headers to sources in order to remove header file inclusions that do not need to be there. * Move one-line methods that do not call other modules methods to the header files. * Replace implicit inclusion of header files by explicit inclusions of the header files needed. * Explicitly include header files in other modules that relied on implicit inclusion of said header files. * Update the year in the Copyright notice of modules modified.

drivers/Ethernet, and anything Ethernet-related in the “HAL” region has never been used in Mbed OS 5. Ethernet is not provided like other core things such as serial. So drivers/Ethernet.h, drivers/Ethernet.cpp and hal/ethernet_api.h, and anything referring to them is obsolete. From 5.0-5.8, Ethernet drivers were written natively as lwIP or Nanostack drivers, and attached via their native APIs. No Mbed OS APIs were involved for driver attachment. Since 5.9, Ethernet drivers are provided by a class derived from `EMAC`, and is attached to lwIP or Nanostack. The above assume Ethernet drivers that send and receive Ethernet frames, in other words it assumes it uses an on-board IP stack. If the interface has an off-board IP stack, then a custom driver needs to derive from `EthInterface` or `WifiInterface` and implement the complete socket API for them. Applications can use various degrees of abstraction by designating particular classes: * NetworkInterface - any interface type * EthInterface/WifiInterface/MeshInterface/CellularInterface – specific interface type, no particular implementation * EthernetInterface – Ethernet using on-board stack, default EMAC * EthernetInterface<EMAC> - Ethernet using on-board stack and particular EMAC ESP8266Interface/BG96CellularModem – Particular network interface The top couple have static “get_default_instance()” methods to locate an appropriate interface for the current target. Or applications can nominate their driver specifically using the lower forms. “EthernetInterface” is a concrete implementation of the abstract “EthInterface”, it means “Use default onboard stack with default EMAC driver, defaults selected by get_default_instance”. That’s only half an abstraction – really portable apps should be saying “EthInterface::get_default_instance” (not caring about onboard or offboard), or “NetworkInterface::get_default_instance” (not caring about Wifi or Ethernet).

* Move source files and internal headers to `internal` dir * Amend test files and other modules include paths for internal headers * Add Doxygen comments for documenting internal and public drivers APIs * Remove incorrectly grouped Devicekey from the drivers Doxygen group and add to the newly created `Device Key` group.

* Move private stuff into internal dir * Fix relative paths * Add Doxygen * Update copyright

The macro `MBED_DEPRECATED` only takes one argument. Also move it to ensure the class is marked deprecated correctly.

…on (#8)

ciarmcom · 2019-07-03T15:00:51Z

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

evedon · 2019-07-04T08:49:57Z

I will review this PR

0xc0170 · 2019-07-04T11:47:48Z

Travis failures, please fix

0xc0170 · 2019-07-04T13:50:17Z

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"

We do not yet use internal namespace inside mbed namespace (mbed::internal), we move files to internal folder instead.
We still expose these headers via mbed.h as I read above, in the release note. What benefit does this have? If I do #include bar.h - and this is internal header file, the only way to find out is to read doxygen docs (line about the group currently only) or check location of the file.

The benefit for this internal files: No guarantees for changes inside internal/ (as for HAL it always is).

Is that it?

hugueskamba · 2019-07-05T10:52:03Z

Travis failures, please fix

Currently looking at it.

evedon · 2019-07-05T11:13:30Z

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"

We do not yet use internal namespace inside mbed namespace (mbed::internal), we move files to internal folder instead.
We still expose these headers via mbed.h as I read above, in the release note. What benefit does this have? If I do #include bar.h - and this is internal header file, the only way to find out is to read doxygen docs (line about the group currently only) or check location of the file.

The benefit for this internal files: No guarantees for changes inside internal/ (as for HAL it always is).

Is that it?

Internal headers should not be exposed in mbed.h, only public headers. Internal headers should only be included by mbed-os source files, probably from the same local directory.
A customer application should not be using internal headers. At the moment the build system will not prevent a customer application to include an internal header but it should when we introduce the new build system (include path should only include path to public headers directories).

kjbracey · 2019-07-05T11:32:08Z

What does it mean for a source file to be "internal" or not? It doesn't make sense to me for "internal/XXX.h" and "YYY.h" to have their source files next to each other. "drivers/internal/SerialWireOutput.cpp" mislead me to thinking you were making SerialWireOutput internal.
I see some public base classes are being wrongly marked here as private, eg SerialBase, TimerEvent. If it's a public base class, it's usually public API, unless indicated otherwise. Especially if virtual. Possibly they could be marked internal, but you'd need to make sure all inherited stuff was redocumented in the public one - people shouldn't have to read internal headers to understand the API.
Lots of inline functions are becoming non-inline. Probably okay in most cases, but some key ones like GPIO writes are deliberately engineered to be fully inlined all the way to the register write, and this breaks that. I'm not sure what any of those changes have to do with public versus private though. If you're trying to stop public header files including private ones, that's wrong - the public headers files will frequently have to include the private ones. The private ones will be taken off the primary path, but the public header files will still need to get at them.
This is going to cause a huge conflict with Sleep rework, RTOS API for bare metal, wait deprecations #10104. See the approach taken there for public vs internal - public APIs have both sources and headers and namespace outside "internal", and internal APIs have all of source, headers and namespace inside "internal".

kjbracey · 2019-07-05T11:44:01Z

There's possibly a slight overlap on meaning of "internal" here. The packaging here suggests a true hiding, whereby you could build the system to a library .o file, delete the "internal" directory and just ship the public includes.

That's a strong internal, which I don't think we need. I think our primary interest is in stopping people messing with classes/APIs that are subject to change.

In the mbed OS 3 build system, you may have had:

component-name/header.h (public header, namespace mbed)
component-name/internal/header-impl.h (implementation of main header, maybe some namespace mbed::impl)
source/header.c (source for header.h)
source/private.c (some internal code)
source/private.h (header for internal code)

That illustrates the two types of "internal". "Implementation detail that will need to be on the path" and "stuff that doesn't need to be shipped with the binary".

Nanostack uses that structure, as it was intended to be shipped in binary form without the source directory.

But I think we are only going for implementation detail here.

evedon · 2019-07-05T12:13:05Z

What does it mean for a source file to be "internal" or not? It doesn't make sense to me for "internal/XXX.h" and "YYY.h" to have their source files next to each other. "drivers/internal/SerialWireOutput.cpp" mislead me to thinking you were making SerialWireOutput internal.

All source files are moved to "internal". Originally the directory was named "source" and we went for "internal" because some internal header files are also included. I don't have a strong opinion on the actual name.

I see some public base classes are being wrongly marked here as private, eg SerialBase, TimerEvent. If it's a public base class, it's usually public API, unless indicated otherwise. Especially if virtual. Possibly they could be marked internal, but you'd need to make sure all inherited stuff was redocumented in the public one - people shouldn't have to read internal headers to understand the API.

I agree with that. We will correct it so that public base class are part of the public API.

Lots of inline functions are becoming non-inline. Probably okay in most cases, but some key ones like GPIO writes are deliberately engineered to be fully inlined all the way to the register write, and this breaks that. I'm not sure what any of those changes have to do with public versus private though. If you're trying to stop public header files including private ones, that's wrong - the public headers files will frequently have to include the private ones. The private ones will be taken off the primary path, but the public header files will still need to get at them.

We are not trying to stop public header files to include private ones.
The requirement was to remove source code from the headers, unless it's necessary. Thank you for pointing out which key functions should be inline and we will correct it.

evedon · 2019-07-05T12:25:10Z

There's possibly a slight overlap on meaning of "internal" here. The packaging here suggests a true hiding, whereby you could build the system to a library .o file, delete the "internal" directory and just ship the public includes.

That's a strong internal, which I don't think we need. I think our primary interest is in stopping people messing with classes/APIs that are subject to change.

You are right that our primary interest is in identifying public headers to stop people using classes/API that are subject to change.
If you main objection is with the name "internal" then I am happy to change that to "source". We have not split any header file between a public header part and implementation header part so we don't have any header-impl.h file.

hugueskamba · 2019-07-05T12:47:57Z

Travis failures, please fix

Currently looking at it.

@0xc0170
Please see 052c46d and bee7ae4

hugueskamba · 2019-07-05T12:49:01Z

I agree with that. We will correct it so that public base class are part of the public API.

@evedon @kjbracey-arm Working on that now.

Done. See 71137a4

This avoids the overlap in meaning with a strong `internal` dir which would allow shipping the public header without the content of the internal dir. The subirs `source` instead contain classes and APIs that users should not use and are subject to change.

hugueskamba · 2019-07-05T13:53:09Z

There's possibly a slight overlap on meaning of "internal" here. The packaging here suggests a true hiding, whereby you could build the system to a library .o file, delete the "internal" directory and just ship the public includes.

That's a strong internal, which I don't think we need. I think our primary interest is in stopping people messing with classes/APIs that are subject to change.

In the mbed OS 3 build system, you may have had:

component-name/header.h (public header, namespace mbed)

component-name/internal/header-impl.h (implementation of main header, maybe some namespace mbed::impl)

source/header.c (source for header.h)

source/private.c (some internal code)

source/private.h (header for internal code)

That illustrates the two types of "internal". "Implementation detail that will need to be on the path" and "stuff that doesn't need to be shipped with the binary".

Nanostack uses that structure, as it was intended to be shipped in binary form without the source directory.

But I think we are only going for implementation detail here.
@kjbracey-arm @evedon d04b0a3

drivers/source/DigitalIn.cpp

hugueskamba

The diff for commit 3eb37d7 is incorrect.
Here is what was done:

git mv source/DigitalIn.cpp AnalogIn.cpp`
git rm source/AnalogIn.cpp

kjbracey · 2019-07-11T08:32:25Z

The diff for commit 3eb37d7 is incorrect.
Here is what was done:
git mv source/DigitalIn.cpp AnalogIn.cpp`
git rm source/AnalogIn.cpp

All Git diffs, including rename/copy detection are done by textual analysis. Git doesn't ever record diffs, it stores "before" and "after" tree contents.

If it shows up wrong on GitHub, that's heuristics at GitHub's end's fault. Similarly for "git diff" - it's diff's fault.

When using Git locally you can try to massage diff results by adjusting various threshold parameters on the diff, but nothing you can do that I'm aware of for GitHub.

It doesn't mean there's necessarily anything wrong with the commit, as long as your "before" and "after" are fine.

You should probably delete the last few commits, they're just adding to the confusion.

0xc0170 · 2019-07-11T08:38:11Z

I have't reviewed the implementation detail yet. I might not manage to complete it before I am away. @kjbracey-arm 👍 for driving this.

The view here https://github.com/ARMmbed/mbed-os/tree/f878164a753415ca0d085bec6616071be2c420c6/drivers looks good (internal/source folders and providing public ones in the component root).

…anup

kjbracey

I'm fairly happy with how this looks. Folder structure seems good.

I'm still not a general fan of pulling stuff out of the headers. Some stuff really wants to be there for optimisation purposes (and increasingly will be required to be there for C++11 constexpr).

Here, it seems to me that most of the stuff that really gains from being inline now remains there, but then I'm not sure whether having the less clear-cut definitions moved off to the C files is a clarity improvement. Is "some of the stuff in .cpp" really better than "all of the stuff in .h"? Not sure it's hurting much, but not convinced it's helping either.

I'd be interested to see some build size comparisons (for both release and develop profiles) on the various toolchains. I'm sure the earlier versions would be a loss, but is this one now a gain? It might be if the compilers are a bit over-eager to inline with size optimisation on.

I still want to discuss this with @bulislaw - haven't had a chance yet.

Couple of specific change requests in comments.

This will conflict quite a bit with #10104 and #10895, which should be landing on master soon - you may need to rework. I'd want to see this squashed and tidied before landing on master too.

kjbracey · 2019-07-11T09:49:43Z

drivers/AnalogOut.h

-        write(percent);
-        return *this;
-    }
+    AnalogOut &operator= (float percent);


This is probably a lost optimisation - forcing an out-of-line function to return a usually-not-used *this. Prevents tailcall, will produce unpleasant code and increase stack usage.

As a rule, I'd say anything of the form

X &operator?(...) { some_method(); return *this; }

should remain inline, whether or not some_method is inline. That optimises the return nicely.

OK. We will do.

kjbracey · 2019-07-11T10:10:28Z

drivers/internal/TableCRC.cpp

@@ -16,11 +16,9 @@
 */

 #include <stdint.h>
-#include "drivers/TableCRC.h"
+#include "drivers/internal/TableCRC.h"


Yes, TableCRC.h is in the correct place, but logically, TableCRC.cpp should be inside source, as it's not required to be visible on the path. It would be part of the stuff that would be replaced by a binary.

It could be either source/TableCRC.cpp or source/internal/TableCRC.cpp. The latter would seem kind of logical, but I don't have a strong view, as long as it's in source.

I don't have a strong view on this either but I'd prefer all the source files under source, so let's keep source/TableCRC.cpp

hugueskamba · 2019-07-11T12:17:31Z

The diff for commit 3eb37d7 is incorrect.
Here is what was done:
git mv source/DigitalIn.cpp AnalogIn.cpp`
git rm source/AnalogIn.cpp
All Git diffs, including rename/copy detection are done by textual analysis. Git doesn't ever record diffs, it stores "before" and "after" tree contents.

If it shows up wrong on GitHub, that's heuristics at GitHub's end's fault. Similarly for "git diff" - it's diff's fault.

When using Git locally you can try to massage diff results by adjusting various threshold parameters on the diff, but nothing you can do that I'm aware of for GitHub.

It doesn't mean there's necessarily anything wrong with the commit, as long as your "before" and "after" are fine.

You should probably delete the last few commits, they're just adding to the confusion.

Thanks for the explanation @kjbracey-arm .
The commits were already gone when you commented. I am not sure how you could still see them.
We will squash everything before merging.

Removing the implementation does not result in a header removal from AnalogOut.h loses optimisation by forcing an the method to return a usually-not-used `*this`. It also prevents tailcall and will produce unpleasant code and increase stack usage.

evedon · 2019-07-11T12:29:13Z

This will conflict quite a bit with #10104 and #10895, which should be landing on master soon - you may need to rework. I'd want to see this squashed and tidied before landing on master too.

@kjbracey-arm This PR goes on a feature branch. We will add two PRs shortly for Platform/ and usb/ respectively. Final PR to master will be squashed.

evedon · 2019-07-11T12:54:30Z

Pending CI, I will merge this PR to the feature branch as two pending PRs are waiting for this one.

There is one more thing to check as per Kevins' comment:

Here, it seems to me that most of the stuff that really gains from being inline now remains there, but then I'm not sure whether having the less clear-cut definitions moved off to the C files is a clarity improvement. Is "some of the stuff in .cpp" really better than "all of the stuff in .h"? Not sure it's hurting much, but not convinced it's helping either.

If any rework is required based on the outcome of the discussion, we will do it in a different PR.

kjbracey

let's keep source/TableCRC.cpp

"keep"? It's currently in internal next to the header.

Anyway, approved, but will need another pass before master.

evedon · 2019-07-11T14:02:21Z

@hugueskamba We agreed to have all source files under source... Sorry to be a pain, can you move TableCRC.cpp and any similar others

The `internal` dirs should only contain header 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"

evedon and others added 8 commits July 3, 2019 11:59

RTOS public apis (#1)

f4b986d

* Moved source files and internal headers to internal/ * Added doxygen commands

Clean up events (#3)

82ae01a

* Move private stuff into internal dir * Fix relative paths * Add Doxygen * Update copyright

FiX build when Ethernet is included (#6)

3456f5f

The macro `MBED_DEPRECATED` only takes one argument. Also move it to ensure the class is marked deprecated correctly.

Fix the incorrect Doxygen group alias for RTOS public API documentati…

11003ea

…on (#8)

Fix RTOS internal API Doxygen documentation (#7)

fc48e43

ciarmcom requested review from evedon, gpsimenos and a team July 3, 2019 15:00

ciarmcom added the needs: review label Jul 3, 2019

0xc0170 requested a review from bulislaw July 4, 2019 11:43

Fix code style errors.

052c46d

Add Doxygen ingroup command as a recognised token in CI spellchecker

bee7ae4

hugueskamba added 2 commits July 5, 2019 14:24

Classify public base classes as public APIs

71137a4

hugueskamba added 6 commits July 10, 2019 11:47

Restore methods made inline in header files

893b930

Add Doxygen to Watchdog and relocate source file

2c3e4ec

Fix events test

7f1049b

Fix astyle issues

69b016a

Fix Serial unlock method definition

8fa81d3

Fix events tests by appending source location in internal dir

f5cbb0d

evedon reviewed Jul 10, 2019

View reviewed changes

drivers/source/DigitalIn.cpp Show resolved Hide resolved

hugueskamba commented Jul 11, 2019

View reviewed changes

Merge branch 'feature-public-headers' into feature-public-headers-cle…

48248d7

…anup

kjbracey reviewed Jul 11, 2019

View reviewed changes

evedon approved these changes Jul 11, 2019

View reviewed changes

kjbracey approved these changes Jul 11, 2019

View reviewed changes

hugueskamba added 2 commits July 11, 2019 15:17

Move source files from internal dirs to source dirs

488af02

The `internal` dirs should only contain header files.

Fixing events tests

da4baf3

evedon merged commit 50ffc35 into ARMmbed:feature-public-headers Jul 11, 2019

hugueskamba deleted the feature-public-headers-cleanup branch July 12, 2019 07:28

hugueskamba mentioned this pull request Jul 19, 2019

Feature public headers #11073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public and internal APIs cleanup #10955

Public and internal APIs cleanup #10955

hugueskamba commented Jul 3, 2019 •

edited

Loading

ciarmcom commented Jul 3, 2019

evedon commented Jul 4, 2019

0xc0170 commented Jul 4, 2019

0xc0170 commented Jul 4, 2019 •

edited

Loading

hugueskamba commented Jul 5, 2019

evedon commented Jul 5, 2019

kjbracey commented Jul 5, 2019

kjbracey commented Jul 5, 2019 •

edited

Loading

evedon commented Jul 5, 2019

evedon commented Jul 5, 2019

hugueskamba commented Jul 5, 2019

hugueskamba commented Jul 5, 2019 •

edited

Loading

hugueskamba commented Jul 5, 2019

hugueskamba left a comment •

edited

Loading

kjbracey commented Jul 11, 2019

0xc0170 commented Jul 11, 2019

kjbracey left a comment

kjbracey Jul 11, 2019

evedon Jul 11, 2019

kjbracey Jul 11, 2019

evedon Jul 11, 2019

hugueskamba commented Jul 11, 2019

evedon commented Jul 11, 2019

evedon commented Jul 11, 2019

kjbracey left a comment

evedon commented Jul 11, 2019

Public and internal APIs cleanup #10955

Public and internal APIs cleanup #10955

Conversation

hugueskamba commented Jul 3, 2019 • edited Loading

Description

Pull request type

Reviewers

Release Notes

ciarmcom commented Jul 3, 2019

evedon commented Jul 4, 2019

0xc0170 commented Jul 4, 2019

0xc0170 commented Jul 4, 2019 • edited Loading

hugueskamba commented Jul 5, 2019

evedon commented Jul 5, 2019

kjbracey commented Jul 5, 2019

kjbracey commented Jul 5, 2019 • edited Loading

evedon commented Jul 5, 2019

evedon commented Jul 5, 2019

hugueskamba commented Jul 5, 2019

hugueskamba commented Jul 5, 2019 • edited Loading

hugueskamba commented Jul 5, 2019

hugueskamba left a comment • edited Loading

Choose a reason for hiding this comment

kjbracey commented Jul 11, 2019

0xc0170 commented Jul 11, 2019

kjbracey left a comment

Choose a reason for hiding this comment

kjbracey Jul 11, 2019

Choose a reason for hiding this comment

evedon Jul 11, 2019

Choose a reason for hiding this comment

kjbracey Jul 11, 2019

Choose a reason for hiding this comment

evedon Jul 11, 2019

Choose a reason for hiding this comment

hugueskamba commented Jul 11, 2019

evedon commented Jul 11, 2019

evedon commented Jul 11, 2019

kjbracey left a comment

Choose a reason for hiding this comment

evedon commented Jul 11, 2019

hugueskamba commented Jul 3, 2019 •

edited

Loading

0xc0170 commented Jul 4, 2019 •

edited

Loading

kjbracey commented Jul 5, 2019 •

edited

Loading

hugueskamba commented Jul 5, 2019 •

edited

Loading

hugueskamba left a comment •

edited

Loading