-
Notifications
You must be signed in to change notification settings - Fork 78
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 up warnings and errors from latest Rolling releases. #99
Fix up warnings and errors from latest Rolling releases. #99
Conversation
This cleans up two problems: 1. rclcpp no longer accepts bare "declare_parameters" without a type, so add an empty string by default. 2. The tf2_geometry_msgs headers all should now use the .hpp suffix, so fix that as well. This should allow this package to build without warnings on the buildfarm. Signed-off-by: Chris Lalancette <[email protected]>
ros2_ouster/src/ouster_driver.cpp
Outdated
@@ -48,8 +48,8 @@ OusterDriver::OusterDriver( | |||
this->declare_parameter("proc_mask", std::string("IMG|PCL|IMU|SCAN")); | |||
|
|||
// Declare parameters used across ALL _sensor_ implementations | |||
this->declare_parameter("lidar_ip"); | |||
this->declare_parameter("computer_ip"); | |||
this->declare_parameter("lidar_ip", std::string("")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally don't want a default, is there not an API that sets the type without setting the value to something? I would like it to crash immediately if its not provided with a valid set of IPs. I don't want to even attempt to connect to a bogus address since the networking aspects of sensors are generally limited and I'm not sure it would successfully exit as invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, maybe. Let me play around with setting the type via templating to see if that is happier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that works. See 3dbecf1 .
Signed-off-by: Chris Lalancette <[email protected]>
Thanks! |
I'll run a sync with Rolling on Friday with some other packages. I haven't done a full sync across Nav2 in a long time now |
Perfect, thanks! |
) * Fix up warnings and errors from latest Rolling releases. This cleans up two problems: 1. rclcpp no longer accepts bare "declare_parameters" without a type, so add an empty string by default. 2. The tf2_geometry_msgs headers all should now use the .hpp suffix, so fix that as well. This should allow this package to build without warnings on the buildfarm. Signed-off-by: Chris Lalancette <[email protected]> * Feedback from review. Signed-off-by: Chris Lalancette <[email protected]>
This cleans up two problems:
a type, so add an empty string by default.
suffix, so fix that as well.
This should allow this package to build without warnings on the
buildfarm.
Signed-off-by: Chris Lalancette [email protected]
This should fix the failing builds, like in https://build.ros2.org/view/Rbin_uJ64/job/Rbin_uJ64__ros2_ouster__ubuntu_jammy_amd64__binary/