ros / roscpp_core

ros distribution sandbox
89 stars 116 forks source link

Added helpers for constructing time in milli/microseconds #122

Closed peci1 closed 1 year ago

peci1 commented 4 years ago

Closes #114.

I've added constants that help converting nano-/micro-/milli-seconds and seconds between each other. The conversions are defined only for the cases where precision is not lost ("bigger" units to "smaller" units). The other direction has to be done via division, which should itself be a warning sign that precision might be lost (a concern of @tfoote).

I'm not sure about the names, whether they should be all upper-case, or if the chosen names like 'NSecInUSec' are okay. Also, I've put the constants directly in the ros namespace - that's mainly for convenience and to make typing of these constants shorter. They could be implemented as static members of DurationBase and TimeBase, but that seems weird, as that gives you access to the same constant via five different base classnames...

I'd also find a few more time-related things useful, and although they're not directly related to this PR, it might make sense to add them. Just let me know and I can add them. Or I can post them as separate PRs.

  1. Provide static constants Time::ZERO, Duration::ZERO etc. And for TimeBase-derived classes, maybe also Time::UNINITIALIZED (which would be the same as zero).
  2. Provide static factories for creating the time/duration objects from numeric values. Right now, there is Time::fromBoost() which goes in the same direction. Also, constructors can be seen as a kind of static factories from either a double (seconds), or a tuple of ints (seconds, nanoseconds). For all other cases, you first have to create an instance with a dummy value and then call the from* function on it. To provide a unified API, I'd suggest adding static methods like Time::fromSec(double), Time::fromMSec(uint64_t), Time::fromUSec(uint64_t) and Time::fromNSec(uint64_t). But these names are already "taken" by non-static functions, so they'd have to use different names. As an example, I'd like names Time::milliseconds(uint64_t), Time::millisec(uint64_t) or Time::msec(uint64_t). And I'm undecided whether to also add the grammatically incorrect Time::milisec() just for the case somebody doesn't know there are two Ls. But if this variant were missing, he'd get a compile error, and at least GCC would probably directly tell him the name of the correct function, so maybe this "convenience" function is not needed in the end.
  3. The least related, I'm always missing a toString() function that would return the exact string that's printed using the ostream<< operator. I know that there's a straightforward way via stringstream, but it's unnecessarily slow (see e.g. https://github.com/ros/geometry2/pull/470).
peci1 commented 4 years ago

Commit 7e6e7f5 that converts magic constants in rostime to use the added (non-magic) constants is, of course, optional. But I think it'd be good to actually make use of these constants at least in the rostime package, so that people can see how they can be used.

peci1 commented 4 years ago

I addressed most of the comments - renamed the constants, made their names uppercase, and added static factories Time::microseconds(uint64_t) and similar.

I really like what these factories allow. Compare:

ros::Duration(0, 1000000).sleep()  // old way
ros::Duration(0.001).sleep()  // old way, may lead to precision loss
ros::Duration::milliseconds(1).sleep()  // new way
peci1 commented 4 years ago

@tfoote I probably have a different view on the ROS time API. For me, ros::Time objects are the precise objects that hold time values and there I don't have to be afraid of accumulating or multiplying them. On the other hand, the double values got from toSec() are just imprecise values mostly useful for printing or constructing strings. I wouldn't dare accumulate or multiply these values.

Thus, from my point of view, adding more untypechecked time values with double type is not a problem. But if someone sees this differently, or (even worse) doesn't know or care about the type of the return values, he could run into precision issues...

If these added "untypes" are a problem for you and others, what about restricting this PR to just the constants and factories, and the toMSec() and similar methods would be left out? That would still allow people to construct time values conveniently (like ros::Duration::milliseconds(1)), which is the most important part for me. And if somebody wanted to know the value of the time object in milliseconds, he could always utilize conversion to nanoseconds and divide by one of the constants. The division itself should trigger a warning sign in the developer's mind that precision might get lost.

tfoote commented 4 years ago

But if someone sees this differently, or (even worse) doesn't know or care about the type of the return values, he could run into precision issues...

The division itself should trigger a warning sign in the developer's mind that precision might get lost.

These are things that developers should be aware of, but they will cause insidious bugs if they're not caught if the developer is inexperienced, in a hurry, or just not paying close attention. The constants are the most likely things to cause these sorts of errors. If we improve the std::chrono integration I think we should be able to leverage it for the conversions. And similarly the factories could just be leveraging std::chrono datatypes.

peci1 commented 4 years ago

Okay, and is someone currently working on the integration? And what are the chances such integration would get into Noetic/Melodic?

I could help, but I've never worked with std::chrono...