[PATCH v2] Comment added

[Sebastian Huber]

On 17/06/2019 12:45, Ravindra Kumar Meena wrote:
> 
> 
> On Mon, Jun 17, 2019 at 1:45 PM Sebastian Huber 
> <sebastian.huber at embedded-brains.de 
> <mailto:sebastian.huber at embedded-brains.de>> wrote:
> 
>     On 16/06/2019 20:13, Ravindra Meena wrote:
>      > ---
>      >   misc/CTF/record-ctf.ref | 55
>     +++++++++++++++++++++++++++++++++----------------
>      >   1 file changed, 37 insertions(+), 18 deletions(-)
>      >
>      > diff --git a/misc/CTF/record-ctf.ref b/misc/CTF/record-ctf.ref
>      > index a27757c..bdb9648 100644
>      > --- a/misc/CTF/record-ctf.ref
>      > +++ b/misc/CTF/record-ctf.ref
>     [...]
>      >   clock {
>      > -    name = ctf_clock;
>      > -    freq = 1000;
>      > -    offset_s = 1421703448;
>      > +    name = ctf_clock; /* name of montonic clock */
>      > +    freq = 1000; /* frequency, in HZ */
> 
>     Why 1000Hz?
> 
> The freq field is the initial frequency of the clock, in Hz. If the freq 
> field is not present, the frequency is assumed to be 1000000000 
> (providing clock increment of 1 ns). I set to 1000HZ just for getting 
> started.

This clock is for timestamps. For which timestamps is it?

> 
> 
>      > +    offset_s = 1421703448;
> 
>     What is this for an offset?
> 
> This offset is a difference between the beginning of clock and the 
> actual start of the clock. The event will be recorded just after the offset.

Sorry, I don't know what this means. What is the beginning of the clock 
and what is the actual start?

> 
> 
>      >   };
>      >
>      > +/*
>      > + *  A reference to the clock added within an integer type
>      > + */
>      > +
>      >   typealias integer {
>      >       size = 64;
>      >       map = clock.ctf_clock.value;
>      >   } := ctf_clock_int_t;
> 
>     Why does this start with "ctf_"?
> 
> ctf_clock is the name of clock struct I created just above this. It is a 
> reference to the clock name.

In case you can freely define names, then please don't include "ctf" in 
it. This suggests that this name is somehow part of the CTF specification.

> 
> 
>      >
>      > +/*
>      > + * Trace stream packet having header and context.
>      > + *
>      > + * @param event.header includes id(unique identifier of stream)
>     and timestamp.
>      > + * @param packet.context includes clock timestamp, cpu id, event
>     and event data.
>      > + */
>      > +
>      >   stream {
>      >       id = 0;
>      > +    event.header := struct {
>      > +        uint32_t id;
> 
>     What is this for an id?
> 
> It is a unique identifier for stream header. There will be many streams 
> so in order to identify the streams I have set the id here.

Could you please elaborate this a bit more. What is the purpose of this 
identifier? How generates the streams? Why can there be many streams? 
Who creates the identifiers?

> 
> 
>      > +        ctf_clock_int_t timestamp;
> 
>     Timestamp of what?
> 
>   It is the timestamp of the stream header. 

It is obvious that this is the timestamp of the stream header. Who 
generates this timestamp and when?

> It is different from event 
> header. The event heder can have a beginning  and ending timestamp.
> 
> 
>      > +    };
>      >       packet.context := struct {
>      >               ctf_clock_int_t timestamp;
>      >               uint32_t cpu;
>      >               uint32_t event;
>      >               uint64_t data;
>      >       };
>      > -    event.header := struct {
>      > -        uint32_t id;
>      > -        ctf_clock_int_t timestamp;
>      > -    };
>      >   };
>      >
>      >   event {
>      > -    id = 0;
>      > -    name = "ctf_event";
>      > -    stream_id = 0;
>      > +    id = 0; /* event id
>      > +    name = "ctf_event"; /* event name */
>      > +    stream_id = 0; /* signifies stream id which event is
>     supposed to concat with events */
>      >       fields := struct {
>      > -        uint32_t a;
>      > -        uint16_t b;
>      > -        string c;
>      > +        uint32_t a; /*event 1*/
>      > +        uint16_t b; /*event 2*/
>      > +        string c; /*event 3*/
> 
>     What are events 1, 2, 3?
> 
> I have added this event just for getting started. I thought it would be 
> better to get it reviewed this much TSDL code.

Adding some random data is just confusing. It should be the TSDL of a 
record item stream. As I said before, you have two options with respect 
to the timestamp clock. You can

1. use the timestamps based on CPU clock cycles

OR

2. use the timestamps after some post-processing based on CLOCK_MONOTONIC.

You have to explain what you use.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.