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

Document where APIs can be called from using doxygen #21061

Closed
nashif opened this issue Nov 28, 2019 · 12 comments · Fixed by #32972
Closed

Document where APIs can be called from using doxygen #21061

nashif opened this issue Nov 28, 2019 · 12 comments · Fixed by #32972
Assignees
Labels
RFC Request For Comments: want input from the community

Comments

@nashif
Copy link
Member

nashif commented Nov 28, 2019

Introduction

Right now we have no standard way for documenting what APIs can be called from where and if they for example are allowed to be called from thread context or ISR context or if they can be used during initialization of the system for example.

Problem description

Documentation of the above is spread around the project either in the implementation itself or in the API docs or in the guides, but there is no 1 single place where we reliably and consistently document this.

Proposed change

Use a custom tag in doxygen to document the context from which an API can be called from

Detailed RFC

Add a new alias to doxygen (@allowed_from) and use it consistently to document from what contexts APIs are allowed to be called.

Proposed change (Detailed)

for example:

diff --git a/doc/zephyr.doxyfile.in b/doc/zephyr.doxyfile.in
index 147f94f629..087fc17892 100644
--- a/doc/zephyr.doxyfile.in
+++ b/doc/zephyr.doxyfile.in
@@ -224,6 +224,7 @@ ALIASES                = "rst=\verbatim embed:rst:leading-asterisk" \
                          "endrst=\endverbatim"

 ALIASES               += "req=\xrefitem req \"Requirement\" \"Requirements\" "
+ALIASES            += "allowed_from=\xrefitem context \"Allowed from\" \"Allowed from\" "

 # This tag can be used to specify a number of word-keyword mappings (TCL only).
 # A mapping has the form "name=value". For example adding "class=itcl::class"

will allow us to do this:

diff --git a/include/kernel.h b/include/kernel.h
index 23c0fd17f0..2ab916bcd4 100644
--- a/include/kernel.h
+++ b/include/kernel.h
@@ -1890,7 +1890,7 @@ __syscall void k_queue_init(struct k_queue *queue);
  * -EINTR and K_POLL_STATE_CANCELLED state (and per above, subsequent
  * k_queue_get() will return NULL).
  *
- * @note Can be called by ISRs.
+ * @allowed_from Threads, ISRs
  *
  * @param queue Address of the queue.
  *

which will result in:

image

instead of what we have now:

image

The above will also generate 1 single page with all the information:

image

Dependencies

Doxygen changes

Concerns and Unresolved Questions

How do we enforce this for example?

Alternatives

Alternative is to create some table or adhoc documentation that will always be out of sync. If we make this a requirement for any new API, then we can always rely on the autogenerated doxygen documentation.

@nashif nashif added the RFC Request For Comments: want input from the community label Nov 28, 2019
@alexanderwachter
Copy link
Member

We need the possibility to make the statement depending on parameters.
For example: Can be called by ISRs if timeout is K_NO_WAIT

@pabigot
Copy link
Collaborator

pabigot commented Nov 28, 2019

Are threads and ISRs the only calling contexts that matter? What about userspace vs kernel, and pre-kernel init levels? Does or will Zephyr support multiple privilege levels?

@alexanderwachter true, and hopefully that would be specified for all ISR-callable blocking (yielding) APIs.

I have no issues with something like this as part of a solution to #18970, but it doesn't eliminate the need for #20959.

@nashif
Copy link
Member Author

nashif commented Nov 28, 2019

Are threads and ISRs the only calling contexts that matter? What about userspace vs kernel, and pre-kernel init levels? Does or will Zephyr support multiple privilege levels?

This was just an example, initialization, userspace and supervisor modes are other contexts that can be added to this tag.

I have no issues with something like this as part of a solution to #18970, but it doesn't eliminate the need for #20959.

This RFC is not trying eliminate anything, having the terminology well defined is a good thing indeed. Need to see this in the final form though and submitted to the right location in the documentation so we can move forward with this.

@carlescufi
Copy link
Member

carlescufi commented May 12, 2020

API meeting:

Agreement on name: Function properties

/**
* @funcprops
* - @ref _funcprops::isr-ok: works whether it is being invoked from interrupt or thread context.
* - @ref _funcprops::supervisor: blah blah
*/

@carlescufi
Copy link
Member

API meeting 19th May:

  • doc: add alias for properties of functions #25411 needs more work and prototyping, deferred until 2.4.0
  • Need to decide whether we want a very shorthand way of expressing the properties (@funcprop sleep) or a more explanatory description in each function (which then requires copy-pasting definitions everywhere).
  • No postprocessing of header files whatsovere, whatever is in the .h file gets rendered directly by doxygen and breathe.

@pabigot
Copy link
Collaborator

pabigot commented Jul 28, 2020

This is dependent on #18970, or at least needs to be coordinated with it.

@utzig
Copy link
Member

utzig commented Sep 25, 2020

Similar ideas are used by ChibiOS/RT but with simpler (imo) Doxygen ALIASES:

https://github.com/ChibiOS/ChibiOS/blob/master/doc/rt/Doxyfile_chm#L241

Looking here you can see the different classes being used: https://github.com/ChibiOS/ChibiOS/blob/master/os/rt/src/chmtx.c

there's @api, @init, @sclass, etc. And it results in the following output:

http://chibiforge.org/doc/20.3/full_rm/group__mutexes.html#ga01630802b66eae988109e8a78147e988

@nashif
Copy link
Member Author

nashif commented Sep 25, 2020

Similar ideas are used by ChibiOS/RT but with simpler (imo) Doxygen ALIASES:

yes and I like those, however, how do you get those to show in sphinx?

@utzig
Copy link
Member

utzig commented Sep 25, 2020

Similar ideas are used by ChibiOS/RT but with simpler (imo) Doxygen ALIASES:

yes and I like those, however, how do you get those to show in sphinx?

ChibiOS/RT does not use Sphinx at all. I see your point and I will check out how to do it properly. The main reason for my suggestion was that I find it hard to enforce the correctness of the parameters passed to allowed-from, while with simple macros with no parameters we can check for typos and such things at build time.

@utzig
Copy link
Member

utzig commented Oct 23, 2020

@nashif #29483 should already render \xrefitem, so this issue can move forward. It looks like this: breathe-doc/breathe#589 (comment)

I also have a PR which is already functional to build all cross-reference pages, which with a bit more polishing should eventually land on Breathe: breathe-doc/breathe#593

@utzig
Copy link
Member

utzig commented Nov 6, 2020

@carlescufi Could you assign this to me or can I do it?

@carlescufi carlescufi assigned utzig and unassigned carlescufi Nov 10, 2020
@carlescufi
Copy link
Member

Assigning to @utzig since he's already posted a PR in breathe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request For Comments: want input from the community
Projects
None yet
5 participants