rock-core / base-types

C/C++ and Ruby definition of the base types in Rock
6 stars 40 forks source link

Timestamp for each types #164

Closed annaborn closed 1 year ago

annaborn commented 1 year ago

Couple of weeks ago we discussed to have a timestamp information not only in samples types but in each type introduced and used in the rock.

There was similar discussion a long time ago, but it was related only to command: https://www.dfki.de/pipermail/rock-dev/2013-November/003703.html

The idea is to introduce a header class, which will be inherited by base types and will contain timestamp. Also the header class may contain any other fundamental information.

@doudou @planthaber @moooeeeep @chhtz @skasperski @maltewi @malter @Priyanka328 @m0rsch1 @pierrewillenbrockdfki @haider8645 @0Nel @brean

doudou commented 1 year ago

The reason that timestamp information is not included in non-samples types is composability. If we put them everywhere, we'll end up with composed types that have 20 timestamps that are essentially the same.

I'm also not thrilled by the common "header type", since, in practice, timestamp is the only common field. That's adding a template for close to zero benefit.

planthaber commented 1 year ago

What do you think about auto-adding a timestamp to the types via orogen?

Documentation-wise it is a horror, as you need to know that there is a default-added timestamp field that is not part of the type in the header. Otherwise it would solve the composability.

We could misuse the #pragma preproceccor commands to add special orogen commands for a header, e.g.

#pragma orogen_once base::samples::Timestamp timestamp;

The normal cpp compiler just ignores the pragma, orogen could parse it and add the type only in the top level of a type

doudou commented 1 year ago

Auto-adding a timestamp makes no sense, as the timestamps needs to be filled in a way that makes sense (i.e. not always base::Time::now())

If in your systems want to make sure all output types have a timestamp, create yourself a static check tool that looks into all orogen projects and verifies it.

planthaber commented 1 year ago

Hmm, valid points.

What do you thing about adding a namespace "base::commands::timestamped" to provide timestamped versions of the command types through inheritance?

Similar to base::samples::Motion2d here: https://github.com/rock-core/base-types/blob/master/src/samples/CommandSamples.hpp

Still there are options, e.g. these types could be added in base/orogen/types or the template invocation and types could also go to a different typekit

doudou commented 1 year ago

I'm back to "I see little benefit of having a base class"

I do understand why you want to add timestamps to commands, and I think it's a valid endeavour. Unfortunately, the current state of "type migration" in Rock makes changing the existing types a real problem, so, yes I think that the specific namespace idea is valid.

But I think that having a base class just to add a timestamp is not worth the hassle. De-facto, this base class will be frozen as soon as any of the new command types are used, so why create it at all ?

annaborn commented 1 year ago

@doudou Hello Sylvain, I would like to close this issue. To improve the debugging process, we will extend rock-display (its cpp version for qt5) to display a receiving time for port data.