Add reboot service #359

mattrouss · 2025-07-11T13:45:57Z

Add /reboot service to restart camera on failure. Support USB and GMSL.
Please note that GMSL reboot impacts all connected cameras.

Copilot

Pull Request Overview

Adds a new /reboot service to allow restarting the ZED camera on failure, with support for both USB and GMSL inputs.

Registers the reboot service in initServices
Implements camera restart logic (restartCamera, threadFunc_zedRestart) and synchronization helpers (guardDataThread, releaseDataThread)
Updates grab and sensor threads to pause during camera reboot

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
zed_camera_component_main.cpp	Added reboot service, restart logic, synchronization, and callback
zed_camera_component.hpp	Declared new methods, members, and service pointer for reboot
sl_types.hpp	Added `rebootSrvPtr` typedef

Comments suppressed due to low confidence (3)

zed_components/src/zed_camera/include/zed_camera_component.hpp:1017

[nitpick] The name mSrvReboot (service name) is ambiguous next to mRebootSrv (service pointer). Rename it to mSrvRebootName for consistency with other service name members.

  const std::string mSrvReboot = "reboot";

zed_components/src/zed_camera/include/zed_camera_component.hpp:44

[nitpick] New methods (guardDataThread, releaseDataThread, restartCamera, threadFunc_zedRestart, callback_reboot) lack comments. Please add Doxygen or inline documentation explaining their purpose and usage.

  void guardDataThread();

zed_components/src/zed_camera/src/zed_camera_component_main.cpp:294

The new /reboot service and restartCamera logic are not covered by existing tests. Consider adding unit or integration tests for both successful and failure reboot scenarios.

  srv_name = srv_prefix + mSrvReboot;

Copilot · 2025-07-11T13:46:56Z

zed_components/src/zed_camera/src/zed_camera_component_main.cpp

+      // Wait until camera has been closed by reboot service call
+      mRebootCondVar.wait(lock, [&]() { return !mCameraRebooting; });
+
+      if (mRestartThread.joinable()) {


Joining the restart thread while holding mRebootMutex can lead to deadlocks or block other camera threads. Consider moving the join() call outside the locked section or unlocking before joining.

Suggested change

if (mRestartThread.joinable()) {

bool shouldJoin = mRestartThread.joinable();

lock.unlock();

if (shouldJoin) {

github-actions · 2025-08-11T00:39:04Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment otherwise it will be automatically closed in 5 days

Myzhar · 2025-08-11T02:56:02Z

Keep alive

github-actions · 2025-09-12T00:31:05Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment otherwise it will be automatically closed in 5 days

Myzhar · 2025-09-12T06:08:15Z

To be merged...

Copilot

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-12T06:11:01Z

zed_components/src/zed_camera/src/zed_camera_component_main.cpp

+  RCLCPP_INFO(get_logger(), "=== RESTARTING CAMERA ===");
+
+  // Wait longer for GMSL camera connection
+  int camera_timeout_sec = sl_tools::isZEDX(mCamRealModel) ? 15: mCamTimeoutSec;


The magic number 15 should be defined as a named constant (e.g., ZEDX_CAMERA_TIMEOUT_SEC) to improve code maintainability and make the special timeout value for ZEDX cameras more explicit.

Copilot · 2025-09-12T06:11:02Z

zed_components/src/zed_camera/src/zed_camera_component_main.cpp

+    mConnStatus = mZed->open(mInitParams);
+
+    if (mConnStatus == sl::ERROR_CODE::SUCCESS) {
+      DEBUG_STREAM_COMM("Opening successfull");


There's a typo in the debug message: 'successfull' should be 'successful'.

Suggested change

DEBUG_STREAM_COMM("Opening successfull");

DEBUG_STREAM_COMM("Opening successful");

Copilot · 2025-09-12T06:11:02Z

zed_components/src/zed_camera/src/zed_camera_component_main.cpp

+  int camera_timeout_sec = sl_tools::isZEDX(mCamRealModel) ? 15: mCamTimeoutSec;
+
+
+  while (1) {


Use while (true) instead of while (1) for better readability and to follow modern C++ best practices.

Suggested change

while (1) {

while (true) {

Copilot · 2025-09-12T06:11:02Z

zed_components/src/zed_camera/src/zed_camera_component_main.cpp

+
+    rclcpp::sleep_for(std::chrono::seconds(camera_timeout_sec));
+  }
+  return false;


The return false; statement on line 3381 is unreachable code because the infinite while (1) loop on line 3329 only exits via return statements inside the loop.

Suggested change

return false;

Copilot · 2025-09-12T06:11:03Z

zed_components/src/zed_camera/src/zed_camera_component_main.cpp

+
+  // If reboot thread is waiting and no threads are accessing ZED, notify it
+  if (mCameraRebooting && activeUsers == 0) {
+      mRebootCondVar.notify_all();


The indentation is inconsistent here. The code inside the if statement has extra spaces that don't match the project's indentation style.

Suggested change

mRebootCondVar.notify_all();

mRebootCondVar.notify_all();

Copilot · 2025-09-12T06:11:03Z

zed_components/src/zed_camera/src/zed_camera_component_main.cpp

+      std::lock_guard<std::mutex> lock(mRebootMutex);
+      mCameraRebooting = false;


The indentation is inconsistent here. The code inside the scope has extra spaces that don't match the project's indentation style.

Suggested change

std::lock_guard<std::mutex> lock(mRebootMutex);

mCameraRebooting = false;

std::lock_guard<std::mutex> lock(mRebootMutex);

mCameraRebooting = false;

Copilot · 2025-09-12T06:11:03Z

zed_components/src/zed_camera/src/zed_camera_component_main.cpp

+      std::lock_guard<std::mutex> lock(mRebootMutex);
+      mCameraRebooting = true;


The indentation is inconsistent here. The code inside the scope has extra spaces that don't match the project's indentation style.

Suggested change

std::lock_guard<std::mutex> lock(mRebootMutex);

mCameraRebooting = true;

std::lock_guard<std::mutex> lock(mRebootMutex);

mCameraRebooting = true;

Copilot · 2025-09-12T06:11:03Z

zed_components/src/zed_camera/src/zed_camera_component_main.cpp

+      std::lock_guard<std::mutex> lock(mRebootMutex);
+      mCameraRebooting = true;
+  }
+
+  // Wait for all threads to stop using camera
+  {
+      std::unique_lock<std::mutex> lock(mRebootMutex);
+      mRebootCondVar.wait(lock, [&]() { return activeUsers == 0; });


The indentation is inconsistent here. The code inside the scope has extra spaces that don't match the project's indentation style.

Suggested change

std::lock_guard<std::mutex> lock(mRebootMutex);

mCameraRebooting = true;

}

// Wait for all threads to stop using camera

{

std::unique_lock<std::mutex> lock(mRebootMutex);

mRebootCondVar.wait(lock, [&]() { return activeUsers == 0; });

std::lock_guard<std::mutex> lock(mRebootMutex);

mCameraRebooting = true;

}

// Wait for all threads to stop using camera

{

std::unique_lock<std::mutex> lock(mRebootMutex);

mRebootCondVar.wait(lock, [&]() { return activeUsers == 0; });

Copilot · 2025-09-12T06:11:04Z

zed_components/src/zed_camera/src/zed_camera_component_main.cpp

+      }
+
+        // Mark that this thread is using the camera
+        ++activeUsers;


The indentation is inconsistent here. This line has extra spaces that don't match the surrounding code's indentation style.

Suggested change

++activeUsers;

++activeUsers;

github-actions · 2025-10-13T00:34:52Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment otherwise it will be automatically closed in 5 days

github-actions · 2025-11-13T00:34:27Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment otherwise it will be automatically closed in 5 days

Matthew Rousseau and others added 3 commits June 25, 2025 17:33

[wip] Add reboot service with thread safety

3186f2f

Restart camera in grab thread after reboot call

e924d1d

Run camera restart loop in separate thread

184eefa

mattrouss requested review from Myzhar and Copilot July 11, 2025 13:45

Copilot AI reviewed Jul 11, 2025

View reviewed changes

github-actions bot added the Stale label Aug 11, 2025

github-actions bot removed the Stale label Aug 12, 2025

github-actions bot added the Stale label Sep 12, 2025

Myzhar requested a review from Copilot September 12, 2025 06:08

Copilot AI reviewed Sep 12, 2025

View reviewed changes

github-actions bot removed the Stale label Sep 13, 2025

github-actions bot added the Stale label Oct 13, 2025

Myzhar removed the Stale label Oct 13, 2025

github-actions bot added Stale closed_for_stale Issue closed for inactivity labels Nov 13, 2025

github-actions bot closed this Nov 19, 2025

Myzhar reopened this Nov 19, 2025

github-actions bot removed closed_for_stale Issue closed for inactivity Stale labels Nov 20, 2025

-      if (mRestartThread.joinable()) {
+      bool shouldJoin = mRestartThread.joinable();
+      lock.unlock();
+      if (shouldJoin) {

	DEBUG_STREAM_COMM("Opening successfull");
	DEBUG_STREAM_COMM("Opening successful");

		int camera_timeout_sec = sl_tools::isZEDX(mCamRealModel) ? 15: mCamTimeoutSec;


		while (1) {

		std::lock_guard<std::mutex> lock(mRebootMutex);
		mCameraRebooting = false;

		std::lock_guard<std::mutex> lock(mRebootMutex);
		mCameraRebooting = true;

Add reboot service #359

Are you sure you want to change the base?

Add reboot service #359

Uh oh!

Conversation

mattrouss commented Jul 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 11, 2025

Uh oh!

Myzhar commented Aug 11, 2025

Uh oh!

github-actions bot commented Sep 12, 2025

Uh oh!

Myzhar commented Sep 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 13, 2025

Uh oh!

github-actions bot commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants