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.

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

Fix the keep_last warning when using system defaults. #2082

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

clalancette
Copy link
Contributor

If the user creates SystemDefaultsQoS setting, they are explicitly asking for SystemDefaults. In that case, we should not warn, and just let the underlying RMW choose what it wants. Implement that here by passing a boolean parameter through that decides when we should print out the warning, and then skip printing that warning when SystemDefaultsQoS is created.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

If the user creates SystemDefaultsQoS setting, they are
explicitly asking for SystemDefaults.  In that case, we should
*not* warn, and just let the underlying RMW choose what it
wants.  Implement that here by passing a boolean parameter
through that decides when we should print out the warning,
and then skip printing that warning when SystemDefaultsQoS
is created.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

Note that this should fix ros2/rmw#341

Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

LGTM

@clalancette
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya fujitatomoya merged commit 3db2ece into rolling Jan 13, 2023
@delete-merged-branch delete-merged-branch bot deleted the clalancette/fix-system-defaults-warning branch January 13, 2023 21:47
@mmattb
Copy link

mmattb commented Jan 24, 2023

Since this change went in I see my nav2_util build failing with:

--- stderr: nav2_util                                                                          
/usr/bin/ld: /opt/ros/rolling/lib/libbondcpp.so: undefined reference to `rclcpp::KeepLast::KeepLast(unsigned long)'
collect2: error: ld returned 1 exit status
gmake[2]: *** [test/CMakeFiles/test_string_utils.dir/build.make:266: test/test_string_utils] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:347: test/CMakeFiles/test_string_utils.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....

Not sure why the type conversion unsigned long -> size_t isn't working there? When I change it to something like this there is no problem.

diff --git a/rclcpp/include/rclcpp/qos.hpp b/rclcpp/include/rclcpp/qos.hpp
index 2ad49487..e3721510 100644
--- a/rclcpp/include/rclcpp/qos.hpp
+++ b/rclcpp/include/rclcpp/qos.hpp
@@ -99,7 +99,8 @@ struct RCLCPP_PUBLIC KeepAll : public rclcpp::QoSInitialization
 /// Use to initialize the QoS with the keep_last history setting and the given depth.
 struct RCLCPP_PUBLIC KeepLast : public rclcpp::QoSInitialization
 {
-  explicit KeepLast(size_t depth, bool print_depth_warning = true);
+  explicit KeepLast(size_t depth);
+  explicit KeepLast(size_t depth, bool print_depth_warning);
 };
 
 /// Encapsulation of Quality of Service settings.
diff --git a/rclcpp/src/rclcpp/qos.cpp b/rclcpp/src/rclcpp/qos.cpp
index 2453149a..1e61bf09 100644
--- a/rclcpp/src/rclcpp/qos.cpp
+++ b/rclcpp/src/rclcpp/qos.cpp
@@ -78,6 +78,11 @@ KeepAll::KeepAll()
 : QoSInitialization(RMW_QOS_POLICY_HISTORY_KEEP_ALL, 0)
 {}
 
+KeepLast::KeepLast(size_t depth)
+: QoSInitialization(RMW_QOS_POLICY_HISTORY_KEEP_LAST, depth, true)
+{
+}
+
 KeepLast::KeepLast(size_t depth, bool print_depth_warning)
 : QoSInitialization(RMW_QOS_POLICY_HISTORY_KEEP_LAST, depth, print_depth_warning)
 {

What is C++ doing here? This is Ubuntu 22.04 / ros2_rolling. Does libbondcpp need recompiling? Does the Ubuntu package for it need updating? Not sure the C++ details there.

@clalancette
Copy link
Contributor Author

Almost certainly the problem is that you have a mixed workspace. That is, you have rclcpp in your overlay, and bondcpp in your underlay (/opt/ros/rolling). Since bondcpp hasn't been rebuilt against the new version of rclcpp, it has the wrong linkage.

Either you need to build everything from source here (including bondcpp), or you need to wait until we do an rclcpp release with this change in it (which will rebuild bondcpp underneath).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants