Fix deprecation warnings for upcoming ROS Galactic release by jacobperron · Pull Request #631 · cra-ros-pkg/robot_localization

jacobperron · 2021-03-17T22:25:43Z

Building against the latest version of rclcpp produces some deprecation warnings. This PR fixes warnings related to rclcpp::Duration construction (6b36efd) and rclcpp::Node::declare_parameter (7055b82).

Related tickets upstream, deprecating usage:

The second commit (7055b82) related to declare_parameter is not compatible with Foxy, so I recommend creating a new galactic-devel branch and we can retarget this PR.

If we are constructing from nanoseconds we must be explicit about the units. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

If a default value is not provided, then we must specify the parameter type. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

src/ros_filter.cpp

SteveMacenski · 2021-03-17T22:45:07Z

src/ros_filter.cpp

        first_measurement->topic_name_;
      // revertTo may invalidate first_measurement
-      if (!revertTo(first_measurement_time - rclcpp::Duration(1))) {
+      if (!revertTo(first_measurement_time - rclcpp::Duration(1ns))) {


@ayrton04 should this be 1ns, 1s, ...?

Thanks, I meant to ask the same question.

If I understand the equivalent ROS 1 code, it is also subtracting 1 nanosecond:

robot_localization/src/ros_filter.cpp

Line 611 in a2da33b

if (!revertTo(firstMeasurementTime - 1e-9)) // revertTo may invalidate firstMeasurement

So the PRs involved here are

https://github.com/cra-ros-pkg/robot_localization/pull/266/files

and

https://github.com/cra-ros-pkg/robot_localization/pull/279/files

The only reason we subtract any time at all is to ensure that we go to a measurement (presumably the first) that is before first_measurement_time. But we could also just change this to use >=:

robot_localization/src/ros_filter.cpp

Lines 3322 to 3323 in 3326e80

while (!filter_state_history_.empty() &&

filter_state_history_.back()->last_measurement_time_ > time)

If I had to guess (I didn't dig too far), the measurement times were likely stored as doubles originally, and the 1e-9 thing was just me being careful with floating point numbers. But ROS times have exact representations, so I think the 1e-9 could go, and we can change the revertTo reverse iteration to check for >= instead of just >.

However, that change could be outside of scope for this. It was meant to be 1 nanosecond, yes.

SteveMacenski · 2021-03-17T22:46:40Z

@jacobperron I opened a ros2 branch to target it to since those from_XYZ's aren't in foxy

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2021-03-17T23:06:15Z

Hold up, there's more warnings in other files I've overlooked.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2021-03-18T00:15:53Z

Hold up, there's more warnings in other files I've overlooked.

Okay, I think I got them all.

src/robot_localization_estimator.cpp

SteveMacenski · 2021-03-18T00:53:26Z

Otherwise LGTM, waiting for Tom's response on that one comment

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

src/ros_filter.cpp

When there is a NoParameterOverrideProvided exception, the parameter is not declared so we don't need to undeclare it. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

ayrton04 · 2021-03-22T10:21:07Z

@SteveMacenski, feel free to merge. I know you said LGTM, but your review still had comments requested, so I re-requested your review.

Try-catches were added in #631 due to a new rclcpp feature enforcing static parameter. The behavior was later patched for this particular use-case in ros2/rclcpp#1673, so now we can avoid having to try-catch. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Add missing message_filters dependency (#666) Headers from message_filters are included here: https://github.com/cra-ros-pkg/robot_localization/blob/67098c2341b5d1ccbcceb8eede60e79db74814a6/include/robot_localization/ros_robot_localization_listener.h\#L41-L42 Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Remove try-catch blocks around declare_parameter (#663) Try-catches were added in #631 due to a new rclcpp feature enforcing static parameter. The behavior was later patched for this particular use-case in ros2/rclcpp#1673, so now we can avoid having to try-catch. Signed-off-by: Jacob Perron <jacob@openrobotics.org> Co-authored-by: Jacob Perron <jacob@openrobotics.org>

* Remove try-catch blocks around declare_parameter (#663) Try-catches were added in #631 due to a new rclcpp feature enforcing static parameter. The behavior was later patched for this particular use-case in ros2/rclcpp#1673, so now we can avoid having to try-catch. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add missing message_filters dependency (#666) Headers from message_filters are included here: https://github.com/cra-ros-pkg/robot_localization/blob/67098c2341b5d1ccbcceb8eede60e79db74814a6/include/robot_localization/ros_robot_localization_listener.h\#L41-L42 Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Revert "Enable QoS overrides (#657)" This reverts commit 2816e92. Co-authored-by: Jacob Perron <jacob@openrobotics.org>

jacobperron added 2 commits March 17, 2021 15:12

Fix deprecation warnings when constructing rclcpp::Duration objects

6b36efd

If we are constructing from nanoseconds we must be explicit about the units. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Fix deprecated calls to declare_parameter

7055b82

If a default value is not provided, then we must specify the parameter type. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

SteveMacenski requested changes Mar 17, 2021

View reviewed changes

jacobperron changed the base branch from foxy-devel to ros2 March 17, 2021 22:53

Refactor

ca9e807

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Fix other deprecated uses of rclcpp::Duration

afa37fa

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

SteveMacenski reviewed Mar 18, 2021

View reviewed changes

src/robot_localization_estimator.cpp Outdated Show resolved Hide resolved

Minor refactor

774841b

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron commented Mar 19, 2021

View reviewed changes

src/ros_filter.cpp Outdated Show resolved Hide resolved

Catch exceptions when no parameter override is provided

3326e80

When there is a NoParameterOverrideProvided exception, the parameter is not declared so we don't need to undeclare it. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron mentioned this pull request Mar 19, 2021

Document alternative for declaring an optional parameter ros2/rclcpp#1588

Closed

ayrton04 approved these changes Mar 22, 2021

View reviewed changes

ayrton04 requested a review from SteveMacenski March 22, 2021 10:20

SteveMacenski approved these changes Mar 22, 2021

View reviewed changes

SteveMacenski merged commit 9fba3e8 into cra-ros-pkg:ros2 Mar 22, 2021

jacobperron deleted the fix_deprecation_warnings branch March 22, 2021 18:48

jacobperron mentioned this pull request May 20, 2021

Remove try-catch blocks around declare_parameter #663

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deprecation warnings for upcoming ROS Galactic release#631

Fix deprecation warnings for upcoming ROS Galactic release#631
SteveMacenski merged 6 commits intocra-ros-pkg:ros2from
jacobperron:fix_deprecation_warnings

jacobperron commented Mar 17, 2021

Uh oh!

Uh oh!

SteveMacenski Mar 17, 2021

Uh oh!

jacobperron Mar 17, 2021

Uh oh!

jacobperron Mar 18, 2021

Uh oh!

ayrton04 Mar 22, 2021

Uh oh!

ayrton04 Mar 22, 2021

Uh oh!

SteveMacenski commented Mar 17, 2021

Uh oh!

jacobperron commented Mar 17, 2021

Uh oh!

jacobperron commented Mar 18, 2021

Uh oh!

Uh oh!

SteveMacenski commented Mar 18, 2021

Uh oh!

Uh oh!

ayrton04 commented Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	while (!filter_state_history_.empty() &&
	filter_state_history_.back()->last_measurement_time_ > time)

Conversation

jacobperron commented Mar 17, 2021

Uh oh!

Uh oh!

SteveMacenski Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jacobperron Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jacobperron Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

ayrton04 Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

ayrton04 Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

SteveMacenski commented Mar 17, 2021

Uh oh!

jacobperron commented Mar 17, 2021

Uh oh!

jacobperron commented Mar 18, 2021

Uh oh!

Uh oh!

SteveMacenski commented Mar 18, 2021

Uh oh!

Uh oh!

ayrton04 commented Mar 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants