search

ros / roscpp_core

ros distribution sandbox
88 stars 116 forks source link

Fix for Duration::fromSec() which had rounding issues #93

Closed pbeeson closed 5 years ago

pbeeson commented 5 years ago

Fix Duration::fromSec() call to 1) handle the case where nanoseconds were rounding to another second and 2) handle negative durations properly.

See https://github.com/ros/roscpp_core/issues/92

dirk-thomas commented 5 years ago

Can you please add unit tests for the cases addressed by this patch.

pbeeson commented 5 years ago

I have a few free cycle tonight. By "unit test" did you mean a test to be run from a n automated builder? I don't see any other unit tests in the package, so I'm unsure the format you want that to take. Or do you just mean an example case? If the latter that's already here: https://github.com/ros/roscpp_core/issues/92

dirk-thomas commented 5 years ago

I have a few free cycle tonight.

Great, thank you.

By "unit test" did you mean a test to be run from a n automated builder? I don't see any other unit tests in the package, so I'm unsure the format you want that to take.

The rostime package does have two gtests already: https://github.com/ros/roscpp_core/tree/1110bb03024d5b698c481b41e5eeb7f4b5afbcbd/rostime/test

Adding another test function to one of the existing unit test was what I was suggesting.

pbeeson commented 5 years ago

Ok @dirk-thomas

pbeeson commented 5 years ago

Friendly bump.

pbeeson commented 5 years ago

I had a customer today that filed an issue with my larger code base because an incorrectly printed Duration was parsed from ros logs into their GUI and stopped their system because of a mismatch between the printed value and the real value.

This PR is a trivial change that makes the Duration() rounding code match the existing Time() rounding code, thus it now stores times properly. So, I'm respectfully asking for it to be considered for merge in the near future.

poripsa commented 5 years ago

There are several issues open on ROS relating to error: Duration is out of dual 32-bit range. I get this error inconsistently when I use MoveIt retime_trajectory. A fix for this issue would be great.

Some queries in ROS resources: https://answers.ros.org/questions/query:Duration%20is%20out%20of%20dual%2032-bit%20range/

And there are more on the github pages.

dirk-thomas commented 5 years ago

Thank you for the patch.