On 20 May 2015 at 06:56, Jerin Jacob <[email protected]> wrote: > On Wed, May 20, 2015 at 12:25:12AM +0200, Ola Liljedahl wrote: >> On 19 May 2015 at 15:34, Jacob, Jerin <[email protected]> >> wrote: >> > Ola, >> > >> > Is there any specific reason for following check in timer validation test ? >> pa >> > >> > diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c >> > index 554b353..724026e 100644 >> > --- a/test/validation/odp_timer.c >> > +++ b/test/validation/odp_timer.c >> > @@ -260,7 +260,7 @@ static void handle_tmo(odp_event_t ev, bool stale, >> > uint64_t prev_tick) >> > >> > if (ttp != NULL) { >> > /* Internal error */ >> > - CU_ASSERT_FATAL(ttp->ev == ODP_EVENT_INVALID); >> > +----------> CU_ASSERT_FATAL(ttp->ev == ODP_EVENT_INVALID); >> > ttp->ev = ev; >> > } >> > } >> > >> > AFAIU, I should be CU_ASSERT_FATAL(ttp->ev != ODP_EVENT_INVALID) as >> > tt[i].ev = odp_timeout_to_event(odp_timeout_alloc(tbp)) specified while >> > preparing all timers. >> Yes the timers are still inactive and the timeout event is stored in >> the 'ev' member. >> >> handle_timeout() is called for received timeouts (timer has expired). >> In that case, the corresponding 'ev' member should not contain any >> timeout event. >> >> > >> > Am I missing something in the timer specification ? >> Or the timer specification is missing something? >> >> odp_timer_set_abs(tt[i].tim, tck, &tt[i].ev); (line 309) is supposed >> to grab the timeout event (on success) and clear the variable (write >> ODP_TIMEOUT_INVALID), that's why the timeout is passed by reference >> ("&tt[i].ev"). >> >> Possibly this is not specified clearly enough in timer.h: >> * @param[in,out] tmo_ev Reference to an event variable that points to >> * timeout event or NULL to reuse the existing timeout event. Any existing >> * timeout event that is replaced by a successful set operation will be >> * returned here. >> >> The new timeout event is read from *tmo_ev. The old timeout event (if >> timer was active) or ODP_TIMEOUT_INVALID (if timer was inactive) is >> stored in *tmo_ev. I hope this is at least clear in the reference >> implementation. > > We are on same page, except the last notes > IMO, linux generic timer implementation details leaked into creating the test > case. Well I don't agree and I hope I can convince you.
> > AFAIU, *tmo_ev should have the event that used for _arming_ the timer so > that application can do some look up after receiving event through queue or > something similar.. > What is the point of providing "ODP_TIMEOUT_INVALID" to application back, > What the > use of it for the application. It is possible to set an already active timer (which then is already associated with a timeout). If the user specifies a new timeout, the old timeout must be returned to the user (because all alloc and free of timeouts is the responsibility of the user). So any old timeout (for an already active timer) is return in "*tmo_ev". But it is possible that the timer has already expired (and the timeout been delivered) or wasn't active to start with. We want the application to be able to differ between these two scenarios and we achieve this by updating "*tmo_ev" accordingly. When the timer_set call return, if *tmo_ev != ODP_EVENT_INVALID, an timeout has been returned and the application needs to do something with it. If *tno_ev == ODP_EVENT_INVALID, no timeout was returned. I hope you can agree with this line of thinking. > > IMO, two way we can come to a conclusion for this issue. > 1) Remove CU_ASSERT_FATAL(ttp->ev == ODP_EVENT_INVALID) in handle_tmo > function in unit testcase > 2) Or some reason, If application wants ODP_TIMEOUT_INVALID(for inactive > case) in *tmo_ev > lets update the specification so that it will clear for odp implementer Yes the spec needs to be clearer. > > any thought from application perspective ? > > >> >> > >> > Thanks, >> > Jerin. >> > _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
