ros / roscpp_core

ros distribution sandbox
88 stars 116 forks source link

Helper for constructing time in mili/microseconds? #114

Closed peci1 closed 1 year ago

peci1 commented 4 years ago

I've just noticed that neither roscpp nor rospy provide any easy means of constructing time in miliseconds or microseconds. Why is that? Wouldn't it be nice to have constants like MSEC_IN_NSEC and USEC_IN_NSEC or something similar? Also, the magic number 1000000000 shows up a lot in the source code. This could also be converted to a constant like NSEC_IN_SEC.

With these constants, some time values could be constructed more predictably without the constant fear of floating-point numbers imprecision. If you e.g. specify the length of a pulse of a lidar, you're handling pretty small numbers (microseconds), and you want them to be precise.

Would a PR adding these constants/macros welcome, or is there a good (design?) reason why these values are not provided?

As well, I can imagine adding functions like TimeBase::fromMSec() etc., but that would be a bit larger API change (though ABI should remain untouched).

dirk-thomas commented 4 years ago

The API already offers fromNSec(). Converting your milliseconds / microseconds to nanoseconds sounds like a trivial operation. While a constant could help you can easily make the expression more readable like this:

fromNSec(my_milliseconds * 1000 * 1000)

So I personally don't think it should be added. If you still want to make a PR adding constants / new API for that I am happy to review them though.

peci1 commented 4 years ago

Just a point of view - ROS tries to be open to minorities and people with disabilities. I can imagine not everybody knows off the top of his/her head how many nanoseconds there are in a milisecond. Further, people with dyscalculia may be completely unable to count six or more zeroes... For me, this is another good reason to add some kind of helpers.

tfoote commented 4 years ago

One of the major challenges with adding these sort of intermediate representations to the API is that you can easily get people accidentally reducing their precision through lazy conversions that they don't realize round or floor.

Logic like

int myns1 = 100;
int myus = USEC_IN_NSEC * myns;
int myns2 = myus * NSEC_IN_USEC;

Leads to myns != myns2

std::chrono has protections for many of these things and you'll find that it's quite complicated to use but keeps developers from accidentally losing precision like my example.

Sticking to a single integer representation avoids these mistakes. Cleaning up internal magic numbers into consts certainly makes sense. And having display methods for different scales makes sense. But I would suggest that we not support/encourage storage of time in anything but the existing primitives we already have with full precision.

peci1 commented 4 years ago

I've sent a PR with a solution to this issue: #122.

dirk-thomas commented 4 years ago

@tfoote Maybe you want to review / comment on #122 since you commented above in the past.

peci1 commented 1 year ago

Partly solved by #123 .