Added functions to convert between time_point and secNsec by ahcorde · Pull Request #150 · gazebosim/gz-math

ahcorde · 2020-08-26T09:21:26Z

This PR is related with this issue gazebosim/gz-common#61. The idea it's to deprecate the class common::Time in favor of std::chrono.

Related with

Added helpers fucntions to converte between

sec and nanosec to time_point
time_point to sec and nanosec
some tests too

Signed-off-by: ahcorde ahcorde@gmail.com

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2020-08-26T14:08:43Z

I added this function to convert a time_point to string. is this the right place? 489a79e

include/ignition/math/Helpers.hh

Signed-off-by: ahcorde <ahcorde@gmail.com>

codecov · 2020-08-27T11:14:27Z

Codecov Report

Merging #150 into ign-math6 will decrease coverage by 0.01%.
The diff coverage is 97.14%.

@@              Coverage Diff              @@
##           ign-math6     #150      +/-   ##
=============================================
- Coverage      99.25%   99.24%   -0.02%     
=============================================
  Files             59       59              
  Lines           5788     5823      +35     
=============================================
+ Hits            5745     5779      +34     
- Misses            43       44       +1

Impacted Files	Coverage Δ
include/ignition/math/Helpers.hh	`99.13% <97.14%> (-0.87%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ff0f1c...f9ef912. Read the comment docs.

Signed-off-by: ahcorde <ahcorde@gmail.com>

include/ignition/math/Helpers.hh

JShep1 · 2020-08-27T17:57:26Z

Do you think it would be appropriate to also place a std::string (in format dd hh:mm:ss.nnn) to std::chrono::time_point helper function in here? I'm currently working on one for a gazebo gui plugin and thought it might be helpful to add it to a more global location. If we want to do this, I can create a separate PR. @scpeters @ahcorde

JShep1 · 2020-08-28T05:31:56Z

Do you think it would be appropriate to also place a std::string (in format dd hh:mm:ss.nnn) to std::chrono::time_point helper function in here? I'm currently working on one for a gazebo gui plugin and thought it might be helpful to add it to a more global location. If we want to do this, I can create a separate PR. @scpeters @ahcorde

See #152

JShep1

Looks (and works) great! I'm currently using these helper functions in this plugin and it's quite handy. Some small nits with documentation and such but other than that I think it should be ready to go.

include/ignition/math/Helpers.hh

JShep1 · 2020-08-28T05:37:36Z

include/ignition/math/Helpers.hh

+        _nanosec);
+      std::chrono::system_clock::time_point result =
+        std::chrono::system_clock::from_time_t(0);
+      result = result + duration;


Small nit: could condense with result += duration, I'll leave it up to you though, same with all below additions like this made in the tests

This line is failing in OSX https://build.osrfoundation.org/job/ignition_math-ci-pr_any-homebrew-amd64/666/consoleFull#1943795879aa60f765-a60a-427d-900c-dc128ab22287 even if you use your suggestion. Any thoughts?

sorry for making suggestions that don't actually work; I thought it would be cleaner. I'll take a look with my mac later today

I looked into this a bit: Seems like OSX stores chrono to only a microsecond precision, whereas Ubuntu goes to nanosecond precision. It seems like all platforms have different precisions. See eddelbuettel/nanotime#74 and https://stackoverflow.com/questions/55120594/different-behaviour-of-system-clock-on-windows-and-linux

include/ignition/math/Helpers.hh

Signed-off-by: ahcorde <ahcorde@gmail.com>

…nitionrobotics/ign-math into ahcorde/time/helper_functions

Cast to system_clock::duration where necessary. Signed-off-by: Steve Peters <scpeters@openrobotics.org>

scpeters · 2020-08-28T23:42:38Z

I've fixed compilation on macOS in f11f678, though there is a test failure; I'll comment on the specific line

scpeters · 2020-08-28T23:45:27Z

src/Helpers_TEST.cc

+  parts = math::timePointToSecNsec(point);
+
+  EXPECT_EQ(parts.first, 60);
+  EXPECT_EQ(parts.second, 5789);


this expectation is failing on macOS, it gets 5000 instead of 5789. My guess is that the system_clock::duration cast in line 542 is changing the time base to microseconds, presumably because that's the resolution of the system clock?

I'm not sure what the best way to account for that is

oops, I should have read @JShep1's comment first: #150 (comment)

perhaps we should expect duration_cast<system_clock::duration>(5789)?

include/ignition/math/Helpers.hh

Signed-off-by: ahcorde <ahcorde@gmail.com>

…nitionrobotics/ign-math into ahcorde/time/helper_functions

ahcorde · 2020-08-31T13:44:57Z

ŧhank you @scpeters for the fix and @JShep1 for the pointer to the precision.

mjcarroll

Should we be using steady_clock instead?

include/ignition/math/Helpers.hh

Signed-off-by: ahcorde <ahcorde@gmail.com>

JShep1

LGTM other than some documentation updates

include/ignition/math/Helpers.hh

src/Helpers_TEST.cc

include/ignition/math/Helpers.hh

src/Helpers_TEST.cc

include/ignition/math/Helpers.hh

Signed-off-by: ahcorde <ahcorde@gmail.com>

mjcarroll · 2020-09-01T21:05:17Z

include/ignition/math/Helpers.hh

 #include <limits>
 #include <string>
 #include <iostream>
+#include <sstream>


While we're in here, can we alphabetize includes?

done :) f9ef912

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Added functions to convert between time_point and secNsec Signed-off-by: ahcorde <ahcorde@gmail.com> * Add function to convert a time_point in a string Signed-off-by: ahcorde <ahcorde@gmail.com> * Simplify math in timepoint convertions Signed-off-by: ahcorde <ahcorde@gmail.com> * make linters happy Signed-off-by: ahcorde <ahcorde@gmail.com> * make linters happy Signed-off-by: ahcorde <ahcorde@gmail.com> * make linters happy Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed test Signed-off-by: ahcorde <ahcorde@gmail.com> * Added test for timePointToString Signed-off-by: ahcorde <ahcorde@gmail.com> * Added documentation feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Fix macOS build Cast to system_clock::duration where necessary. Signed-off-by: Steve Peters <scpeters@openrobotics.org> * Fixed test and added a note with the precision on different OS Signed-off-by: ahcorde <ahcorde@gmail.com> * Change std::chrono::system_clock for std::chrono::steady_clock Signed-off-by: ahcorde <ahcorde@gmail.com> * Removed comment Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * alphabetize headers in helpers.hh Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: John Shepherd <john@openrobotics.org> Co-authored-by: Steve Peters <scpeters@openrobotics.org>

Added functions to convert between time_point and secNsec

ef60586

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde added enhancement New feature or request 🔮 dome Ignition Dome labels Aug 26, 2020

ahcorde requested a review from scpeters as a code owner August 26, 2020 09:21

ahcorde self-assigned this Aug 26, 2020

github-actions bot added 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint labels Aug 26, 2020

This was referenced Aug 26, 2020

Added utility functions to convert between msg::time and std::chrono time_point gazebosim/gz-msgs#81

Merged

Deprecate common::Time? gazebosim/gz-common#61

Closed

Add function to convert a time_point in a string

489a79e

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde mentioned this pull request Aug 26, 2020

Using std::chrono instead of common::Time gazebosim/gz-gui#116

Merged

scpeters reviewed Aug 26, 2020

View reviewed changes

include/ignition/math/Helpers.hh Outdated Show resolved Hide resolved

include/ignition/math/Helpers.hh Outdated Show resolved Hide resolved

Simplify math in timepoint convertions

b7ccbcd

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from scpeters August 26, 2020 16:13

ahcorde added 3 commits August 26, 2020 18:23

make linters happy

770b512

Signed-off-by: ahcorde <ahcorde@gmail.com>

make linters happy

8f13ba3

Signed-off-by: ahcorde <ahcorde@gmail.com>

make linters happy

a9cd19f

Signed-off-by: ahcorde <ahcorde@gmail.com>

JShep1 self-requested a review August 27, 2020 01:22

Fixed test

b2b1451

Signed-off-by: ahcorde <ahcorde@gmail.com>

Added test for timePointToString

0efc2c1

Signed-off-by: ahcorde <ahcorde@gmail.com>

scpeters reviewed Aug 27, 2020

View reviewed changes

include/ignition/math/Helpers.hh Outdated Show resolved Hide resolved

JShep1 mentioned this pull request Aug 28, 2020

Add string to time function #152

Merged

Merge branch 'ign-math6' into ahcorde/time/helper_functions

ec4845f

JShep1 suggested changes Aug 28, 2020

View reviewed changes

ahcorde added 2 commits August 28, 2020 11:34

Added documentation feedback

e5061a4

Signed-off-by: ahcorde <ahcorde@gmail.com>

Merge branch 'ahcorde/time/helper_functions' of https://github.com/ig…

fef7f9a

…nitionrobotics/ign-math into ahcorde/time/helper_functions

Fix macOS build

f11f678

Cast to system_clock::duration where necessary. Signed-off-by: Steve Peters <scpeters@openrobotics.org>

scpeters reviewed Aug 28, 2020

View reviewed changes

include/ignition/math/Helpers.hh Outdated Show resolved Hide resolved

ahcorde added 2 commits August 31, 2020 15:43

Fixed test and added a note with the precision on different OS

36eb876

Signed-off-by: ahcorde <ahcorde@gmail.com>

Merge branch 'ahcorde/time/helper_functions' of https://github.com/ig…

ab0e075

…nitionrobotics/ign-math into ahcorde/time/helper_functions

mjcarroll reviewed Aug 31, 2020

View reviewed changes

include/ignition/math/Helpers.hh Outdated Show resolved Hide resolved

ahcorde added 2 commits September 1, 2020 15:45

Change std::chrono::system_clock for std::chrono::steady_clock

8067150

Signed-off-by: ahcorde <ahcorde@gmail.com>

Removed comment

76b3555

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested review from JShep1, mjcarroll and scpeters September 1, 2020 16:38

JShep1 suggested changes Sep 1, 2020

View reviewed changes

include/ignition/math/Helpers.hh Outdated Show resolved Hide resolved

include/ignition/math/Helpers.hh Outdated Show resolved Hide resolved

include/ignition/math/Helpers.hh Outdated Show resolved Hide resolved

JShep1 reviewed Sep 1, 2020

View reviewed changes

src/Helpers_TEST.cc Outdated Show resolved Hide resolved

chapulina reviewed Sep 2, 2020

View reviewed changes

include/ignition/math/Helpers.hh Outdated Show resolved Hide resolved

include/ignition/math/Helpers.hh Outdated Show resolved Hide resolved

src/Helpers_TEST.cc Outdated Show resolved Hide resolved

JShep1 reviewed Sep 2, 2020

View reviewed changes

include/ignition/math/Helpers.hh Outdated Show resolved Hide resolved

Added feedback

87d71f8

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested review from JShep1 and chapulina September 2, 2020 08:31

ahcorde mentioned this pull request Sep 2, 2020

Replace common::Time with std::chrono gazebosim/gz-rendering#135

Merged

mjcarroll approved these changes Sep 2, 2020

View reviewed changes

alphabetize headers in helpers.hh

f9ef912

Signed-off-by: ahcorde <ahcorde@gmail.com>

JShep1 approved these changes Sep 2, 2020

View reviewed changes

ahcorde merged commit d5dc70f into ign-math6 Sep 3, 2020

ahcorde deleted the ahcorde/time/helper_functions branch September 3, 2020 07:47

ahcorde mentioned this pull request Sep 3, 2020

[Forwardport master] Added functions to convert between time_point and secNsec (#150) #154

Closed

Conversation

ahcorde commented Aug 26, 2020

Uh oh!

ahcorde commented Aug 26, 2020

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

JShep1 commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JShep1 commented Aug 28, 2020

Uh oh!

JShep1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scpeters commented Aug 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahcorde commented Aug 31, 2020

Uh oh!

mjcarroll left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JShep1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Aug 27, 2020 •

edited

Loading

JShep1 commented Aug 27, 2020 •

edited

Loading

JShep1 left a comment •

edited

Loading

JShep1 left a comment •

edited

Loading