Added functions to convert between time_point and secNsec #150

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 [email protected]

Signed-off-by: ahcorde <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

Cast to system_clock::duration where necessary. Signed-off-by: Steve Peters <[email protected]>

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 <[email protected]>

…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 <[email protected]>

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 <[email protected]>

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

include/ignition/math/Helpers.hh

@@ -23,6 +23,8 @@
 #include <limits>
 #include <string>
 #include <iostream>
+#include <sstream>


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

done :) f9ef912

Signed-off-by: ahcorde <[email protected]>

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

Added functions to convert between time_point and secNsec

ef60586

Signed-off-by: ahcorde <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

make linters happy

8f13ba3

Signed-off-by: ahcorde <[email protected]>

make linters happy

a9cd19f

Signed-off-by: ahcorde <[email protected]>

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

Fixed test

b2b1451

Signed-off-by: ahcorde <[email protected]>

Added test for timePointToString

0efc2c1

Signed-off-by: ahcorde <[email protected]>

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

Fix macOS build

f11f678

Cast to system_clock::duration where necessary. Signed-off-by: Steve Peters <[email protected]>

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 <[email protected]>

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 <[email protected]>

Removed comment

76b3555

Signed-off-by: ahcorde <[email protected]>

ahcorde requested review from scpeters, mjcarroll and JShep1 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 <[email protected]>

ahcorde requested review from chapulina and JShep1 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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added functions to convert between time_point and secNsec #150

Added functions to convert between time_point and secNsec #150

ahcorde commented Aug 26, 2020

ahcorde commented Aug 26, 2020

codecov bot commented Aug 27, 2020 •

edited

Loading

JShep1 commented Aug 27, 2020 •

edited

Loading

JShep1 commented Aug 28, 2020

JShep1 left a comment •

edited

Loading

JShep1 Aug 28, 2020

ahcorde Aug 28, 2020

scpeters Aug 28, 2020

JShep1 Aug 28, 2020

scpeters commented Aug 28, 2020

scpeters Aug 28, 2020

scpeters Aug 28, 2020

scpeters Aug 29, 2020

ahcorde commented Aug 31, 2020

mjcarroll left a comment

JShep1 left a comment •

edited

Loading

mjcarroll Sep 1, 2020

ahcorde Sep 2, 2020

Added functions to convert between time_point and secNsec #150

Added functions to convert between time_point and secNsec #150

Conversation

ahcorde commented Aug 26, 2020

ahcorde commented Aug 26, 2020

codecov bot commented Aug 27, 2020 • edited Loading

Codecov Report

JShep1 commented Aug 27, 2020 • edited Loading

JShep1 commented Aug 28, 2020

JShep1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scpeters commented Aug 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahcorde commented Aug 31, 2020

mjcarroll left a comment

Choose a reason for hiding this comment

JShep1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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