[RSDK-11481] Implement cli to learn to manage camera #78

SebastianMunozP · 2025-07-29T21:15:51Z

https://viam.atlassian.net/browse/RSDK-11481

…ation

…ne RealSense Camera at the same time

… in device struct

lia-viam

LGTM, just some suggestions/points for discussion

src/cli/main.cpp

lia-viam · 2025-07-30T13:32:26Z

src/cli/main.cpp

+    return std::fabs(p.x) >= min_distance || std::fabs(p.y) >= min_distance || std::fabs(p.z) >= min_distance;
+}
+
+std::vector<unsigned char> RGBPointsToPCD(std::tuple<rs2::points, rs2::video_frame> data)


i think this implementation could be mostly copied from the one in my orbbec PR if you like

src/cli/main.cpp

packaging/appimages/viam-camera-realsense-cli-x86_64.yml

seanavery · 2025-08-05T15:06:23Z

src/cli/main.cpp

+    return std::fabs(p.x) >= min_distance || std::fabs(p.y) >= min_distance || std::fabs(p.z) >= min_distance;
+}
+
+std::vector<PointXYZRGB> getRGBPoints(std::pair<rs2::points, rs2::video_frame>&& data) {


Why do we need to use the rvalue && reference on the input pair?
Would we use rvalue ref so that we can use std::move?
If that was the case I would expect:

auto points = std::move(data.first); auto video_frame = std::move(data.second);

Also, curious why we pass in as a std::pair as opposed to separate args if we immediately split it up at the beginning of the method.

Still learning C++, so mostly asking for my understanding

Rvalue reference is the result of std::move. Its implementation is simply a static_cast to &&. See here for discussion:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-consume

I see, thanks for the ref!

Unresolving to ask a question. So does that mean the caller of this function always needs to pass in a std::move value? What if they don't

hexbabe

Left a mixture of cpp learning questions, regular questions, and suggestions

hexbabe · 2025-08-07T18:55:15Z

Makefile

+appimage-amd64-cli: export ARCH = x86_64
+appimage-amd64-cli: viam-camera-realsense
+	$(call BUILD_APPIMAGE,$(OUTPUT_NAME),$(ARCH))
+	cp ./packaging/appimages/$(OUTPUT_NAME)-*-$(ARCH).AppImage ./packaging/appimages/deploy/


[nit] missing newline eof

hexbabe · 2025-08-07T19:07:40Z

packaging/appimages/viam-camera-realsense-cli-x86_64.yml

+  if [ "${SANITIZE}" = "ON" ]; then
+    mkdir -p $TARGET_APPDIR/usr/lib
+    cp /lib/x86_64-linux-gnu/libabsl_* $TARGET_APPDIR/usr/lib/
+    cp /lib/x86_64-linux-gnu/libboost_* $TARGET_APPDIR/usr/lib/
+    cp /lib/x86_64-linux-gnu/libicu* $TARGET_APPDIR/usr/lib/
+  fi


have we tested if ASAN works with this binary?

hexbabe · 2025-08-07T19:13:56Z

src/cli/main.cpp

+
+#include <librealsense2/rs.hpp>
+
+static const double min_distance = 1e-6;


unclear why we need to filter min_distance. Is it to reduce noise?

hexbabe · 2025-08-07T19:14:17Z

src/cli/main.cpp

+    unsigned int rgb;
+};
+
+bool validPoint(const rs2::vertex &p)


Suggested change

bool validPoint(const rs2::vertex &p)

bool isValidPoint(const rs2::vertex &p)

hexbabe · 2025-08-07T19:20:45Z

src/cli/main.cpp

+    return std::fabs(p.x) >= min_distance || std::fabs(p.y) >= min_distance || std::fabs(p.z) >= min_distance;
+}
+
+std::vector<PointXYZRGB> getRGBPoints(std::pair<rs2::points, rs2::video_frame>&& data) {


Unresolving to ask a question. So does that mean the caller of this function always needs to pass in a std::move value? What if they don't

hexbabe · 2025-08-07T19:41:16Z

src/cli/main.cpp

+// If they match, it returns true and prints the matching parameters
+// If they do not match, it returns false
+// alignment
+bool checkIfMatchingColorDepthProfiles(const rs2::video_stream_profile &color,


This feels like something the SDK should take care of... is there no utility they provide that does this?

hexbabe · 2025-08-07T19:43:40Z

src/cli/main.cpp

+    auto cfg = createSwD2CAlignConfig(my_dev->pipe, ctx, my_dev->device);
+    if (cfg == nullptr)
+    {
+        std::cerr << "Failed to create matching color-depth config for device: " << my_dev->serial_number << std::endl;


why do we handle the error in this way? why not throw

hexbabe · 2025-08-07T19:44:37Z

src/cli/main.cpp

+            auto dsp = dp.as<rs2::video_stream_profile>();
+            if (dsp.format() != RS2_FORMAT_Z16)
+            {
+                continue; // Only consider Z16 format for depth


sorry if this is already something we do in the realsense module that is just being mirrored here, but is there a reason why Z16 and RGB8 are the formats we consider valid, as well as matching?

hexbabe · 2025-08-07T19:46:44Z

src/cli/main.cpp

+
+    std::cout << "waiting for key press\n";
+    std::cin.get();
+    std::cout << "stopping orbbec program" << std::endl;


hexbabe · 2025-08-07T19:46:56Z

src/cli/main.cpp

+    devices_by_serial().clear();
+    std::lock_guard<std::mutex> frame_lock(frame_set_by_serial_mu());
+    frame_set_by_serial().clear();
+    std::cout << "stopped orbbec program" << std::endl;


SebastianMunozP added 9 commits July 24, 2025 19:08

[RealSense] Adding first version of the CLI

62e8e56

[RealSense] Improvements to CLI

1b3d172

[RealSense] CLI app able to write a pcd file

54922ba

Merge branch 'main' of github.com:SebastianMunozP/viam-camera-realsense

31221f8

[RealSense] Adding stopStreams()

1ed4d4c

[RealSense] Formatting source file

5b321e0

[RealSense] Fixing unplugging issue that caused sudden program termin…

7b4e0e1

…ation

[RealSense] Fix the issue that prevented us from handling more than o…

ceac00b

…ne RealSense Camera at the same time

[RealSense] Adding support for FPS logging, bundling dev related data…

58e3173

… in device struct

SebastianMunozP requested review from hexbabe, lia-viam, nicksanford and seanavery July 29, 2025 21:16

lia-viam reviewed Jul 30, 2025

View reviewed changes

SebastianMunozP force-pushed the RSDK-11481-Implement-CLI-to-learn-to-manage-camera branch from 92d8e00 to 58e3173 Compare August 4, 2025 02:39

SebastianMunozP added 2 commits August 3, 2025 23:24

[RSDK-11481] Addressing suggestions from Lia

65d0c9d

[RSDK-11481] Improving RGBPointsToPCD

e536006

seanavery reviewed Aug 5, 2025

View reviewed changes

hexbabe reviewed Aug 7, 2025

View reviewed changes


		#include <librealsense2/rs.hpp>

		static const double min_distance = 1e-6;

	bool validPoint(const rs2::vertex &p)
	bool isValidPoint(const rs2::vertex &p)

[RSDK-11481] Implement cli to learn to manage camera #78

Are you sure you want to change the base?

[RSDK-11481] Implement cli to learn to manage camera #78

Uh oh!

Conversation

SebastianMunozP commented Jul 29, 2025

Uh oh!

lia-viam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanavery Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hexbabe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seanavery Aug 5, 2025 •

edited

Loading