On 3 December 2014 at 05:02, Ola Liljedahl <[email protected]> wrote:
> >> >> diff --git a/example/timer/odp_timer_test.c
> >> >> b/example/timer/odp_timer_test.c
> >> >
> >> >
> >> > We need to rename this, having a "test" in example makes no sense at
> >> > all.
> >> > This patch series should rename this as an example - see below on the
> >> > implication on the large amount of code stored as code in the header
> >> > file
> >> > below, probably that needs to be in the resulting example.
> >> OK. But I wanted to limit the changes introduced by this patch series
> >> and then post additional patches that clean up the testing of the
> >> timer (e.g. odp_timer_ping has to go).
> >
> >
> > Can we capture the work that would still be needed in here 1.0
> > implementation deltas doc ?
> > I am worried you will skip the country :), but if you can do this soon,
> in
> > the interests of progress I'd be ok with the list in the doc
> Yes, I have added some text on odp_timer_test and odp_timer_ping.
>
>
> >
> >>
> >>
> >> > The "test" portion of this should become a cunit test
> >> OK. But perhaps not in this patch.
> >>
> >
> > Can we capture the work that would still be needed in here 1.0
> > implementation deltas doc ?
> Ditto.
>
>
> >> > Use C comments /* */
> >> I can't because this is inside another C comment and those are not
> >> recursive.
> >
> >
> > I had no problem with doxygen or the compiler that I noticed when I
> applied
> > the patch can we take a look ?
> Take a look at what?
>
I think that before we are done that in this case the example code needs to
be real example code, or the examples broken into snippets with regular
text surrounding them, so I wondered if that were possible to do the latter
in this case.
Going forward any place that needs a comment in a comment we should treat
it as broken. If an example snippet in a header file requires its own
comments I think it is too complex and must be converted to a real example.
Maybe that is too strict but having comments in comments just feels wrong.
>
> >
> >>
> >>
> >> > Can this example not be linked to the actual example so that the
> example
> >> > in
> >> > the doc is always assured to be as correct as the tests executing it
> in
> >> > CI ?
> >> These are not complete examples so cannot compile. I just want to show
> >> how the API is supposed to be used for a number of common use cases.
> >> Possibly a good example will be able to replace all of these header
> >> file/doxygen fragments. Would that be better?
> >
> >
> > Striving for the best case
> >
> > A good example which runs and that allows doxygen to pull chunks of code
> out
> > to to display would be best.
> > If the code can just compile that might be a better assurance that they
> > don't rot in the header file.
> >
> > But I would stop short of verbatim including it from another file with no
> > processing, it may as stay as it is and we can check back after 2.0 to
> see
> > if the examples still work :)
> Yes.
>
> >
> >
> >
> >>
> >>
> >>
> >> > After reviewing further there are a lot of examples in the header
> file I
> >> > am
> >> > sure they will rot unless we link them to the actual example code.
> >> They will rot only if the API is changed (except pure extensions). But
> >> code that actually compiles against the header file is of course
> >> better.
> >>
> >>
> >> >
> >> >>
> >> >> +#define SEC 1000000000ULL //1s expressed in nanoseconds
> >> >> +odp_timer_pool_t tcp_tpid =
> >> >> + odp_timer_pool_create("TCP",
> >> >> + buffer_pool,
> >> >> + 1000000,//resolution 1ms
> >> >> + 0,//min tmo
> >> >> + 7200 * SEC,//max tmo length 2hours
> >> >> + 40000,//num_timers
> >> >> + true,//shared
> >> >> + ODP_CLOCK_CPU
> >> >> + );
> >> >> +if (tcp_tpid == ODP_TIMER_POOL_INVALID)
> >> >> +{
> >> >> + //Failed to create timer pool => fatal error
> >> >> +}
> >> >> +
> >> >> +
> >> >> +//Setting up a new connection
> >> >> +//Allocate retransmission timeout (identical for supervision
> timeout)
> >> >> +//The user pointer points back to the connection context
> >> >> +conn->ret_tim = odp_timer_alloc(tcp_tpid, queue, conn);
> >> >> +//Check if all resources were successfully allocated
> >> >> +if (conn->ret_tim == ODP_TIMER_INVALID)
> >> >> +{
> >> >> + //Failed to allocate all resources for connection => tear
> down
> >> >> + //Tear down connection
> >> >> + ...
> >> >> + return false;
> >> >> +}
> >> >> +//All necessary resources successfully allocated
> >> >> +//Compute initial retransmission length in timer ticks
> >> >> +conn->ret_len = odp_timer_ns_to_tick(tcp_tpid, 3 * SEC);//Per
> RFC1122
> >> >> +//Allocate a timeout buffer
> >> >> +conn->tmo_buf = odp_buffer_alloc(buffer_pool);
> >> >> +if (conn->tmo_buf == ODP_BUFFER_INVALID)
> >> >> + ODP_ABORT("Failed to allocate timeout buffer\n");
> >> >> +//Arm the timer with our timeout
> >> >> +conn->expected = odp_timer_set_rel(conn->ret_tim, conn->ret_len,
> >> >> + &conn->tmo_buf);
> >> >> +//Check return value for tooearly or toolate expiration tick
> >> >
> >> >
> >> > too early and too late needs spaces - multiple instances
> >> I was mimicking the symbolic error codes.
> >> From odp_timer.h:
> >> #define ODP_TICK_TOOEARLY 0xFFFFFFFFFFFFFFFDULL
> >> #define ODP_TICK_TOOLATE 0xFFFFFFFFFFFFFFFEULL
> >>
> >
> > Ok, if there is a good reason, it just looked funny to my eye.
> It's a reason, it's subjective whether it's a good reason. But I our
> public identifiers to be constructed by three parts. First the "odp_"
> prefix. Second some kind of module or context identifier, e.g. "TICK"
> here. And third the specific part, either what the function does or
> the error code etc.
>
> ODP_TICK_TOOEARLY vs. ODP_TICK_TOO_EARLY
> ODP_TICK_TOOLATE vs. ODP_TICK_TOO_LATE
> ?
>
> >
> >>
> >> Do you require additional underscores here as well?
> >
> >
> > I think we should use underscores consistently, we really dont across the
> > code now and I am not sure what the rule is for when we don't use them,
> if
> > anyone knows I'll add it to the API guide lines doc.
> >
> > I would fix the #defines too.
> OK
>
I don't have anything else I would hold the patch for, we have capture the
need to fix the actual example and tests.
--
*Mike Holmes*
Linaro Sr Technical Manager
LNG - ODP
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp
