Thagdorn/picture saver by TomHagdorn · Pull Request #2 · carologistics/vision_system

TomHagdorn · 2026-03-11T11:13:30Z

add srv for taking pictures with the picam

Copilot

Pull request overview

Adds a ROS 2 service to persist camera frames from picam_client to disk, configurable via a default save directory parameter.

Changes:

Introduces a new SavePicture service definition and wires it into the node.
Adds save_directory parameter (and config) plus runtime directory creation and PNG writing.
Stores a “latest image” by subscribing to the node’s own camera/image topic.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`picam_client/srv/SavePicture.srv`	Defines request/response for saving one or more images with an interval.
`picam_client/src/picam_client_node.cpp`	Implements `/picam_client/save_picture`, adds save-directory param, stores latest image via self-subscription, changes image encoding on publish.
`picam_client/include/picam_client/picam_client_node.hpp`	Declares new service, subscription, and `latest_image_` storage.
`picam_client/config/params.yaml`	Adds default `save_directory` parameter value.
`picam_client/CMakeLists.txt`	Registers the new service interface for code generation.

Comments suppressed due to low confidence (1)

picam_client/CMakeLists.txt:23

This PR introduces std::filesystem usage, but the package does not set a C++ standard in CMake. On some toolchains/distros this may still default to C++14 and fail to compile/link. Set CMAKE_CXX_STANDARD 17 (or use target_compile_features(... cxx_std_17)) for this package/target.

find_package(cv_bridge REQUIRED)
find_package(OpenCV REQUIRED)
find_package(rosidl_default_generators REQUIRED)

rosidl_generate_interfaces(${PROJECT_NAME}
  "srv/SavePicture.srv"
  "srv/SetConfidence.srv"
  "srv/SetIOU.srv"
  "srv/StreamControl.srv"
  DEPENDENCIES std_msgs

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

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T11:28:04Z


  auto msg =
-      cv_bridge::CvImage(std_msgs::msg::Header(), "bgr8", img).toImageMsg();
+      cv_bridge::CvImage(std_msgs::msg::Header(), "rgb8", img).toImageMsg();


cv::imdecode(..., cv::IMREAD_COLOR) returns a BGR image, but the message is published with encoding "rgb8". This makes the sensor_msgs/Image encoding inconsistent with the underlying pixel data and will swap channels for downstream consumers (including your own toCvCopy(..., BGR8) subscriber). Publish as bgr8 or convert the Mat to RGB before publishing with rgb8.

Suggested change

cv_bridge::CvImage(std_msgs::msg::Header(), "rgb8", img).toImageMsg();

cv_bridge::CvImage(std_msgs::msg::Header(), "bgr8", img).toImageMsg();

Copilot · 2026-03-11T11:28:04Z

+  for (int i = 0; i < count; ++i) {
+    // Wait for the interval before taking subsequent pictures
+    if (i > 0) {
+      std::this_thread::sleep_for(
+          std::chrono::milliseconds(static_cast<int>(interval * 1000)));
+    }
+
+    // Grab the latest image
+    cv::Mat image;
+    if (latest_image_.empty()) {
+      response->success = false;
+      response->message = "No image available from camera";
+      response->saved_files = saved_files;
+      return;
+    }
+    image = latest_image_.clone();


handle_save_picture runs in the same (single-threaded) executor as the image_sub_ callback (see main() using rclcpp::spin). While this service callback is sleeping/writing files, the subscription callback cannot run, so latest_image_ won’t update and the loop will likely save the same frame repeatedly (and starve other callbacks). Consider updating latest_image_ directly in handle_image_message (read thread) with proper synchronization, or moving save work to a separate thread / callback group with a multithreaded executor.

Copilot · 2026-03-11T11:28:05Z

+  // Determine save directory
+  std::string save_dir = request->save_directory.empty()
+                             ? save_directory_
+                             : request->save_directory;
+
+  // Create directory if it doesn't exist
+  try {
+    std::filesystem::create_directories(save_dir);
+  } catch (const std::filesystem::filesystem_error &e) {


The service allows callers to provide an arbitrary save_directory and the node will create directories and write files there. In ROS deployments where services are reachable over the network, this is a filesystem write primitive. Consider restricting saves to a configured base directory (e.g., ignore/validate request paths, require relative paths, reject ../absolute paths) or adding an explicit allowlist.

Copilot · 2026-03-11T11:28:05Z

+  int count = request->count <= 0 ? 1 : request->count;
+  float interval = request->interval <= 0.0f ? 1.0f : request->interval;
+
+  std::vector<std::string> saved_files;
+
+  for (int i = 0; i < count; ++i) {


count is effectively unbounded (any positive int32). A large request can block the node for a long time and potentially fill disk. Consider enforcing a reasonable maximum (constant/parameter) and returning an error when exceeded.

Copilot · 2026-03-11T11:28:05Z

+  RCLCPP_INFO(get_logger(), "Service '/save_picture' created at: %s",
+              save_picture_srv_->get_service_name());


The log line says Service '/save_picture' created... but the actual service name registered is /picam_client/save_picture. This is misleading when debugging service discovery; update the log label to match the real service path.

Copilot · 2026-03-11T11:28:05Z

+string save_directory           # Directory to save pictures (empty = use default from params)
+int32 count                     # Number of pictures to take (default: 1)
+float32 interval                # Interval between pictures in seconds (default: 1.0)
+---
+bool success
+string message
+string[] saved_files             # List of saved file paths


The save_directory field is used directly by handle_save_picture as a filesystem path, allowing any ROS client that can call this service to choose an arbitrary directory for file creation. An attacker can supply values like /tmp/attacker or ../../var/app/data and the node will pass this straight into std::filesystem::create_directories and cv::imwrite, enabling path traversal and unauthorized creation of directories/files outside the intended picture directory. To mitigate this, constrain save_directory to a configured base path (e.g. normalize and enforce that it stays under save_directory_, or expose only predefined subdirectories/relative names instead of accepting an arbitrary absolute path).

Suggested change

string save_directory # Directory to save pictures (empty = use default from params)

int32 count # Number of pictures to take (default: 1)

float32 interval # Interval between pictures in seconds (default: 1.0)

---

bool success

string message

string[] saved_files # List of saved file paths

string save_directory # Relative subdirectory under the configured base picture directory

# (empty = use default from params; must NOT be an absolute path or contain "..")

int32 count # Number of pictures to take (default: 1)

float32 interval # Interval between pictures in seconds (default: 1.0)

---

bool success

string message

string[] saved_files # List of saved file paths (relative to the configured base picture directory)

TomHagdorn · 2026-03-11T12:10:03Z

@copilot open a new pull request to apply changes based on the comments in this thread

Copilot · 2026-03-11T12:10:13Z

@TomHagdorn I've opened a new pull request, #3, to work on those changes. Once the pull request is ready, I'll request review from you.

TomHagdorn added 4 commits March 10, 2026 20:04

add service and logic

f9f1b62

forgot to add service

1892a26

remove mutex and burst capability

df5060e

change to rgb

37e7777

TomHagdorn requested a review from Copilot March 11, 2026 11:21

Copilot started reviewing on behalf of TomHagdorn March 11, 2026 11:21 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

Copilot AI mentioned this pull request Mar 11, 2026

Fix BGR encoding, thread safety, path traversal, and count limit in SavePicture service #3

Draft

Navigation Menu

Thagdorn/picture saver - #2

Thagdorn/picture saver#2
TomHagdorn wants to merge 4 commits into
mainfrom
thagdorn/picture_saver

TomHagdorn commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

TomHagdorn commented Mar 11, 2026

Uh oh!

Copilot AI commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	cv_bridge::CvImage(std_msgs::msg::Header(), "rgb8", img).toImageMsg();
	cv_bridge::CvImage(std_msgs::msg::Header(), "bgr8", img).toImageMsg();

		RCLCPP_INFO(get_logger(), "Service '/save_picture' created at: %s",
		save_picture_srv_->get_service_name());

Conversation

TomHagdorn commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

TomHagdorn commented Mar 11, 2026

Uh oh!

Copilot AI commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants