doc: Add documentation for data structures, rework timing overview #19201

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.

Sign up for GitHub

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

Jump to bottom

Closed

andyross wants to merge 1 commit into zephyrproject-rtos:master from andyross:doc-lib-timing

Collaborator

andyross commented Sep 16, 2019

Another documentation PR bringing the timeout subsystem up to date with current code and adding a few pages detailing the various data structures we provide in lib/os.

As before, this reflects my preschool-level understanding of RST and is going to need significant technical cleanup before merge. And there are a few diagrams (that I also need to get help on) that need to be added in a few spots too.

andyross requested a review from dbkinder as a code owner

September 16, 2019 21:01

andyross added the DNM label

zephyrbot added the area: Documentation label

Collaborator

zephyrbot commented Sep 16, 2019 •

edited

Loading

Some checks failed. Please fix and resubmit.

checkpatch issues

-:406: ERROR:TRAILING_WHITESPACE: trailing whitespace
#406: FILE: doc/reference/kernel/timing/clocks.rst:231:
+  $

-:410: WARNING:TYPO_SPELLING: 'auxilliary' may be misspelled - perhaps 'auxiliary'?
#410: FILE: doc/reference/kernel/timing/clocks.rst:235:
+An auxilliary job of the timing subsystem is to provide tick counters

-:890: WARNING:TYPO_SPELLING: 'teh' may be misspelled - perhaps 'the'?
#890: FILE: doc/reference/misc/data_structures.rst:382:
+will copy bytes out of teh ring buffer in the order that they were

- total: 1 errors, 2 warnings, 927 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

NOTE: Whitespace errors detected.
      You may wish to use scripts/cleanpatch or scripts/cleanfile

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

Member

nashif commented Sep 16, 2019

@ulfalizer the checkpatch output should be treated as preformatted text. This used to work before.

Collaborator

ulfalizer commented Sep 16, 2019

@nashif
Yeah, noticed it. Will add a fix once I'm done with some other stuff.

Collaborator

ulfalizer commented Sep 16, 2019 •

edited

Loading

@nashif
Fix: zephyrproject-rtos/ci-tools#90

jukkar reviewed

View reviewed changes

doc/reference/kernel/timing/clocks.rst

    
              Other options for timeout initialization follow the unit conventions

              described above: ``K_TIMEOUT_US()``, ``K_TIMEOUT_TICKS()`` and

              ``K_TIMEOUT_CYC()`` specify timeout values that will expire after

Member

jukkar Sep 17, 2019

Not really related to this PR but just pondering why we have K_TIMEOUT_CYC() instead of K_TIMEOUT_CYCLE(). Saving two bytes here seems unnecessary as we have K_TIMEOUT_TICKS() which is identical length.

aescolar reviewed

View reviewed changes

doc/reference/kernel/timing/clocks.rst

    
              Kernel time is tracked in several units which are used for different

              purposes.

              Real time values, typically specified in milliseconds on microseconds,

Member

aescolar Sep 17, 2019

typo: on -> or?

dbkinder suggested changes

View reviewed changes

Contributor

dbkinder left a comment

Your reST-foo powers are increasing with practice. :)

doc/reference/kernel/timing/clocks.rst

    
              The hardware clock can be used to measure time with higher precision than

              that provided by kernel services based on the system clock.

              .. _clock_limitations:

Contributor

dbkinder Sep 17, 2019

There's a reference to this "Clock Limitations" section coming from https://docs.zephyrproject.org/latest/reference/kernel/timing/timers.html#timer-limitations so we need to provide a target in your new clocks.rst doc with this same name, or remove the reference from doc/reference/kernel/timing/timers.rst

doc/reference/kernel/timing/clocks.rst

    
              ==========

              Kernel time is tracked in several units which are used for different

              purposes.

Contributor

dbkinder Sep 17, 2019

add something like this to connect this opening sentence to the following paragraphs:

purposes: real time values, cycles, and ticks.

doc/reference/kernel/timing/clocks.rst

    
              hardware perfectly.

              The kernel presents a "cycle" count via the ``k_cycle_get_32()`` API.

              The intent is that this counter represents the fastest cycle counter

Contributor

dbkinder Sep 17, 2019

Delete The intent is that and start the sentence with This counter ...

doc/reference/kernel/timing/clocks.rst

    
              The kernel presents a "cycle" count via the ``k_cycle_get_32()`` API.

              The intent is that this counter represents the fastest cycle counter

              that the operating system is able to present to the user (for example,

              a CPU cycle counter) and that the read operation is very fast.  The

Contributor

dbkinder Sep 17, 2019

Change and that the read to and the read

doc/reference/kernel/timing/clocks.rst

    
              The intent is that this counter represents the fastest cycle counter

              that the operating system is able to present to the user (for example,

              a CPU cycle counter) and that the read operation is very fast.  The

              expectation is that very sensitive application code might use this in

Contributor

dbkinder Sep 17, 2019

Delete The expectation is that very and start the sentence with Sensitive application ...

doc/reference/kernel/timing/clocks.rst

    
              example: a ``struct wait_q``, or a ``k_tid_t`` thread struct).

              Note that all variant units passed via a ``k_timeout_t`` are converted

              to ticks once on insertion into the list.  There no

Contributor

dbkinder Sep 17, 2019

I think once can be deleted here.

Also, There are no

doc/reference/kernel/timing/clocks.rst

    
              expected to be embedded within subsystem-defined data structures (for

              example: a ``struct wait_q``, or a ``k_tid_t`` thread struct).

              Note that all variant units passed via a ``k_timeout_t`` are converted

Contributor

dbkinder Sep 17, 2019

Delete Note that and begin the sentence with All variants...

doc/reference/kernel/timing/clocks.rst

    
              guaranteed at the tick level no matter how many events exist or how

              long a timeout might be.

              Note that the list structure means that the CPU work involved in

Contributor

dbkinder Sep 17, 2019

Change this first sentence to:

Because of this timeout list structure, the CPU work to manage
large numbers of timeouts is quadratic to the number of active
timeouts.

doc/reference/kernel/timing/clocks.rst

    
              comparatively simple API.

              * The driver is expected to be able to "announce" new ticks to the

                kernel via the ``z_clock_nanounce()`` call, which passes an integer

Contributor

dbkinder Sep 17, 2019

probably z_clock_announce()

doc/reference/kernel/timing/clocks.rst

    
              counter driver can be trivially implemented also:

              * The driver can receive interrupts at a regular rate corresponding to

                the OS tick rate, calling z_clock_anounce() with an argument of one

Contributor

dbkinder Sep 17, 2019

put ticks around z_clock_announce (and fix misspelling)

aescolar reviewed

View reviewed changes

Member

aescolar left a comment •

edited

Loading

Thanks for this @andyross .
Just a few comments from me. In some of them I'm very picky, so you are welcome to ignore them if you do not agree.

doc/reference/kernel/timing/clocks.rst

    
              * The kernel ``k_delayed_work`` API provides a timeout parameter

                indicating when a work queue item will be added to the system queue.

              All these values are specified using a ``k_timeout_t`` value.  This is

Member

aescolar Sep 17, 2019

(In the PR description) Could you add a reference to 17155 marking this one as blocked by that one?

doc/reference/kernel/timing/clocks.rst

    
              ``K_TIMEOUT_CYC()`` specify timeout values that will expire after

              specified numbers of microseconds, ticks and cycles, respectively.

              Precision of ``k_timeout_t`` values is configurable, with the default

Member

aescolar Sep 17, 2019

Nit: When you said "precision" I expected you refered to the resolution (i.e. ticks) not to the bit-width / range.

doc/reference/kernel/timing/clocks.rst

    
                to the kernel which indicates how many ticks may elapse before the

                kernel must receive an announce call to trigger registered timeouts.

                It is legal to announce new ticks before that moment (though they

                must be correct) but delay after that will cause events to be

Member

aescolar Sep 18, 2019

Nit: This "but delay after that will cause events to be missed" would seem to make this paragraph a bit more confusing than it should, as it is really re-stating as a negative something which was stated before.
I would just remove it.

doc/reference/kernel/timing/clocks.rst

    
                and real world time.

              * The driver is expected to provide a ``z_clock_set_timeout()`` call

                to the kernel which indicates how many ticks may elapse before the

Member

aescolar Sep 18, 2019

Nit: This paragraph was a bit difficult to understand in a first read.
(it seems you provide more clearly a list of things this function should not do, but are less clear about what this function should do.)
Would it be a bit clearer to write this line as:
"to the kernel. The kernel will use it to indicate how many.."

Member

aescolar Sep 18, 2019

It may be nice to put a bit more of clear emphasis first in what is expected the function will do. And then, clearly demarked, after, what it should not do.

doc/reference/kernel/timing/clocks.rst

    
              * The next timeout value passed back to the driver via

                ``z_clock_set_timeout()`` is done identically for every CPU.  So by

                default, every CPU will see simultaneous timer interrupts for every

                event, even though by definition only one of them.  This is probably

Member

aescolar Sep 18, 2019

This sentence seems to be cut mid way (verb missing): ", even though by definition only one of them"

doc/reference/kernel/timing/clocks.rst Outdated

    
                a correct default for timing sensitive applications (because it

                minimizes the chance that an errant ISR or interrupt lock will delay

                a timeout), but may be a performance problem in some cases.  The

                current architecture expects that any such optimization.

Member

aescolar Sep 18, 2019

This sentence also seems to need rephrasing

carlescufi requested review from pizi-nordic and pabigot

September 18, 2019 12:18

pabigot requested changes

View reviewed changes

Collaborator

pabigot left a comment

While I think this is good, it is premature as it documents a system that has #17155 merged into it. Exactly what will be provided by that PR is still in discussion as there has been no response to review comments yet.

doc/reference/kernel/timing/clocks.rst

    
              For asynchronous timekeeping, the kernel defines a "ticks" concept.  A

              "tick" is the internal count in which the kernel does all its internal

              uptime and timeout bookeeping.  Interrupts are expected to be

Collaborator

pabigot Sep 18, 2019

This should be "Timeout interrupts..."

doc/reference/kernel/timing/clocks.rst

    
              "near" (round to nearest).  Finally the output precision can be

              specified as either 32 or 64 bits.

              For example: ``k_ms_to_ticks_ceil32()`` will convert a millisecond

Collaborator

pabigot Sep 18, 2019

This section only applies to #17155 (and the names and API provided are in dispute). I think once this gets through review it should be made part of that PR.

andyross force-pushed the doc-lib-timing branch from 2145d21 to 30651c7 Compare

September 18, 2019 16:55

pizi-nordic reviewed

View reviewed changes

doc/reference/kernel/timing/clocks.rst

    
              For asynchronous timekeeping, the kernel defines a "ticks" concept.  A

              "tick" is the internal count in which the kernel does all its internal

              uptime and timeout bookeeping.  Interrupts are expected to be

Collaborator

pizi-nordic Sep 23, 2019

Timer interrupts...

doc/reference/kernel/timing/clocks.rst

    
              All Zephyr ``k_timeout_t`` events specified using the API above are

              managed in a single, global queue of events.  Each event is stored in

              a double-linked list, with an attendant delta count in ticks from the

              previous event.  The action to take on an event is specified as a

Collaborator

pizi-nordic Sep 23, 2019

What is the time reference for first event in the list?

doc/reference/kernel/timing/clocks.rst

    
                kernel must receive an announce call to trigger registered timeouts.

                It is legal to announce new ticks before that moment (though they

                must be correct) but delay after that will cause events to be

                missed.  Note that the timeout value passed here is in a delta from

Collaborator

pizi-nordic Sep 23, 2019

Missed? I would say "delayed"...

doc/reference/kernel/timing/clocks.rst

    
                It is legal to announce new ticks before that moment (though they

                must be correct) but delay after that will cause events to be

                missed.  Note that the timeout value passed here is in a delta from

                current time, but that does not absolve the driver of the

Collaborator

pizi-nordic Sep 23, 2019

Current time? As far as I remember all system clock API uses last announcement as a reference...

doc/reference/kernel/timing/clocks.rst

    
              * The driver is expected to provide a ``z_clock_elapsed()`` call which

                provides a current indication of how many ticks have elapsed (as

                compared to a real world clock) since the last call to

                ``z_clock_announce()``, which the kernel needs to test newly

Collaborator

pizi-nordic Sep 23, 2019

Since last announced tick. The call to z_clock_announce() is always delayed (by interrupt/driver latency).

andyross force-pushed the doc-lib-timing branch from 30651c7 to cefe32b Compare

September 23, 2019 20:23


          doc: Add documentation for data structures, rework timing overview

6cfab82

Rework the timer documentation so it reflects current design, adding
significant detail on internals and the orhogonalized unit support in
the existing code.

Also add a new document describing the list, tree and ring buffer data
structures under lib/os.  We had doxygen references for this stuff,
but never had an overview document.

Signed-off-by: Andy Ross <[email protected]>

andyross force-pushed the doc-lib-timing branch from cefe32b to 6cfab82 Compare

September 23, 2019 20:49

Collaborator Author

andyross commented Sep 23, 2019

New version to address comments. Also includes some very nice diagrams from @laurenmurphyx64 in the data structures section. Cleaned up some phrasing in the pre-existing k_timer docs to better reflect current conventions (and to fix that broken link, which is how I got there).

andyross mentioned this pull request

New timeout API #17155

Closed

andyross mentioned this pull request

doc: Add documentation for data structures #20323

Closed

carlescufi mentioned this pull request

doc: kernel: Use K_NO_WAIT instead of 0 for one-shot timers #24270

Closed

carlescufi reviewed

View reviewed changes

doc/reference/kernel/timing/timers.rst

+                expires for the first time.  This is a ``k_timeout_t`` value that
+                may be initialized via different units.
+              * A :dfn:`period` specifying the time interval between all timer

Member

carlescufi Apr 10, 2020

You need to update the two code examples below:

k_timer_start(&my_status_timer, K_MSEC(200), 0);
and
k_timer_start(&my_sync_timer, K_MSEC(500), 0);

andyross mentioned this pull request

Kernel documentation additions & rework #24742

Merged

Collaborator Author

andyross commented Apr 27, 2020

Close this, updated work is in #24742

andyross closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jukkar jukkar left review comments

carlescufi carlescufi left review comments

pizi-nordic pizi-nordic left review comments

aescolar aescolar left review comments

pabigot pabigot requested changes

dbkinder dbkinder requested changes

Labels

area: Documentation DNM