-
Notifications
You must be signed in to change notification settings - Fork 315
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
Component (Draft) #132
base: humble-devel
Are you sure you want to change the base?
Component (Draft) #132
Conversation
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.
Some hints for easier code review
@@ -4,6 +4,12 @@ project(aruco_ros) | |||
if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") | |||
add_compile_options(-Wall -Wextra -Wpedantic) | |||
endif() | |||
|
|||
# compile release by default | |||
if(NOT CMAKE_BUILD_TYPE) |
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.
default Release compile
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.
Nice 👍
Good one
add_executable(marker_publisher src/marker_publish.cpp | ||
src/aruco_ros_utils.cpp) | ||
target_include_directories(marker_publisher | ||
add_library(marker_publisher_component |
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.
marker_publisher_component
|
||
rclcpp_components_register_node(marker_publisher_component | ||
PLUGIN "aruco_ros::ArucoMarkerPublisherComponent" | ||
EXECUTABLE marker_publisher) |
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.
executable to maintain current behavior
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.
Yes, better to maintain current behaviour
install(TARGETS marker_publisher single double | ||
DESTINATION lib/${PROJECT_NAME}) | ||
install(TARGETS marker_publisher_component single double | ||
LIBRARY DESTINATION lib |
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.
library installation
#include "tf2_geometry_msgs/tf2_geometry_msgs.hpp" | ||
#include "tf2_ros/buffer.h" |
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.
alphabetic sort
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.
👍
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.
Hello @mhubii!
First of all, thank you so much for your contribution.
We understand in some instances, we might need components. As we can also maintain the current behavior. We are fine with the proposed changes.
@@ -4,6 +4,12 @@ project(aruco_ros) | |||
if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") | |||
add_compile_options(-Wall -Wextra -Wpedantic) | |||
endif() | |||
|
|||
# compile release by default | |||
if(NOT CMAKE_BUILD_TYPE) |
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.
Nice 👍
Good one
#include "tf2_geometry_msgs/tf2_geometry_msgs.hpp" | ||
#include "tf2_ros/buffer.h" |
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.
👍
|
||
rclcpp_components_register_node(marker_publisher_component | ||
PLUGIN "aruco_ros::ArucoMarkerPublisherComponent" | ||
EXECUTABLE marker_publisher) |
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.
Yes, better to maintain current behaviour
hi @saikishor , thank you so much for the review! I'll continue working on it a little, add some tests etc. Will let you know once ready. |
Thank you so much! |
sorry this PR is getting a little out of hand and might be difficult for review. I did some unnecessary formatting. But now supports parameter re-configure and the current behavior should remain unchanged. Happy to role back a little on the formatting changes and open new PR. |
Problem
High latency over DDS for high resolution cameras like
zed2i
Solution
Component-based architecture for intra-process communication. Initially for
marker_publisher
To do
marker_publish
componentsimple_double
componentsimple_single
componentConvenience launch mixins(better other PR)Current components
Notes
Could you please give me a hand here @Noel215, thank you so much!