Skip to content

Comments

cpp camera merge to main#29

Open
DueroBone wants to merge 18 commits intomainfrom
dmood/cpp-camera
Open

cpp camera merge to main#29
DueroBone wants to merge 18 commits intomainfrom
dmood/cpp-camera

Conversation

@DueroBone
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings February 8, 2026 21:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR migrates the ROS2 camera publisher from the autonomous_kart Python package to a new C++ (ament_cmake) package (autonomous_kart_cpp) and updates launch/scripts to use the C++ camera node.

Changes:

  • Added a new autonomous_kart_cpp package with a C++ camera_node that publishes /camera/image_raw via OpenCV/cv_bridge.
  • Updated bringup launch files to run the C++ camera node; removed the old Python camera node implementation.
  • Updated autonomous_kart packaging/script files and added workspace/editor artifacts.

Reviewed changes

Copilot reviewed 11 out of 17 changed files in this pull request and generated 15 comments.

Show a summary per file
File Description
src/autonomous_kart_cpp/src/camera_node.cpp New C++ camera node with simulation video playback + publishing thread/timer logic
src/autonomous_kart_cpp/CMakeLists.txt New build rules for the C++ camera executable
src/autonomous_kart_cpp/package.xml New package manifest for the C++ camera package
src/autonomous_kart/setup.py Disables Python camera package and alters camera_node console script entry point
src/autonomous_kart/autonomous_kart/launch/bringup_sim.launch.py Switches camera node launch from autonomous_kart to autonomous_kart_cpp
src/autonomous_kart/autonomous_kart/launch/bringup_pi.launch.py Switches camera node launch from autonomous_kart to autonomous_kart_cpp
src/autonomous_kart/autonomous_kart/nodes/opencv_pathfinder/opencv_pathfinder_node.py Adds timing variables in image callback
src/autonomous_kart/autonomous_kart/nodes/camera/camera_node.py Removes the previous Python camera node
scripts/start.bash Builds with --symlink-install before launching
src/autonomous_kart/autonomous_kart/params/pathfinder.yaml Whitespace-only change
.vscode/settings.json Adds VS Code workspace settings (includes absolute paths)
.vscode/c_cpp_properties.json Adds VS Code C++ IntelliSense configuration
.vscode/browse.vc.db-wal Adds VS Code generated DB artifact
.vscode/browse.vc.db-shm Adds VS Code generated DB artifact
data/.DS_Store Adds macOS Finder metadata file
.DS_Store Adds macOS Finder metadata file
Comments suppressed due to low confidence (1)

src/autonomous_kart/autonomous_kart/launch/bringup_sim.launch.py:46

  • These launch files now reference the autonomous_kart_cpp package for the camera node. The autonomous_kart package manifest should declare a runtime dependency on autonomous_kart_cpp (so the launch file’s dependency is explicit and installations don’t fail at runtime).
                    Node(
                        package="autonomous_kart_cpp",
                        executable="camera_node",
                        name="camera_node",
                        parameters=[camera_yaml],
                    ),

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +35 to +36
std::string video_path = "/ws/data/EVC_test_footage/vid.mp4"; // using low quality video for performance
cap_.open(video_path);
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

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

The simulation video path is hard-coded to /ws/data/.../vid.mp4, which makes the node non-portable and hard to deploy. Consider declaring a video_path parameter (with a reasonable default) and using that instead.

Copilot uses AI. Check for mistakes.
find_package(OpenCV REQUIRED)

add_executable(camera_node src/camera_node.cpp)
ament_target_dependencies(camera_node rclcpp sensor_msgs cv_bridge OpenCV)
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

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

ament_target_dependencies(camera_node ... OpenCV) may not be the right way to link against system OpenCV. If this fails to link/include correctly, prefer using target_link_libraries(camera_node ${OpenCV_LIBS}) (or OpenCV imported targets) and target_include_directories with ${OpenCV_INCLUDE_DIRS}.

Suggested change
ament_target_dependencies(camera_node rclcpp sensor_msgs cv_bridge OpenCV)
ament_target_dependencies(camera_node rclcpp sensor_msgs cv_bridge)
target_include_directories(camera_node PUBLIC ${OpenCV_INCLUDE_DIRS})
target_link_libraries(camera_node ${OpenCV_LIBRARIES})

Copilot uses AI. Check for mistakes.
#include "cv_bridge/cv_bridge.h"
#include "opencv2/opencv.hpp"
#include <thread>
#include <mutex>
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

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

This file uses std::vector and std::chrono (and std::bind) but does not include <vector> / <chrono> / <functional>. Relying on transitive includes is brittle and can break builds with different standard libraries; please include the headers you directly use.

Suggested change
#include <mutex>
#include <mutex>
#include <chrono>
#include <functional>
#include <vector>

Copilot uses AI. Check for mistakes.
self.angle_pub.publish(self.angle_msg)
endTime = self.get_clock().now()
elapsedTime = (endTime - startTime).nanoseconds / 1e6
# self.logger.info(f"Processing time: {elapsedTime:.2f} ms")
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

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

startTime, endTime, and elapsedTime are assigned but never used (the logging line is commented out). This will fail ament_flake8 (F841) in this repo; either use the values (re-enable logging) or remove these variables.

Suggested change
# self.logger.info(f"Processing time: {elapsedTime:.2f} ms")
self.logger.info(f"Processing time: {elapsedTime:.2f} ms")

Copilot uses AI. Check for mistakes.
}
else
{
cap_.open(0);
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

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

In REAL mode (simulation_mode false), no thread/timer path ever calls cap_.read(...) to populate latest_frame_, so the node will continuously warn and never publish images. Either read from cap_ in timer_callback() for real mode or start a reader thread for both modes.

Suggested change
cap_.open(0);
cap_.open(0);
if (!cap_.isOpened())
{
RCLCPP_ERROR(this->get_logger(), "Failed to open camera device 0");
}
else
{
video_fps_ = cap_.get(cv::CAP_PROP_FPS);
reader_thread_ = std::thread(&CameraNode::read_frames, this);
}

Copilot uses AI. Check for mistakes.
self.angle_msg.data = angles
self.angle_pub.publish(self.angle_msg)
endTime = self.get_clock().now()
elapsedTime = (endTime - startTime).nanoseconds / 1e6
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

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

Variable elapsedTime is not used.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@ShayManor ShayManor left a comment

Choose a reason for hiding this comment

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

Please remove the .vscode folder

Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete this

Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete

.DS_Store Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove

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