Skip to content

[RSDK-11250, RSDK-11251, RSDK-11784, RSDK-11807] - Cpp improvements #27

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.

Sign up for GitHub

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

Merged
seanavery merged 14 commits into from
Sep 3, 2025
Merged

[RSDK-11250, RSDK-11251, RSDK-11784, RSDK-11807] - Cpp improvements #27

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8b8430d
Use cpp sdk logger
seanavery Aug 28, 2025
bfacd8a
Remove namespace log
seanavery Aug 28, 2025
7f320ac
Throw instead of exit
seanavery Aug 28, 2025
5302f27
Update readme
seanavery Aug 28, 2025
f30f89a
Implement get properties
seanavery Aug 28, 2025
e561abd
Remove extra err log
seanavery Sep 2, 2025
dd28139
Downgrade unimplemented to warns
seanavery Sep 2, 2025
f6b2adc
Wrap serve in try catch for better logging
seanavery Sep 2, 2025
b8cbc64
More exception cleanup
seanavery Sep 2, 2025
19e0918
Remove exception log dups in init_csi
seanavery Sep 2, 2025
de8d9b6
Cleaner resource management in stop_pipeline
seanavery Sep 2, 2025
e466d23
Add linter
seanavery Sep 2, 2025
b59e334
Add lint workflow
seanavery Sep 2, 2025
e911063
Lint
seanavery Sep 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,7 @@ Once configured, check the [Logs tab](https://docs.viam.com/program/debug/) of y
"attributes": {
"width_px": 1920,
"height_px": 1080,
"frame_rate": 30,
"debug": true
"frame_rate": 30
},
"depends_on": [],
"name": "csicam_test",
Expand Down Expand Up @@ -137,8 +136,7 @@ On the new component panel, copy and paste the following attribute template into
{
"width_px": <int>,
"height_px": <int>,
"frame_rate": <int>,
"debug": <bool>
"frame_rate": <int>
}
```

Expand All @@ -158,7 +156,6 @@ The following attributes are available for `viam:camera:csi` cameras:
| `width_px` | int | Optional | Width of the image this camera captures in pixels. <br> Default: `1920` |
| `height_px` | int | Optional | Height of the image this camera captures in pixels. <br> Default: `1080` |
| `frame_rate` | int | Optional | The image capture frame rate this camera should use. <br> Default: `30` |
| `debug` | boolean | Optional | Whether or not you want debug input from this camera in your robot's logs. <br> Default: `false` |

Once configured, check the [Logs tab](https://docs.viam.com/program/debug/) of your robot in the Viam app to make sure your camera has connected and no errors are being raised.

Expand Down
99 changes: 37 additions & 62 deletions csi_camera.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,20 @@ using namespace viam::sdk;

CSICamera::CSICamera(const std::string name, const ProtoStruct& attrs) : Camera(std::move(name)) {
device = get_device_type();
std::cout << "Creating CSICamera with name: " << name << std::endl;
std::cout << "Device type: " << device.name << std::endl;
VIAM_SDK_LOG(debug) << "Creating CSICamera with name: " << name;
VIAM_SDK_LOG(debug) << "Device type: " << device.name;
init(attrs);
}

CSICamera::~CSICamera() {
std::cout << "Destroying CSICamera" << std::endl;
VIAM_SDK_LOG(debug) << "Destroying CSICamera";
stop_pipeline();
}

void CSICamera::init(const ProtoStruct& attrs) {
validate_attrs(attrs);
auto pipeline_args = create_pipeline();
if (debug) {
std::cout << "pipeline_args: " << pipeline_args << std::endl;
}
VIAM_SDK_LOG(debug) << "pipeline_args: " << pipeline_args;
init_csi(pipeline_args);
}

Expand All @@ -36,7 +34,6 @@ void CSICamera::validate_attrs(const ProtoStruct& attrs) {
set_attr<int>(attrs, "height_px", &CSICamera::height_px, DEFAULT_INPUT_HEIGHT);
set_attr<int>(attrs, "frame_rate", &CSICamera::frame_rate, DEFAULT_INPUT_FRAMERATE);
set_attr<std::string>(attrs, "video_path", &CSICamera::video_path, DEFAULT_INPUT_SENSOR);
set_attr<bool>(attrs, "debug", &CSICamera::debug, false);
}

template <typename T>
Expand All @@ -56,34 +53,27 @@ void CSICamera::set_attr(const ProtoStruct& attrs, const std::string& name, T CS
}

void CSICamera::reconfigure(const Dependencies& deps, const ResourceConfig& cfg) {
if (debug) {
std::cout << "Reconfiguring CSI Camera module" << std::endl;
}
VIAM_SDK_LOG(debug) << "Reconfiguring CSI Camera module";
stop_pipeline();
auto attrs = cfg.attributes();
init(attrs);
}

Camera::raw_image CSICamera::get_image(const std::string mime_type, const ProtoStruct& extra) {
if (debug) {
std::cout << "hit get_image. expecting mime_type " << mime_type << std::endl;
}
raw_image image;
image.mime_type = DEFAULT_OUTPUT_MIMETYPE;
image.bytes = get_csi_image();
if (image.bytes.empty()) {
std::cerr << "ERROR: no bytes retrieved" << std::endl;
VIAM_SDK_LOG(error) << "no bytes retrieved from get_csi_image";
throw Exception("no bytes retrieved from get_csi_image");
Copy link
Collaborator

@hexbabe hexbabe Aug 28, 2025

Choose a reason for hiding this comment

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

would std::runtime_error be more appropriate of an error to throw?

I'm gonna @ Lia @lia-viam

Copy link

@lia-viam lia-viam Aug 29, 2025

Choose a reason for hiding this comment

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

honestly not super important given the way we handle exceptions. note that viam::sdk::Exception is a public derived class of runtime_error, which matters in situations where you'd be interested in granular filtering on the exception type. You can skip the java stuff but see here: https://www.geeksforgeeks.org/cpp/catching-base-and-derived-classes-as-exceptions-in-cpp-and-java/

the distinction between Exception and runtime_error might matter if we were writing catch (const viam::sdk::Exception&) blocks and doing something meaningful with them, which we are not currently

the thing I would suggest though is rather than logging a message then putting it in an exception, in main.cpp you can wrap the call to serve in a try/catch block, like

try { my_mod.serve(); }
catch (const std::exception& e) {
VIAM_SDK_LOG(err) << e.what();
return EXIT_FAILURE;
}

Copy link
Collaborator Author

@seanavery seanavery Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@lia-viam Thanks for the explanation. I can wrap serve in a try catch, however, I thought that a throw here would get swallowed somewhere in the SDK/GRPC layer... Are you saying std::runtime_error will bring down the module server? This path gets hit behind GetImage/GetImages GRPC calls.

Copy link
Collaborator Author

@seanavery seanavery Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@lia-viam Should I be using GRPCException here instead? My intention is just to error out on the GRPC call.

Copy link
Collaborator Author

@seanavery seanavery Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like the orbbec module is hitting std::runtime_error (which would be similar to the viam::sdk::Exception use here) behind GRPC calls. See get_image error handling.

Copy link

@lia-viam lia-viam Sep 2, 2025

Choose a reason for hiding this comment

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

hey sean sorry I missed the ping. so my concern here was more just the duplication of throwing an error and having to log the same message. if you're implementing a method on a component then yes that gets handled by the grpc layer, although I think it should result in SDK or RDK (I forget which) logging the text of the exception you threw, so it's maybe not necessary to duplicate the logging.

for exception throws not in a component method implementation (or transitively called by one) then yes a try/catch in main would take care of that. this can indeed be more helpful than letting RDK take care of it, because often if you terminate with an uncaught exception you just get a message like "std::terminate called, uncaught exception of type runtime_error" without printing the message.

The main takeaway is it doesn't matter a ton whether you're throwing Exception or runtime_error, but no I wouldn't recommend throwing GRPCException

Copy link
Collaborator Author

@seanavery seanavery Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

if you're implementing a method on a component then yes that gets handled by the grpc layer

Makes sense, thanks.

for exception throws not in a component method implementation (or transitively called by one) then yes a try/catch in main

Ok, added try/catch block around the main function to handle this case better.

Also, removed dup logging, now allowing GRPC swallow to spit out the msg.

}

return image;
}

Camera::image_collection CSICamera::get_images() {
if (debug) {
std::cout << "[get_images] start\n";
}

ProtoStruct empty_extra;
// If image is not available, an exception will be thrown
raw_image image = get_image(DEFAULT_OUTPUT_MIMETYPE, empty_extra);
image.source_name = ""; // empty string because we don't have multiple sources to differentiate

Expand All @@ -94,30 +84,30 @@ Camera::image_collection CSICamera::get_images() {
auto nanoseconds = std::chrono::duration_cast<std::chrono::nanoseconds>(duration_since_epoch);
collection.metadata.captured_at = std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds>(nanoseconds);

if (debug) {
std::cout << "[get_images] end\n";
}
return collection;
}

ProtoStruct CSICamera::do_command(const ProtoStruct& command) {
std::cerr << "do_command not implemented" << std::endl;
VIAM_SDK_LOG(error) << "do_command not implemented";
return ProtoStruct{};
}

Camera::point_cloud CSICamera::get_point_cloud(const std::string mime_type, const ProtoStruct& extra) {
std::cerr << "get_point_cloud not implemented" << std::endl;
VIAM_SDK_LOG(error) << "get_point_cloud not implemented";
return point_cloud{};
}

std::vector<GeometryConfig> CSICamera::get_geometries(const ProtoStruct& extra) {
std::cerr << "get_geometries not implemented" << std::endl;
VIAM_SDK_LOG(error) << "get_geometries not implemented";
return std::vector<GeometryConfig>{};
}

Camera::properties CSICamera::get_properties() {
std::cerr << "get_properties not implemented" << std::endl;
return properties{};
Camera::properties p{};
p.supports_pcd = false;
p.intrinsic_parameters.width_px = width_px;
p.intrinsic_parameters.height_px = height_px;
return p;
}

void CSICamera::init_csi(const std::string pipeline_args) {
Expand All @@ -131,28 +121,23 @@ void CSICamera::init_csi(const std::string pipeline_args) {
std::cerr << "Failed to create the pipeline" << std::endl;
g_print("Error: %s\n", error->message);
g_error_free(error);
std::exit(EXIT_FAILURE);
}

// Print pipeline structure
if (debug) {
GST_DEBUG_BIN_TO_DOT_FILE(GST_BIN(pipeline), GST_DEBUG_GRAPH_SHOW_ALL, "pipeline_structure");
throw Exception("Failed to create the pipeline");
}

// Fetch the appsink element
appsink = gst_bin_get_by_name(GST_BIN(pipeline), "appsink0");
if (!appsink) {
std::cerr << "Failed to get the appsink element" << std::endl;
gst_object_unref(pipeline);
std::exit(EXIT_FAILURE);
throw Exception("Failed to get the appsink element");
}

// Start the pipeline
if (gst_element_set_state(pipeline, GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE) {
std::cerr << "Failed to start the pipeline" << std::endl;
gst_object_unref(appsink);
gst_object_unref(pipeline);
std::exit(EXIT_FAILURE);
throw Exception("Failed to start the pipeline");
}

// Handle async pipeline creation
Expand All @@ -164,7 +149,7 @@ void CSICamera::init_csi(const std::string pipeline_args) {
std::cerr << "Failed to get the bus for the pipeline" << std::endl;
gst_object_unref(appsink);
gst_object_unref(pipeline);
std::exit(EXIT_FAILURE);
throw Exception("Failed to get the bus for the pipeline");
}
}

Expand All @@ -184,7 +169,7 @@ void CSICamera::wait_pipeline() {

if (elapsed_time >= timeout_microseconds) {
std::cerr << "Timeout: GST pipeline state change did not complete within timeout limit" << std::endl;
std::exit(EXIT_FAILURE);
throw Exception("Timeout: GST pipeline state change did not complete within timeout limit");
}

// Wait for a short duration to avoid busy waiting
Expand All @@ -195,32 +180,30 @@ void CSICamera::wait_pipeline() {
std::cout << "GST pipeline state change success" << std::endl;
} else if (ret == GST_STATE_CHANGE_FAILURE) {
std::cerr << "GST pipeline failed to change state" << std::endl;
std::exit(EXIT_FAILURE);
} else if (ret = GST_STATE_CHANGE_NO_PREROLL) {
throw Exception("GST pipeline failed to change state");
} else if (ret == GST_STATE_CHANGE_NO_PREROLL) {
std::cout << "GST pipeline changed but not enough data for preroll" << std::endl;
} else {
std::cerr << "GST pipeline failed to change state" << std::endl;
std::exit(EXIT_FAILURE);
throw Exception("GST pipeline failed to change state");
}
}

void CSICamera::stop_pipeline() {
if (debug) {
std::cout << "Stopping GST pipeline" << std::endl;
}
VIAM_SDK_LOG(debug) << "Stopping GST pipeline";

// Check if pipeline is defined
if (pipeline == nullptr) {
std::cout << "Pipeline is not defined" << std::endl;
VIAM_SDK_LOG(error) << "Pipeline is not defined";
return;
}

// Stop the pipeline
if (gst_element_set_state(pipeline, GST_STATE_NULL) == GST_STATE_CHANGE_FAILURE) {
std::cerr << "Failed to stop the pipeline" << std::endl;
VIAM_SDK_LOG(error) << "Failed to stop the pipeline";
gst_object_unref(appsink);
gst_object_unref(pipeline);
std::exit(EXIT_FAILURE);
throw Exception("Failed to stop the pipeline");
}

// Wait for async state change
Expand All @@ -242,29 +225,25 @@ void CSICamera::catch_pipeline() {
switch (GST_MESSAGE_TYPE(msg)) {
case GST_MESSAGE_ERROR:
gst_message_parse_error(msg, &error, &debugInfo);
std::cerr << "Error: " << error->message << std::endl;
std::cerr << "Debug Info: " << debugInfo << std::endl;
VIAM_SDK_LOG(error) << "Error: " << error->message;
VIAM_SDK_LOG(error) << "Debug Info: " << debugInfo;
stop_pipeline();
std::exit(EXIT_FAILURE);
throw Exception("Failed to stop the pipeline");
break;
case GST_MESSAGE_EOS:
std::cout << "End of stream received" << std::endl;
VIAM_SDK_LOG(info) << "End of stream received";
stop_pipeline();
std::exit(EXIT_SUCCESS);
throw Exception("Failed to stop the pipeline");
break;
case GST_MESSAGE_WARNING:
gst_message_parse_warning(msg, &error, &debugInfo);
if (debug) {
std::cout << "Warning: " << error->message << std::endl;
std::cout << "Debug Info: " << debugInfo << std::endl;
}
VIAM_SDK_LOG(warn) << "Warning: " << error->message;
VIAM_SDK_LOG(warn) << "Debug Info: " << debugInfo;
break;
case GST_MESSAGE_INFO:
gst_message_parse_info(msg, &error, &debugInfo);
if (debug) {
std::cout << "Info: " << error->message << std::endl;
std::cout << "Debug Info: " << debugInfo << std::endl;
}
VIAM_SDK_LOG(info) << "Info: " << error->message;
VIAM_SDK_LOG(info) << "Debug Info: " << debugInfo;
break;
default:
// Ignore other message types
Expand Down Expand Up @@ -355,10 +334,6 @@ int CSICamera::get_frame_rate() const {
return frame_rate;
}

bool CSICamera::is_debug() const {
return debug;
}

GstElement* CSICamera::get_appsink() const {
return appsink;
}
Expand Down
4 changes: 2 additions & 2 deletions csi_camera.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
#include <viam/sdk/config/resource.hpp>
#include <viam/sdk/resource/reconfigurable.hpp>
#include <viam/sdk/common/proto_value.hpp>
#include <viam/sdk/log/logging.hpp>
#include <viam/sdk/common/exception.hpp>

#include "utils.h"

Expand All @@ -23,7 +25,6 @@ class CSICamera : public Camera, public Reconfigurable {
device_type device;

// Camera
bool debug;
int width_px = 0;
int height_px = 0;
int frame_rate = 0;
Expand Down Expand Up @@ -71,7 +72,6 @@ class CSICamera : public Camera, public Reconfigurable {

// Getters
std::string get_name() const;
bool is_debug() const;
Copy link
Collaborator Author

@seanavery seanavery Aug 28, 2025

Choose a reason for hiding this comment

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

Removing debug attribute

int get_width_px() const;
int get_height_px() const;
int get_frame_rate() const;
Expand Down
17 changes: 8 additions & 9 deletions main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <viam/sdk/resource/resource.hpp>
#include <viam/sdk/registry/registry.hpp>
#include <viam/sdk/components/camera.hpp>
#include <viam/sdk/log/logging.hpp>

#include "constraints.h"
#include "csi_camera.h"
Expand All @@ -15,22 +16,20 @@
using namespace viam::sdk;

int main(int argc, char *argv[]) {
std::cout << "### STARTING VIAM CSI CAMERA MODULE" << std::endl;
// Every Viam C++ SDK program must have one and only one Instance object
// which is created before any other C++ SDK objects and stays alive until
// all Viam C++ SDK objects are destroyed.
Instance inst;
Copy link
Collaborator Author

@seanavery seanavery Aug 28, 2025

Choose a reason for hiding this comment

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

Need to move Instance before any logger use


VIAM_SDK_LOG(info) << "### STARTING VIAM CSI CAMERA MODULE";

// Gstreamer initialization
gst_init(&argc, &argv);

// Device type and params
auto device = get_device_type();
auto api_params = get_api_params(device);

std::cout << "Device type: " << device.name << std::endl;
std::cout << "API Namespace: " << api_params.api_namespace << std::endl;

// Every Viam C++ SDK program must have one and only one Instance object
// which is created before any other C++ SDK objects and stays alive until
// all Viam C++ SDK objects are destroyed.
Instance inst;
VIAM_SDK_LOG(info) << "Device type: " << device.name;

auto module_registration = std::make_shared<ModelRegistration>(
API::get<Camera>(),
Expand Down
6 changes: 0 additions & 6 deletions test/test_csi_camera.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,8 @@ TEST(CSICamera, CreateDefault) {

ProtoStruct attrs = std::unordered_map<std::string, ProtoValue>();

attrs.insert(std::make_pair("debug", ProtoValue(true)));

CSICamera camera("test", attrs);

EXPECT_EQ(camera.is_debug(), true);
EXPECT_EQ(camera.get_width_px(), DEFAULT_INPUT_WIDTH);
EXPECT_EQ(camera.get_height_px(), DEFAULT_INPUT_HEIGHT);
EXPECT_EQ(camera.get_frame_rate(), DEFAULT_INPUT_FRAMERATE);
Expand All @@ -32,15 +29,13 @@ TEST(CSICamera, CreateCustom) {
gst_init(nullptr, nullptr);

ProtoStruct attrs = std::unordered_map<std::string, ProtoValue>();
attrs.insert(std::make_pair("debug", ProtoValue(true)));
attrs.insert(std::make_pair("width_px", ProtoValue(640)));
attrs.insert(std::make_pair("height_px", ProtoValue(480)));
attrs.insert(std::make_pair("frame_rate", ProtoValue(60)));
attrs.insert(std::make_pair("video_path", ProtoValue(std::string("1"))));

CSICamera camera("test", attrs);

EXPECT_EQ(camera.is_debug(), true);
EXPECT_EQ(camera.get_width_px(), 640);
EXPECT_EQ(camera.get_height_px(), 480);
EXPECT_EQ(camera.get_frame_rate(), 60);
Expand All @@ -54,7 +49,6 @@ TEST(CSICamera, StartStopPipeline) {
gst_init(nullptr, nullptr);

ProtoStruct attrs = std::unordered_map<std::string, ProtoValue>();
attrs.insert(std::make_pair("debug", ProtoValue(true)));

CSICamera camera("test", attrs);

Expand Down