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

Add TIME_MAX and DURATION_MAX functions #538

Merged
merged 10 commits into from
Aug 27, 2018
Merged

Add TIME_MAX and DURATION_MAX functions #538

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e00cad1
Add TIME_MAX and DURATION_MAX functions
Aug 20, 2018
50f0412
Merge branch 'master' into master
sagniknitr Aug 21, 2018
5bebeee
Fix Linting Errors
Aug 22, 2018
64c83f7
change funtion name as per coding style
sagniknitr Aug 26, 2018
7231fad
change function name as per coding style
sagniknitr Aug 26, 2018
37d793b
Update duration.cpp
sagniknitr Aug 26, 2018
1e03154
Update time.cpp
sagniknitr Aug 26, 2018
ee4bdd9
Update test_duration.cpp
sagniknitr Aug 26, 2018
ddc03e7
Update time.hpp
sagniknitr Aug 26, 2018
8fb0124
remove extra empty line
dirk-thomas Aug 27, 2018
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
4 changes: 4 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 @@ -89,6 +89,10 @@ class Duration
Duration
operator-(const rclcpp::Duration & rhs) const;

RCLCPP_PUBLIC
static Duration
DURATION_MAX();
Copy link
Member

Choose a reason for hiding this comment

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

Please rename the method to use lowercase (as per code style). Also since it is defined in the class Duration I don't think it needs to be part of the name. So I would suggest just calling it max() which will be used as Duration::max().

Same for time below.


RCLCPP_PUBLIC
Duration
operator*(double scale) const;
Expand Down
5 changes: 5 additions & 0 deletions rclcpp/include/rclcpp/time.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ class Time
rcl_time_point_value_t
nanoseconds() const;

RCLCPP_PUBLIC
static Time
TIME_MAX();


/// \return the seconds since epoch as a floating point number.
/// \warning Depending on sizeof(double) there could be significant precision loss.
/// When an exact time is required use nanoseconds() instead.
Expand Down
7 changes: 7 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 @@ -214,6 +214,13 @@ Duration::nanoseconds() const
return rcl_duration_.nanoseconds;
}

Duration
Duration::DURATION_MAX()
{
Duration duration_max(std::numeric_limits<int32_t>::max(), 999999999);
Copy link
Contributor

Choose a reason for hiding this comment

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

@tfoote This PR and #524 say a duration is limited by the max value that can be given to the constructor accepting seconds and nanoseconds, but there is another constructor taking nanoseconds directly, and that seems to accept a number of nanoseconds as large as the 64bit rcutils type. Should this instead be std::numeric_limits<rcl_duration_value_t>::max()? Alternatively, should the nanoseconds constructor restrict the maximum size to int32_t seconds + 999999999 nanoseconds?

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 the nanoseconds constructor should limit it to this representation. Otherwise we'll overflow the message datastructure.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it would be easier to read as a one line function. No need to name the temporary variable, it could just be returned.

return duration_max;
}

double
Duration::seconds() const
{
Expand Down
7 changes: 7 additions & 0 deletions rclcpp/src/rclcpp/time.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,5 +251,12 @@ operator+(const rclcpp::Duration & lhs, const rclcpp::Time & rhs)
return Time(lhs.nanoseconds() + rhs.nanoseconds(), rhs.get_clock_type());
}

Time
TIME_MAX()
{
Time time_max(std::numeric_limits<int32_t>::max(), 999999999);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it would be easier to read as a one line function. No need to name the temporary variable, it could just be returned.

return time_max;
}


} // namespace rclcpp
8 changes: 8 additions & 0 deletions rclcpp/test/test_duration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,3 +120,11 @@ TEST(TestDuration, negative_duration) {
EXPECT_EQ(expected_value, assignable_duration.nanoseconds());
}
}

TEST(TestDuration, maximum_duration) {
rclcpp::Duration max_duration = rclcpp::Duration::DURATION_MAX();
rclcpp::Duration max(std::numeric_limits<int32_t>::max(), 999999999);

EXPECT_EQ(max_duration, max);
}