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

Component (Draft) #132

Open
wants to merge 12 commits into
base: humble-devel
Choose a base branch
from
Open

Conversation

mhubii
Copy link

@mhubii mhubii commented Apr 4, 2024

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 component
    • Camera info sub (availability issues)
    • Improved logging on start
    • Formatting
  • simple_double component
  • simple_single component
    • Dynamic parameter re-configures
    • Improved logging on start
    • Formatting
  • Convenience launch mixins (better other PR)
  • Examples / tests
  • Update the readme

Current components

component_types

Notes

  • Very happy to iterate and add for other nodes
  • Happy to add tests etc., improve upon launch files, write a bringup package
  • Need some quick feedback if this is desired by the authors!

Could you please give me a hand here @Noel215, thank you so much!

Copy link
Author

@mhubii mhubii left a 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)
Copy link
Author

Choose a reason for hiding this comment

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

default Release compile

Copy link
Member

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
Copy link
Author

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)
Copy link
Author

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

Copy link
Member

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
Copy link
Author

Choose a reason for hiding this comment

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

library installation

aruco_ros/package.xml Show resolved Hide resolved
#include "tf2_geometry_msgs/tf2_geometry_msgs.hpp"
#include "tf2_ros/buffer.h"
Copy link
Author

Choose a reason for hiding this comment

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

alphabetic sort

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@saikishor saikishor left a 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)
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍
Good one

aruco_ros/package.xml Show resolved Hide resolved
aruco_ros/package.xml Show resolved Hide resolved
#include "tf2_geometry_msgs/tf2_geometry_msgs.hpp"
#include "tf2_ros/buffer.h"
Copy link
Member

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)
Copy link
Member

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

@mhubii
Copy link
Author

mhubii commented Apr 5, 2024

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.

@saikishor
Copy link
Member

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!

@mhubii
Copy link
Author

mhubii commented Apr 8, 2024

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.

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.

2 participants