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

Migration ROS2 #13

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from
Draft

Migration ROS2 #13

wants to merge 27 commits into from

Conversation

ThomasPetrie
Copy link
Contributor

Draft PR and discussion space for ROS2 migration of project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In ROS2, if <build_depend>, <build_export_depend> and <exec_depend> tags are all present for one dependency, can replace with the tag to simplify
https://docs.ros.org/en/iron/How-To-Guides/Migrating-from-ROS1/Migrating-Package-XML.html#id7

target_include_directories(weighted_tracker PRIVATE
"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>"
${ament_INCLUDE_DIRS}
#${OpenCV_INCLUDE_DIRS}
Copy link
Contributor Author

@ThomasPetrie ThomasPetrie Dec 16, 2024

Choose a reason for hiding this comment

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

I noticed that we included OpenCV normally elsewhere, why do it differently here?

@@ -61,6 +58,13 @@ add_executable(weighted_tracker
src/tracker.cpp
)

target_include_directories(weighted_tracker PRIVATE
"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>"
Copy link
Contributor Author

@ThomasPetrie ThomasPetrie Dec 16, 2024

Choose a reason for hiding this comment

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

There's usually an /include folder where the .h files are located, and this is where we indicate where to find them. For the trackers, however, everything is in /src. Maybe it would be best if we moved them later?

find_package(detection REQUIRED)
find_package(serial REQUIRED)
find_package(std_msgs REQUIRED)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For these three packages, I'm not sure they are required. Since tracking is a package that is used in the cpp files, I thought I should add it (since now it is necessary to link everthing manually rather than letting catkin link everything), but I'm not sure it's needed.
For the std_msgs and sensor_msgs, in general I'm not sure they are needed in our architecture. At least they aren't needed here (included because tracking includes them).
Even tracking doesn't seem to use it (it only uses basic types for its messages)

@@ -11,19 +11,23 @@
<author email="[email protected]">Sébastien Darche</author>

<buildtool_depend>ament_cmake_ros</buildtool_depend>
<build_depend>rclcpp</build_depend>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if I want to add a package to the CMakeLists, I must add it as a dependency here. I added every package in ther CMakeLists as buildtime and runtime dependencies

@ThomasPetrie
Copy link
Contributor Author

Je devrais avoir fini le port des package.xml et des CMakeLists
À valider

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.

1 participant