doc: reference: add discussion of terms that define API behavior

[pabigot]

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.

[zephyrbot]

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.

[dbkinder]

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

[dbkinder]

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

[dbkinder]

tweak: sleep in an interrupt context.

[pabigot]

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.

[mbolivar]

I agree with @pabigot

[dbkinder]

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.

[dbkinder]

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.

[dbkinder]

tweak: called from an interrupt context.

[dbkinder]

tweak: invoked from an interrupt context

[dbkinder]

tweak: an interrupt context

[dbkinder]

tweak: by an API

[pabigot]

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.

[dbkinder]

You're pretty consistent using bold on these names, but occasionally one slips by: **isr-ok**

[dbkinder]

**async** and APIs

[mbolivar]

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.

[nashif]

I would drop the "note" and make the text continous, also in all following terms.

[pabigot]

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 ?

[nashif]

Maybe use definition lists...

[dbkinder]

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

[pabigot]

All changes have been addressed except:

• remove note sections pending further discussion;
• globally refer to "in an interrupt context" instead of "in interrupt context", pending acceptability of this explanation. This terminology was developed based on recognizing three execution contexts: pre-kernel, thread, and interrupt.

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 spi_transceive_async which is an async sleep function.

[mbolivar]

OK for me. I am +1 on the content and ambivalent about the styling of the informative motivation for each definition.

[dbkinder]

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

[dbkinder]

drop the leading underscore: :ref:`ready state <thread_states>`

[andyross]

Some notes on minutiae. Broadly this all looks fine to me, though potentially a hassle to maintain.

[andyross]

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.

[pabigot]

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.

[andyross]

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()"?

[pabigot]

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.

[andyross]

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.

[pabigot]

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.

[andrewboie]

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

[andrewboie]

Also I really hate that we are duplicating information. This stuff should go in the documentation for the APIs themselves and not maintained here.

[pabigot]

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.

[andrewboie]

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.

[pabigot]

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.

[mbolivar]

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.

stale

[nordic-krch]

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

[pabigot]

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.