Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creating More Accuracy in NS_TO_S #460

Open
wants to merge 6 commits into
base: rolling
Choose a base branch
from
+146 −6
Open

Creating More Accuracy in NS_TO_S #460

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
196e3bf
Creating more accuracy in NS_TO_S
CursedRock17 Apr 16, 2024
375e2f2
More precise testing
CursedRock17 Apr 17, 2024
431e7f6
Fixing uncrustify
CursedRock17 Apr 18, 2024
627a2ea
Adding custom functions
CursedRock17 Jul 12, 2024
006e414
Nitpiking
CursedRock17 Jul 12, 2024
4f51c49
Adding better definitions
CursedRock17 Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion include/rcutils/time.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ extern "C"
#define RCUTILS_US_TO_NS(microseconds) ((microseconds) * 1000LL)

/// Convenience macro to convert nanoseconds to seconds.
#define RCUTILS_NS_TO_S(nanoseconds) ((nanoseconds) / (1000LL * 1000LL * 1000LL))
#define RCUTILS_NS_TO_S(nanoseconds) \
((1e-9 * (double)((int32_t)(nanoseconds % 1000000000l))) + \
((double)((int32_t)((nanoseconds - ((int32_t)(nanoseconds % 1000000000l))) / 1000000000l))))
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment in ros2/rmw_fastrtps#755 on why I'm very nervous about making this change. Basically things that were expecting integer division before are now going to get FPU division. While it may be more accurate, it also changes the expectations.

This actually leads to another observation, which is that the type of nanoseconds is ill-defined because this is a macro.

All of that leads me to another thought. Maybe what we should do here is to make a new function called double rcutils_nanoseconds_to_seconds(double). And then we (slowly) convert uses of the macro over to that new function. That way we aren't making this change for things that don't expect it, and we can get the types right on the new function.

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it may be more accurate, it also changes the expectations.

Yeah as we found in this review, changing the nature of the macro induces errors in testing. To be fair, it's a beneficial change, but if it causes errors in testing there's a high chance it could lead to breakages on the users' end. Though I don't think going the route of new typed function is a better option as it just adds more to the file and alters the way code is written, when I would assume users just want to use a macro, design-wise. Unless most of the macros are rebranded to typed functions, I would feel that it's just an awkward change when it comes to DX.

This actually leads to another observation, which is that the type of nanoseconds is ill-defined because this is a macro.

Yeah while rcutils and dds implementations use int64_t it's not necessarily common knowledge for users.

I would like to create more accuracy, but since there's not a effective way to deprecate macros, I doubt it's worth breaking user behavior to do so, nor is it worth expanding on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to create more accuracy, but since there's not a effective way to deprecate macros, I doubt it's worth breaking user behavior to do so, nor is it worth expanding on.

I think that adding more accurate function calls (especially if they are constexpr) and migrating where it makes sense would work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that adding more accurate function calls (especially if they are constexpr) and migrating where it makes sense would work?

Yeah, that's what I'm thinking we should do.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(especially if they are constexpr) and migrating where it makes sense would work?

Seems fair enough to make more calls, but since rcutils is essentially C repository, did we need these split functions in a different repo?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fair enough to make more calls, but since rcutils is essentially C repository, did we need these split functions in a different repo?

Good point. It can't be constexpr here, but it can at least be a function, be const, and use types.

Alternatively, if we wanted to use C++ and constexpr, we could move it to rcpputils, but then there are certain places it can't be used (like rcutils, rcl, rcl_action, and rmw).


/// Convenience macro to convert nanoseconds to milliseconds.
#define RCUTILS_NS_TO_MS(nanoseconds) ((nanoseconds) / (1000LL * 1000LL))
/// Convenience macro to convert nanoseconds to microseconds.
Expand Down
14 changes: 6 additions & 8 deletions test/test_time.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,12 @@ TEST_F(TestTimeFixture, test_rcutils_time_conversion_macros) {
9007199254740992.); // value is truncated!

// nanoseconds to seconds
EXPECT_EQ(RCUTILS_NS_TO_S(1000000000ll), 1ll); // int64_t
EXPECT_EQ(RCUTILS_NS_TO_S(1000000042ll), 1ll); // int64_t (truncated)
EXPECT_EQ(RCUTILS_NS_TO_S(-1999999999ll), -1ll); // int64_t (truncated)
EXPECT_EQ(RCUTILS_NS_TO_S(200000000.), 0.2); // double
EXPECT_EQ(RCUTILS_NS_TO_S(1.0 + 1.0), 0.000000002); // sum of doubles
EXPECT_EQ(
RCUTILS_NS_TO_S(9007199254740992.),
9007199.254740992); // maximum precision double (53 bits)
EXPECT_EQ(RCUTILS_NS_TO_S(1000000000ll), 1.0); // int64_t
EXPECT_EQ(RCUTILS_NS_TO_S(1000000042ll), 1.000000042); // int64_t
EXPECT_EQ(RCUTILS_NS_TO_S(-1999999999ll), -1.999999999); // int64_t
EXPECT_EQ(RCUTILS_NS_TO_S(1 + 1), 0.000000002); // sum of two int64_ts
EXPECT_EQ(RCUTILS_NS_TO_S(9007199254740992ll),
9007199.254740992); // maximum precision int64_t

// nanoseconds to milliseconds
EXPECT_EQ(RCUTILS_NS_TO_MS(1000000ll), 1ll); // int64_t
Expand Down