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

Jump to bottom

Addition and subtraction assignment operators #2424

Open
wants to merge 16 commits into
base: rolling
Choose a base branch
from
+48 −1
Open

Addition and subtraction assignment operators #2424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
7c57ad5
feat: add/minus for msg::Time and rclcpp::Duration
HuaTsai Jan 30, 2024
ba5b21a
fix: add needed headers
HuaTsai Jan 30, 2024
76afcf6
style: formatting
HuaTsai Jan 30, 2024
a1afbd4
test: operators with message stamp
HuaTsai Jan 30, 2024
f563e00
style: formatting
HuaTsai Jan 31, 2024
78ab451
refactor: function converting nanoseconds to seconds and nanoseconds
HuaTsai Feb 3, 2024
52fc2e6
fix: missing semicolon
HuaTsai Feb 3, 2024
7037cc8
fix: callee should be rcl_time_point_value_t
HuaTsai Feb 3, 2024
963d3e9
fix: add necessary header
HuaTsai Feb 3, 2024
9247045
refactor: move convert function to time.hpp/cpp
HuaTsai Feb 6, 2024
5993efc
Update rclcpp/test/rclcpp/test_duration.cpp
HuaTsai Feb 6, 2024
135c469
refactor: add consistent unsigned suffix 'u'
HuaTsai Feb 7, 2024
9f77873
refactor: return inline function
HuaTsai Feb 13, 2024
0c510bb
Merge branch 'ros2:rolling' into rolling
HuaTsai Feb 14, 2024
0952187
feat: addition/subtraction assignment operators
HuaTsai Feb 15, 2024
5df5661
fix: test case with negative operand
HuaTsai Feb 15, 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
8 changes: 8 additions & 0 deletions rclcpp/include/rclcpp/duration.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,18 @@ RCLCPP_PUBLIC
builtin_interfaces::msg::Time
operator+(const builtin_interfaces::msg::Time & lhs, const rclcpp::Duration & rhs);

RCLCPP_PUBLIC
builtin_interfaces::msg::Time &
operator+=(builtin_interfaces::msg::Time & lhs, const rclcpp::Duration & rhs);

RCLCPP_PUBLIC
builtin_interfaces::msg::Time
operator-(const builtin_interfaces::msg::Time & lhs, const rclcpp::Duration & rhs);

RCLCPP_PUBLIC
builtin_interfaces::msg::Time &
operator-=(builtin_interfaces::msg::Time & lhs, const rclcpp::Duration & rhs);

} // namespace rclcpp

#endif // RCLCPP__DURATION_HPP_
14 changes: 14 additions & 0 deletions rclcpp/src/rclcpp/duration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,13 @@ operator+(const builtin_interfaces::msg::Time & lhs, const rclcpp::Duration & rh
return convert_rcl_time_to_sec_nanos(rcl_time);
}

builtin_interfaces::msg::Time &
operator+=(builtin_interfaces::msg::Time & lhs, const rclcpp::Duration & rhs)
{
lhs = lhs + rhs;
return lhs;
}

builtin_interfaces::msg::Time
operator-(const builtin_interfaces::msg::Time & lhs, const rclcpp::Duration & rhs)
{
Expand All @@ -364,4 +371,11 @@ operator-(const builtin_interfaces::msg::Time & lhs, const rclcpp::Duration & rh
return convert_rcl_time_to_sec_nanos(rcl_time);
}

builtin_interfaces::msg::Time &
operator-=(builtin_interfaces::msg::Time & lhs, const rclcpp::Duration & rhs)
{
lhs = lhs - rhs;
return lhs;
}

} // namespace rclcpp
27 changes: 26 additions & 1 deletion rclcpp/test/rclcpp/test_duration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,11 @@ TEST_F(TestDuration, operators) {
}

TEST_F(TestDuration, operators_with_message_stamp) {
builtin_interfaces::msg::Time time_msg = rclcpp::Time(0, 100000000u); // 0.1s
rclcpp::Duration pos_duration(1, 100000000u); // 1.1s
rclcpp::Duration neg_duration(-2, 900000000u); // -1.1s

// Addition and subtraction operators
builtin_interfaces::msg::Time time_msg = rclcpp::Time(0, 100000000u); // 0.1s
builtin_interfaces::msg::Time res_addpos = time_msg + pos_duration;
EXPECT_EQ(res_addpos.sec, 1);
EXPECT_EQ(res_addpos.nanosec, 200000000u);
Expand All @@ -106,6 +107,30 @@ TEST_F(TestDuration, operators_with_message_stamp) {

EXPECT_THROW(neg_time_msg + max, std::runtime_error);
EXPECT_THROW(time_msg + max, std::overflow_error);

// Addition and subtraction assignment operators
time_msg = rclcpp::Time(0, 100000000u);
time_msg += pos_duration;
EXPECT_EQ(time_msg.sec, 1);
EXPECT_EQ(time_msg.nanosec, 200000000u);

time_msg -= pos_duration;
Copy link

Choose a reason for hiding this comment

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

Any special handling needed for overflow/underflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Ryanf55,
Sorry for forgetting to reply.
The overflow test is already in line 133 below.
For the underflow test, please check here: #2419 (comment).

Or let me paraphrase in another way:
I follow what happens when rclcpp::Duration + msg::Time.
It first becomes rclcpp::Duration + rclcpp::Time (implicit conversion).
During the conversion, negative seconds in msg::Time is not allowed.

rclcpp/rclcpp/src/rclcpp/time.cpp

Lines 52 to 54 in 99f0fc9

if (seconds < 0) {
throw std::runtime_error("cannot store a negative time point in rclcpp::Time");
}

IMO this is a small design flaw since it does not match Unix that uses int32 to store time.
But in practice, who cares about time before 1970, so it should remain fine.
After that, positive int32 seconds and uint32 nanoseconds will generate positive int64 full nanoseconds.
A positive int64 nanoseconds add another int64 duration nanoseconds always not underflow.

That's my current understanding and therefore I do (and can) not test underflow.

Copy link

@Ryanf55 Ryanf55 Feb 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is it written down anywhere that rclcpp::Time must use Unix time? I have used it in places to store time since boot, in the case that the embedded device does not have a RTC or network connection, and thus had no idea of any time except boot time. Certain unsafe uses of while loops, with timeouts and such can lead to weird bugs.

https://docs.ros2.org/beta3/api/rclcpp/classrclcpp_1_1Time.html

I agree, the likelyhood of underflow while using epoch time is low.

Copy link
Contributor Author

@HuaTsai HuaTsai Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is it written down anywhere that rclcpp::Time must use Unix time?

I don't find such description, maybe my statement earlier is too biased.

I have used it in places to store time since boot, in the case that the embedded device does not have a

RTC or network connection, and thus had no idea of any time except boot time.
Yes, this reminds me that the clock source of rclcpp::Time could also be timestamps in rosbag.
Both of them are not using current epoch time.

Copy link

Choose a reason for hiding this comment

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

As long as overflow/overflow is tested somewhere, seems like a good approach to me.

EXPECT_EQ(time_msg.sec, 0);
EXPECT_EQ(time_msg.nanosec, 100000000u);

time_msg += neg_duration;
EXPECT_EQ(time_msg.sec, -1);
EXPECT_EQ(time_msg.nanosec, 0u);

EXPECT_THROW(time_msg -= neg_duration, std::runtime_error); // not allow negative left operand

time_msg = rclcpp::Time(0, 100000000u);
time_msg -= neg_duration;
EXPECT_EQ(time_msg.sec, 1);
EXPECT_EQ(time_msg.nanosec, 200000000u);

EXPECT_THROW(neg_time_msg += max, std::runtime_error);
EXPECT_THROW(time_msg += max, std::overflow_error);
}

TEST_F(TestDuration, chrono_overloads) {
Expand Down