ping. Are we expecting a v2 of this patch? On Sun, Apr 17, 2016 at 2:58 PM, Bill Fischofer <bill.fischo...@linaro.org> wrote:
> General comment: This explanation could benefit considerably from the > inclusion of a state diagram like that used for the pktio documentation. > Here's the suggested timer_fsm.gv file: > > digraph timer_state_machine { > rankdir=LR; > size="10,12"; > node [fontsize=28]; > edge [fontsize=28]; > node [shape=doublecircle]; Timer_Unalloc > Timeout_Unalloc > Timeout_Delivered; > node [shape=circle]; > Timer_Unalloc -> Timer_Alloc [label="odp_timer_alloc()"]; > Timer_Alloc -> Timer_Unalloc [label="odp_timer_free()"]; > Timer_Alloc -> Timer_Set [label="odp_timer_set_abs()"]; > Timer_Alloc -> Timer_Set [label="odp_timer_set_rel()"]; > Timer_Set -> Timer_Alloc [label="odp_timer_cancel()"]; > Timer_Set -> Timeout_Alloc > [label="odp_timer_cancel()" constraint=false]; > Timer_Set -> Timer_Fired [label="(timer expires)"]; > Timer_Fired -> Timeout_Delivered [label="odp_schedule()"]; > Timeout_Unalloc -> Timeout_Alloc > [label="odp_timeout_alloc()" constraint=false]; > Timeout_Alloc -> Timeout_Unalloc > [label="odp_timeout_free()" constraint=false]; > Timeout_Alloc -> Timer_Set > [label="odp_timer_set_abs()" constraint=false]; > Timeout_Alloc -> Timer_Set > [label="odp_timer_set_rel()"]; > Timeout_Delivered -> Timer_Unalloc [label="odp_timer_free()"]; > Timeout_Delivered -> Timer_Set [label="odp_timer_set_abs()"]; > Timeout_Delivered -> Timer_Set [label="odp_timer_set_rel()"]; > Timeout_Delivered -> Timeout_Delivered > [label="odp_timeout_from_event()"]; > Timeout_Delivered -> Timeout_Delivered > [label="odp_timeout_timer()"]; > Timeout_Delivered -> Timeout_Unalloc > [label="odp_event_free() / odp_timeout_freee()" > constraint=false]; > } > > This should be added to doc/images and the .svg file referenced from there > and should help clarify the text in explaining the relationship between > timers and timeouts. > > Additional suggestions/corrections inline: > > On Thu, Apr 14, 2016 at 8:31 AM, Ivan Khoronzhuk < > ivan.khoronz...@linaro.org> wrote: > >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronz...@linaro.org> >> --- >> v2..v1: >> - just rebased with several not important corrections >> >> doc/users-guide/users-guide.adoc | 50 >> ++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 45 insertions(+), 5 deletions(-) >> >> diff --git a/doc/users-guide/users-guide.adoc >> b/doc/users-guide/users-guide.adoc >> index a2e5058..69b1930 100644 >> --- a/doc/users-guide/users-guide.adoc >> +++ b/doc/users-guide/users-guide.adoc >> @@ -335,11 +335,51 @@ The +odp_time_t+ opaque type represents local or >> global timestamps. >> >> === Timer >> Timers are how ODP applications measure and respond to the passage of >> time. >> -Timers are drawn from specialized pools called timer pools that have >> their >> -own abstract type (+odp_timer_pool_t+). Applications may have many timers >> -active at the same time and can set them to use either relative or >> absolute >> -time. When timers expire they create events of type +odp_timeout_t+, >> which >> -serve as notifications of timer expiration. >> +The timer API is supposed to be used when time synchronization with >> events is >> > > delete 'supposed to be' (extraneous) > > >> +needed and has different nature than time API has. Usually, timers >> require a >> > > rephrase (simplify): ...and is different than the time API. > Delete next sentence ("Usually, timers require...") as that's not relevant > to the API or its usage. > > >> +separate h/w module to be used for generating timeouts. Timers are drawn >> from >> +specialized pools called timer pools that have their own abstract type >> +(+odp_timer_pool_t+). Applications may have many timers active at the >> same >> +time and can set them to use either relative or absolute time. When >> timers >> +expire they create events of type +odp_timeout_t+, which serve as >> notifications >> +of timer expiration. The event is placed on a queue pointed while timer >> > > rephrase (clarify): "...placed on a queue specified at timer allocation." > > >> +allocation. >> + >> +Each timer pool can be set with it's own resolution in ns and number of >> > > grammar: its, not it's > > >> +supported timers. So, timer pool can be considered as a time source with >> > > grammar: So, a timer pool can be considered a time source... > > >> +it's own resolution and defined number of timers. All timers in timer >> pool >> +are handled with same time source with same resolution. If user needs >> two types >> +of timers with different requirements for resolution then better to >> create >> +two pools with it's own resolution, it can decrease load on hardware. >> > > grammar and rephrase: All timers in a timer pool have the same time source > and resolution. > If two types of timers with different resolutions are needed, then > applications should create two > timer pools, each with its own resolution. > > >> + >> +An expiration time for the timer is set in it's own ticks, so >> nanoseconds have >> +to be converted first with conversation function >> +odp_timer_ns_to_tick()+, to >> +convert it back to ns use +odp_timer_tick_to_ns()+. Both functions >> require >> +to pass a timer pool used, as it can be sourced with it's own time >> source that >> +can have specific resolution and thus different conversion ratio. >> > > grammar and rephrase for clarity: Each timer pool has its own > implementation-dependent > tick granularity. For portability applications must use the functions > +odp_timer_ns_to_tick()+ > and +odp_timer_tick_to_ns()+ to convert between ns and the pool's native > tick values. Note that > these functions take a timer pool as one of their arguments since each > pool may have its own > time source and conversion ratio. > > >> + >> +To set a timer to deliver a timeout event the two functions can be used: >> ++odp_timer_set_abs()+ and +odp_timer_set_rel()+. Both of them require an >> +event to be passed and a time interval in ticks of corresponding timer >> pool. >> > > grammar: "...a timeout event two functions can..." (delete the) > " Both of these...." > " ...in ticks of the corresponding timer pool" > > >> +The expiration time for a timer can be set based on current tick value >> for >> > > grammar: "...based on the current tick value..." > > >> +a timer pool taken with +odp_timer_current_tick()+, to set a timer >> > > grammar (new sentence): "To set a timer..." > > >> +(absolute time) with a user-provided timeout event, the >> +odp_timer_set_abs()+ >> > > grammar: "...event, +odp_timer_set_abs()+ can be used." (delete the) > > >> +can be used. An event is the event converted from timeout allocated from >> timeout >> +pool with +odp_timeout_alloc()+. The event is returned to timer queue >> when set >> +time interval has expired and can be also converted to timeout with >> ++odp_timeout_from_event()+, if it's needed. To free timeout the >> ++odp_timeout_free()+ can be used, if it's presented as event it can be >> freed >> +as event with +odp_event_free()+. >> > > This is fairly confusing and is where the state diagrams are really > helpful. I'm not going to suggest a wording here but see if doing this in > the context of the state diagram makes it simpler. > > >> + >> +In general, timer pool characterizes time source, and timer is >> characterized >> +with timer pool, user pointer and destination queue for delivering. The >> timeout >> +is characterized with timer, timer pool and time of expiration and is >> placed to >> +queue set while timer allocation. The timeout can be delivered only to >> the >> +destination queue of concrete timer. To get a timer generated a timeout >> ++odp_timeout_timer()+ can be used. >> > > This is also fairly muddy as it's trying to connect too many concepts in a > couple of sentences. I think it's better to describe timers and timeouts > and how they relate and then walk through the state diagram discussing how > applications create, set, cancel, and process timer events. > > >> + >> +When timer is freed it's returned to timer pool and is ready to be >> allocated >> +once again. A timer can be canceled with odp_timer_cancel. >> > > use +odp_timer_cancel()+ here for consistency. > > >> === Synchronizer >> Multiple threads operating in parallel typically require various >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > >
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp