Skip to content

Add doxygen group for HAL specification #5154

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

Closed
wants to merge 1 commit into from

Conversation

c1728p9
Copy link
Contributor

@c1728p9 c1728p9 commented Sep 20, 2017

Add the doxygen group hal_specification as a container group for all the HAL API specifications.

Add the doxygen group hal_specification as a container group for all
the API specifications.
@c1728p9
Copy link
Contributor Author

c1728p9 commented Sep 20, 2017

cc @bulislaw, @0xc0170, @sg-, @pan-

@theotherjimmy
Copy link
Contributor

@c1728p9 Does this need to be in it's own file? Alternatively, What/Who would #include "hal_spcefication.h"?

@c1728p9
Copy link
Contributor Author

c1728p9 commented Sep 20, 2017

@theotherjimmy, this is just information for doxygen. I could move it somewhere else if have any suggestions. It creates a group for the other HAL specifications like in #5008. It looks like this when rendered:
doxygen

@c1728p9
Copy link
Contributor Author

c1728p9 commented Sep 20, 2017

It is less than idea to have dedicated header files for HAL specification doxygen documentation here and in #5008. The only alternative I know of is adding this documentation into the API headers themselves, which is worse since it would add clutter and could confuse porters. Anyone have other ideas on how this information could be documented in doxygen in a less intrusive way?

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 26, 2017

@bulislaw @scartmell-arm happy with the hal specifications in tests/ ?

@pan-
Copy link
Member

pan- commented Sep 26, 2017

The only alternative I know of is adding this documentation into the API headers themselves, which is worse since it would add clutter and could confuse porters.

What would confuse porter if the API specification is in the API documentation ? Those are valuable information for partners, maintainer and user in development phases. Breaking down information locality and relying on doxygen to assemble everything back is inefficient at the moment for two reasons:

  • No mbed cli command to generate/regenerate mbed-os documentation
  • Doc on mbed website point to released versions of mbed OS. As a result their might be 3 month outdated compared to what's on master. For example, latest is for mbed-os 5.5.4 and due to our release cycle, none of the feature additions made for 5.6 are available in the website.

However I do acknowledge that test specification is a completely different beast which cannot be documented in API header files.

Does this need to be in it's own file?

Documentation block which do not belong in source file can go in .dox files as explained here

@bulislaw
Copy link
Member

I think the documentation should live either in the API headers doxygen (in hal/) or in the porting guide. Test should have usual description/explanation, but I don't think it should add any implementation vital info (they can reiterate something, but shouldn't introduce anything new).
I agree it would be useful to have hal_specification doxy group, but I'm not sure why you add it in test-headers directory?

@c1728p9
Copy link
Contributor Author

c1728p9 commented Sep 27, 2017

What would confuse porter if the API specification is in the API documentation ?

@pan- I was worried that the test prototypes would confuse porters since it would be a mix of test prototypes and prototypes for functions that needed to be implemented. After some dabbling I found doxygen could be enabled for source files (eliminating the need for a header file for prototypes) and created PR #5213 for this.

After moving the behavioral documentation into the HAL api header directly (as suggested by @pan-), there isn't much of a need for a dedicated specification group, so I'm closing this PR.

@c1728p9 c1728p9 closed this Sep 27, 2017
@sg- sg- removed the needs: review label Sep 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants