-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
doc: reference: add discussion of terms that define API behavior #21678
doc: reference: add discussion of terms that define API behavior #21678
Conversation
b403e16
to
fd0fa25
Compare
All checks are passing now. Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
fd0fa25
to
05493d3
Compare
7254561
to
28f87a1
Compare
doc/reference/index.rst
Outdated
allowed calling context (thread, ISR, pre-kernel), the effect of a call | ||
on the current thread state, and other behavioral characteristics. | ||
|
||
* :ref:`api_term_reschedule` if executing the function reaches a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll find this list would be more readable as a "dictionary list" where the term is bold and on a line by itself, and the definition is indented:
:ref:`api_term_reschedule`
if executing the function reaches a reschedule point
:ref:`api_term_sleep`
if executing the function can cause the invoking thread to sleep
...
doc/reference/index.rst
Outdated
Details on the behavioral impact of each attribute are in the following | ||
sections. Attributes for some common kernel API are below: | ||
|
||
+-------------------------------+------------+-------+---------+--------+-------+-----------+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this works fine as is, such tables can be hard to edit. If you think this will need updating, easier would be to use a list-table:
.. list-table::
:header-rows: 1
* - name
- reschedule
- sleep
- no-wait
- isr-ok
- async
- supervisor
* - :c:func:`k_busy_wait`
- no
- no*
- no*
- yes*
- no
- no
...
doc/reference/index.rst
Outdated
|
||
It is use of the no-wait feature that allows functions like | ||
:c:func:`k_sem_take` to be invoked from ISRs, since it is not | ||
permitted to sleep in interrupt context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tweak: sleep in an interrupt context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage seems to be mixed, but I think the technical usage should be "interrupt context" (processing an interrupt), contrasted with "thread context" (executing a thread). To say "an interrupt context" suggests a meaningful difference between interrupt contexts. At this time I don't see such a distinction being made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @pabigot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK... I've read these sentences a few times now and I can see your point (I think). It's not that there are different interrupt contexts, but rather it's an interrupt context and not a thread context. It sounds odd to my ear to read "sleep in interrupt context". A common grammar issue with non-English speakers is dropping prepositions and articles, "I sleep in car." vs. "I sleep in a car." I'd still prefer it as "sleep in an interrupt context", but I won't object if you feel strongly.
doc/reference/index.rst
Outdated
:c:func:`k_sem_take` to be invoked from ISRs, since it is not | ||
permitted to sleep in interrupt context. | ||
|
||
**NOTE** That a function has a no-wait path does not imply that taking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure you need a NOTE within a note. Also this sentence reads funny, maybe instead:
A function with a no-wait path does not imply that taking
that path guarantees the function is synchronous.
====== | ||
|
||
The isr-ok attribute is used on a function to indicate that it can be | ||
called from interrupt context. If necessary the function will use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tweak: called from an interrupt context.
doc/reference/index.rst
Outdated
.. note: | ||
|
||
This attribute is intended for **sleep** functions that may be | ||
indirectly invoked from interrupt context with arguments that could |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tweak: invoked from an interrupt context
doc/reference/index.rst
Outdated
no-wait path. | ||
|
||
Functions that are **isr-ok** may be always be safely invoked from | ||
interrupt context, and will return an error if they were unable to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tweak: an interrupt context
doc/reference/index.rst
Outdated
.. note:: | ||
|
||
This attribute is similar to **isr-ok** in function, but is intended | ||
for use by API that is expected to be called in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tweak: by an API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in context that suggests it's for a specific API. Alternatives would be "for use by APIs that are expected", or "by any API that is". I'm going with the latter.
doc/reference/index.rst
Outdated
Generally a function that is **pre-kernel-ok** checks | ||
:c:func:`k_is_pre_kernel` when determining whether it can fulfill its | ||
required behavior. In many cases it would also check | ||
:c:func:`k_is_in_isr` so it can be isr-ok as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're pretty consistent using bold on these names, but occasionally one slips by: **isr-ok**
doc/reference/index.rst
Outdated
|
||
.. note:: | ||
|
||
Be aware that async is orthogonal to context-switching. Some API may |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
**async**
and APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this.
I think it would be useful to get this list of definitions reviewed, agreed upon, and merged before we have a separate discussion in another PR around Doxygen and Sphinx tooling for marking up individual functions.
doc/reference/index.rst
Outdated
The reschedule attribute is used on a function that can reach a | ||
:ref:`reschedule point <scheduling_v2>` within its execution. | ||
|
||
.. note:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop the "note" and make the text continous, also in all following terms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm willing to do that to get this merged, but I'd like additional perspective before committing to it. The material before the note is intended to be normative and technically precise. The material in the note is intended to be commentary that is motivational and explanatory, saying why this is important, how to use it correctly, and how it affects use of the corresponding API.
If there's a different type of callout that should be used I'd be happy to switch to it. @dbkinder ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use definition lists...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a note directive causes the enclosed text to be particularly highlighted when rendered. Given your comment that this "note" is commentary, it would render nicer if you just added a subheading, something like this:
reschedule
==========
The reschedule attribute is used on a function that can reach a
:ref:`reschedule point <scheduling_v2>` within its execution.
Explanation
-----------
The significance of this attribute is that when a rescheduling
function is invoked by a thread it is possible for that thread to be
suspended as a consequence of a higher-priority thread being made
...
28f87a1
to
d88e87e
Compare
All changes have been addressed except:
Please review further. The specific changes may be obscured by a rebase that was necessary to leverage #21726 to enable generation of the documentation for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK for me. I am +1 on the content and ambivalent about the styling of the informative motivation for each definition.
doc/reference/index.rst
Outdated
The reschedule attribute is used on a function that can reach a | ||
:ref:`reschedule point <scheduling_v2>` within its execution. | ||
|
||
.. note:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a note directive causes the enclosed text to be particularly highlighted when rendered. Given your comment that this "note" is commentary, it would render nicer if you just added a subheading, something like this:
reschedule
==========
The reschedule attribute is used on a function that can reach a
:ref:`reschedule point <scheduling_v2>` within its execution.
Explanation
-----------
The significance of this attribute is that when a rescheduling
function is invoked by a thread it is possible for that thread to be
suspended as a consequence of a higher-priority thread being made
...
opportunity to change the identity of the current thread. These points | ||
are called **reschedule points**. Some potential reschedule points are: | ||
|
||
- transition of a thread to the :ref:`ready state <_thread_states>`, for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop the leading underscore: :ref:`ready state <thread_states>`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes on minutiae. Broadly this all looks fine to me, though potentially a hassle to maintain.
:ref:`api_term_reschedule` | ||
if executing the function reaches a reschedule point | ||
:ref:`api_term_sleep` | ||
if executing the function can cause the invoking thread to sleep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine from the perspective of API docs, though note that internally there is a distinction between sleep/suspend and "pend", which is a thread blocked on a wait_q. Might be worth clarifying that here for the benefit of anyone trying to match this up to scheduler code. Alternatively you could use the term "block" (which zephyr doesn't currently) as an umbrella term for both states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a bit odd, but I was told "can't sleep" is the criteria for deciding when something could be used in interrupt context. There's some background on choices and historical usage of different terms (including "block" which people tend to think means "synchronous") in https://github.com/pabigot/zephyr/blob/api/terms/doc/api18970.md. In the end "sleep" was the least inaccurate candidate.
even if it may return an error in that case | ||
:ref:`api_term_pre-kernel-ok` | ||
if the function can be safely called before the kernel has been fully | ||
initialized, even if it may return an error in that case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the actual use case here? This seems overspecific to my ears. The "PRE_KERNEL" init states are note themselves well defined. Can we not write this as "before the kernel launches application threads and enters main()"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case is #21090 where the API to request a clock must be usable both in interrupt kernel and before the kernel starts. I'm using pre-kernel-ok because the way to detect the case and avoid doing bad things is by checking k_is_pre_kernel()
which verifies the condition you identify.
===== | ||
|
||
A function is **async** (i.e. asynchronous) if it may return before the | ||
operation it initiates has completed. An asynchronous function will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absent a framework for reporting event state in a standard way, I don't see that this tag has much meaning. It's not clear in all cases exactly what all the side effects of a function are. I mean, is calling k_queue_put()
"async" or "sync"? Whether a thread wakes up before the function returns depends on external state and current process priorities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k_queue_put()
would be synchronous, because the item will have been put on the queue and any waiters transitioned to a new state before the function returns. onoff_request()
from #21090 is asynchronous, because the request may be satisfied and the operation result provided either before or after the function returns.
d88e87e
to
5b3ccab
Compare
This term needs to be defined to support documenting the effect of various API calls on scheduler selection of the running thread. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Add the flag so the related functions are documented and can be referenced from documentation. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
5b3ccab
to
7f76c5c
Compare
doc/reference/index.rst
Outdated
- ? | ||
- ? | ||
- no | ||
- yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong, k_thread_create can be called from user mode (subject to some restrictions)
Why did you think it can't? The prototype has __syscall
in it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I really hate that we are duplicating information. This stuff should go in the documentation for the APIs themselves and not maintained here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I picked k_thread_create
due to an off-by-one error associating kernel API functions with attributes from #21389. Replaced by k_thread_access_grant
.
We are absolutely not documenting the API in this location. This is only to get the terms defined so it can be done in the headers where it belongs. The table is meant as an example; I've revised the wording so that's more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced by k_thread_access_grant
Except k_thread_access_grant isn't supervisor only either.
We are absolutely not documenting the API in this location.
Yes you are. You are making a table with several kernel APIs and their properties, duplicating information that will be in the documentation of the APIs themselves. These will need to be kept in sync. NACK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a simple solution to both problems: no examples in the documentation. Done.
We can start using these attributes for new API once they're documented. People who have the expertise to specify the correct attributes for each function can back-fill existing API as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are absolutely not documenting the API in this location.
Yes you are. You are making a table with several kernel APIs and their properties, duplicating information that will be in the documentation of the APIs themselves. These will need to be kept in sync. NACK.
FWIW I found it very useful to have a table of examples next to the definitions themselves. I think the resulting document is less readable and harder to understand without it.
bdf84e1
to
47fa753
Compare
doc/reference/index.rst
Outdated
Functions with this attribute may be invoked from interrupt and | ||
pre-kernel contexts only when the parameter selects the no-wait path. | ||
|
||
Functions that are **no-wait** functions are implicitly **sleep**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that no-wait
attribute implies that kernel calls are used (the one that can take K_NO_WAIT)? If that's true then no-wait
functions cannot be called from pre-kernel (assuming kernel api shouldn't be used in pre-kernel).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed that line. Yes, you can use no-wait functions from pre-kernel as long as they're pre-kernel-ok (which is also the default)
Absent any behavior specification every function is no-wait. [edit: That was wrong given the high-level description of the attribute. I've revised the text to say that no-wait is only used when sleep is used.]
If the function is sleep then it can wait, so if there's a way to tell it to not sleep it needs no-wait added explicitly.
The idea was that from an explicit no-wait annotation you can infer that the function must be sleeps, because otherwise there would have been no need to say it was no-wait.
This concept of hierarchical interpretation of attributes with default values that depend on the setting of other tags has proven too confusing. We'll have to work around that either by adding all relevant tags explicitly in the header, or by carefully crafted tooling that does the inference correctly.
47fa753
to
18d79f2
Compare
Define a minimal set of attributes that can be used to indicate the allowed context for invoking specific Zephyr API and kernel functions, and the effect of invoking them on thread and other behavior. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
18d79f2
to
b6b1279
Compare
Define a minimal set of attributes that can be used to indicate the allowed context for invoking specific Zephyr API and kernel functions, and the effect of invoking them on thread and other behavior.
This provides the consistent terminology necessary to make progress on #18970 through #21061.
For evolution of this terminology and background information see #20959.