fix: address PR review findings across atracsys pipeline

- Move parse_arguments inside try/catch in main to prevent uncaught
  exceptions from std::terminate on bad CLI input
- Use size-tracked std::string construction for ftkBuffer error messages
  in atracsys_device.hpp to avoid reading past buffer length
- Guard static cache initialization in geometry_helper.cpp with
  std::once_flag/std::call_once for thread safety
- Fix ftkEnumerateDevices error check from '> FTK_OK' to '!= FTK_OK'
  in helpers.cpp to correctly catch all non-OK returns
- Qualify optionEnumerator as atracsys::sdk::optionEnumerator and move
  anonymous namespace helpers to atracsys::sdk::detail in s3dk_wrapper.hpp
- Always throw on ftkSetInt32 failure in set_device_option regardless of
  required flag, include status code in error message
- Add explicit #include <stdexcept> and <string> to sdk_wrapper.hpp
- Add defensive HOLOHUB_DATA_DIR check in CMakeLists.txt
- Move AsynchronousCondition discovery to start of start() in
  master_source_op.cpp to avoid resource leaks on missing condition
- Expose min_z/max_z/max_x/max_y as operator parameters in
  PointCloudFilterOp instead of hardcoded literals
- Add proper Expected<T> validation before .value() calls in
  point_cloud_filter_op.cpp for clearer error diagnostics
- Qualify std::ios, std::streamsize, std::cerr in geometry_helper.cpp

Signed-off-by: Artrit Telaku <artrit@waylandio.com>
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>

Author: artrittelaku-wayland

applications/atracsys_visualizer/CMakeLists.txt

@@ -44,6 +44,10 @@ else()
   endif()
 endif()
 
+if(NOT DEFINED HOLOHUB_DATA_DIR OR HOLOHUB_DATA_DIR STREQUAL "")
+  message(FATAL_ERROR "HOLOHUB_DATA_DIR must be set when building with datasets")
+endif()
+
 if(HOLOHUB_DOWNLOAD_DATASETS AND NOT EXISTS "${HOLOHUB_DATA_DIR}/atracsys_visualizer/ir_base.gxf_entities")
   set(ATRACSYS_VISUALIZER_DATASET_URL
       "https://github.com/waylandio/holohub/releases/download/dataset-v1.0/atracsys_visualizer.zip")

applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp

@@ -246,24 +246,25 @@ class AtracsysVisualizerApp : public holoscan::Application {
 };
 
 int main(int argc, char** argv) {
-  CommandLineOptions options;
-  if (!parse_arguments(argc, argv, options)) {
-    return 0;
-  }
+  try {
+    CommandLineOptions options;
+    if (!parse_arguments(argc, argv, options)) {
+      return 0;
+    }
 
-  if (options.config_path.empty()) {
-    options.config_path = "atracsys_visualizer.yaml";
-  }
+    if (options.config_path.empty()) {
+      options.config_path = "atracsys_visualizer.yaml";
+    }
 
-  if (options.data_path.empty()) {
-    if (const char* env_path = std::getenv("HOLOSCAN_INPUT_PATH")) {
-      options.data_path = env_path;
-    } else {
-      options.data_path = "data/atracsys_visualizer";
+    if (options.data_path.empty()) {
+      if (const char* env_path = std::getenv("HOLOSCAN_INPUT_PATH")) {
+        options.data_path =
+            std::string(env_path) + "/atracsys_visualizer";
+      } else {
+        options.data_path = "data/atracsys_visualizer";
+      }
     }
-  }
 
-  try {
     auto app = holoscan::make_application<AtracsysVisualizerApp>();
     app->set_source(options.source);
     app->set_data_path(options.data_path);

operators/atracsys_camera/master_source_op.cpp

@@ -183,44 +183,6 @@ void AtracsysMasterSourceOp::setup(holoscan::OperatorSpec& spec) {
 
 void AtracsysMasterSourceOp::start() {
   reset_state();
-  frame_timeout_count_ = 0;
-  first_frame_logged_ = false;
-  first_structured_cloud_logged_ = false;
-
-  auto& dev = AtracsysDevice::instance();
-  dev.init();
-  device_sn_ = dev.serial();
-
-  active_scheduler_mode_ = configured_scheduler_mode();
-  active_hw_mode_ = configured_initial_hw_mode();
-  frame_timeout_ms_ = 500;
-
-  if (!s3dk_) {
-    s3dk_ = std::make_unique<RealS3DKWrapper>();
-  }
-  if (!s3dk_->initializeDeviceHelper(&device_sn_, dev.lib(), &image_type_)) {
-    throw std::runtime_error("AtracsysMasterSourceOp: initializeDeviceHelper failed");
-  }
-
-  load_geometries();
-  configure_camera();
-
-  frame_ = sdk_.createFrame();
-  if (!frame_) {
-    throw std::runtime_error("AtracsysMasterSourceOp: ftkCreateFrame failed");
-  }
-  configure_frame();
-
-  stereo_params_ = s3dk_->createStereoParameters();
-  engine_ = s3dk_->createDefaultEngine();
-  gpu_frame_ = s3dk_->createGpu3DFrame(image_type_);
-  if (gpu_frame_) {
-    const int scale = validated_scale_factor(scale_factor_.get());
-    gpu_frame_->_scale = scale;
-  }
-  if (stereo_params_) {
-    stereo_params_->scale = validated_scale_factor(scale_factor_.get());
-  }
 
   for (auto& [name, cond] : conditions()) {
     if (auto async_cond = std::dynamic_pointer_cast<holoscan::AsynchronousCondition>(cond)) {
@@ -234,9 +196,58 @@ void AtracsysMasterSourceOp::start() {
         "AsynchronousCondition.");
   }
 
-  is_running_ = true;
-  async_cond_->event_state(holoscan::AsynchronousEventState::EVENT_WAITING);
-  capture_thread_ = std::thread(&AtracsysMasterSourceOp::capture_loop, this);
+  frame_timeout_count_ = 0;
+  first_frame_logged_ = false;
+  first_structured_cloud_logged_ = false;
+
+  auto& dev = AtracsysDevice::instance();
+  dev.init();
+
+  try {
+    device_sn_ = dev.serial();
+
+    active_scheduler_mode_ = configured_scheduler_mode();
+    active_hw_mode_ = configured_initial_hw_mode();
+    frame_timeout_ms_ = 500;
+
+    if (!s3dk_) {
+      s3dk_ = std::make_unique<RealS3DKWrapper>();
+    }
+    if (!s3dk_->initializeDeviceHelper(&device_sn_, dev.lib(), &image_type_)) {
+      throw std::runtime_error(
+          "AtracsysMasterSourceOp: initializeDeviceHelper failed");
+    }
+
+    load_geometries();
+    configure_camera();
+
+    frame_ = sdk_.createFrame();
+    if (!frame_) {
+      throw std::runtime_error(
+          "AtracsysMasterSourceOp: ftkCreateFrame failed");
+    }
+    configure_frame();
+
+    stereo_params_ = s3dk_->createStereoParameters();
+    engine_ = s3dk_->createDefaultEngine();
+    gpu_frame_ = s3dk_->createGpu3DFrame(image_type_);
+    if (gpu_frame_) {
+      const int scale = validated_scale_factor(scale_factor_.get());
+      gpu_frame_->_scale = scale;
+    }
+    if (stereo_params_) {
+      stereo_params_->scale = validated_scale_factor(scale_factor_.get());
+    }
+
+    is_running_ = true;
+    async_cond_->event_state(
+        holoscan::AsynchronousEventState::EVENT_WAITING);
+    capture_thread_ =
+        std::thread(&AtracsysMasterSourceOp::capture_loop, this);
+  } catch (...) {
+    AtracsysDevice::instance().shutdown();
+    throw;
+  }
 }
 
 void AtracsysMasterSourceOp::stop() {
@@ -252,6 +263,8 @@ void AtracsysMasterSourceOp::stop() {
   stereo_params_ = nullptr;
   s3dk_.reset();
 
+  AtracsysDevice::instance().shutdown();
+
   holoscan::Operator::stop();
 }
 

operators/atracsys_camera/master_source_op.hpp

@@ -36,6 +36,7 @@ typedef struct CUstream_st* cudaStream_t;
 #include "hardware_mode_command.hpp"
 #include "sdk/atracsys_device.hpp"
 #include "sdk/s3dk_interface.hpp"
+#include "sdk/s3dk_wrapper.hpp"
 #include "sdk/sdk_wrapper.hpp"
 
 namespace holoscan::ops {

operators/atracsys_camera/point_cloud_filter_op.cpp

@@ -53,6 +53,11 @@ void PointCloudFilterOp::setup(holoscan::OperatorSpec& spec) {
              "cuda_stream_pool",
              "CudaStreamPool",
              "CUDA stream pool used for async GPU operations");
+
+  spec.param(min_z_, "min_z", "Min Z", "Minimum Z coordinate for points", 0.0f);
+  spec.param(max_z_, "max_z", "Max Z", "Maximum Z coordinate for points", 5000.0f);
+  spec.param(max_x_, "max_x", "Max X", "Maximum absolute X coordinate for points", 5000.0f);
+  spec.param(max_y_, "max_y", "Max Y", "Maximum absolute Y coordinate for points", 5000.0f);
 }
 
 void PointCloudFilterOp::start() {
@@ -81,9 +86,26 @@ void PointCloudFilterOp::ensure_structured_output_entities(
   const nvidia::gxf::Shape shape{static_cast<int32_t>(point_count), 3};
   auto alloc = nvidia::gxf::Handle<nvidia::gxf::Allocator>::Create(
       context.context(), structured_allocator_.get()->gxf_cid());
+  if (!alloc) {
+      throw std::runtime_error("PointCloudFilterOp: failed to create allocator handle");
+  }
+
   auto msg = nvidia::gxf::Entity::New(context.context());
+  if (!msg) {
+      throw std::runtime_error("PointCloudFilterOp: failed to create new entity");
+  }
+
   auto tensor = msg.value().add<nvidia::gxf::Tensor>(kStructuredTensorName);
-  tensor.value()->reshape<float>(shape, nvidia::gxf::MemoryStorageType::kDevice, alloc.value());
+  if (!tensor) {
+      throw std::runtime_error("PointCloudFilterOp: failed to add tensor to entity");
+  }
+
+  auto reshape_result = tensor.value()->reshape<float>(
+      shape, nvidia::gxf::MemoryStorageType::kDevice, alloc.value());
+  if (!reshape_result) {
+      throw std::runtime_error("PointCloudFilterOp: failed to reshape tensor");
+  }
+
   structured_output_entities_[structured_output_entity_index_].emplace(std::move(msg.value()));
 }
 
@@ -140,10 +162,10 @@ void PointCloudFilterOp::compute(holoscan::InputContext& op_input,
                             height,
                             step,
                             h_Q,
-                            0.0f,
-                            5000.0f,
-                            5000.0f,
-                            5000.0f,
+                            min_z_.get(),
+                            max_z_.get(),
+                            max_x_.get(),
+                            max_y_.get(),
                             d_out_points,
                             cuda_stream);
   check_cuda(cudaGetLastError(), "Failed to launch point cloud filter kernel");

operators/atracsys_camera/point_cloud_filter_op.hpp

@@ -47,6 +47,10 @@ class __attribute__((visibility("default"))) PointCloudFilterOp : public holosca
 
   holoscan::Parameter<std::shared_ptr<holoscan::Allocator>> structured_allocator_;
   holoscan::Parameter<std::shared_ptr<holoscan::CudaStreamPool>> cuda_stream_pool_;
+  holoscan::Parameter<float> min_z_;
+  holoscan::Parameter<float> max_z_;
+  holoscan::Parameter<float> max_x_;
+  holoscan::Parameter<float> max_y_;
 
   static constexpr size_t kEntityRingSize = 4;
   std::array<std::optional<holoscan::gxf::Entity>, kEntityRingSize> structured_output_entities_;

operators/atracsys_camera/sdk/atracsys_device.hpp

@@ -40,7 +40,11 @@ class AtracsysDevice {
 
     ftkLibrary new_lib = ftkInitExt(nullptr, &buffer_);
     if (!new_lib) {
-      throw std::runtime_error(buffer_.data ? buffer_.data : "ftkInitExt failed");
+      if (buffer_.data && buffer_.size > 0) {
+        throw std::runtime_error(
+            std::string(reinterpret_cast<const char*>(buffer_.data), buffer_.size));
+      }
+      throw std::runtime_error("ftkInitExt failed");
     }
 
     try {

operators/atracsys_camera/sdk/geometry_helper.cpp

@@ -19,18 +19,18 @@
 
 #include <ftkInterface.h>
 
+#include <cstdlib>
 #include <cstring>
 #include <fstream>
 #include <iostream>
 #include <limits>
+#include <mutex>
 #include <string>
 
-using namespace std;
-
 static void getDataDirOptionId(uint64_t sn, void* user, ftkOptionsInfo* oi);
 
-int loadRigidBody(ftkLibrary lib, const string& fileName, ftkRigidBody& geometry) {
-  string fullFileName;
+int loadRigidBody(ftkLibrary lib, const std::string& fileName, ftkRigidBody& geometry) {
+  std::string fullFileName;
   bool fromDataDir = false;
   if (!getFullFilePath(lib, fileName, fullFileName, &fromDataDir)) {
     return 2;
@@ -48,37 +48,41 @@ int loadRigidBody(ftkLibrary lib, const string& fileName, ftkRigidBody& geometry
   return fromDataDir ? 1 : 0;
 }
 
-bool getFullFilePath(ftkLibrary lib, const string& fileName, string& fullFilePath,
+bool getFullFilePath(ftkLibrary lib, const std::string& fileName, std::string& fullFilePath,
                      bool* fromSystem) {
   static uint32_t FTK_OPT_DATA_DIR = 0u;
-  static string OPT_DIR;
+  static std::string OPT_DIR;
+  static std::once_flag flag;
 
-  if (FTK_OPT_DATA_DIR == 0u) {
+  std::call_once(flag, [&]() {
     if (ftkEnumerateOptions(lib, 0uLL, getDataDirOptionId, &FTK_OPT_DATA_DIR) !=
         ftkError::FTK_WAR_OPT_GLOBAL_ONLY) {
-      cerr << "Could not get the data directory option ID\n";
-      return false;
+      std::cerr << "Could not get the data directory option ID\n";
+      FTK_OPT_DATA_DIR = 0u;
+      return;
     }
-    if (FTK_OPT_DATA_DIR == 0u) {
-      cerr << "Could not get the data directory option ID\n";
-      return false;
+    if (FTK_OPT_DATA_DIR != 0u) {
+      ftkBuffer buffer{};
+      if (ftkGetData(lib, 0uLL, FTK_OPT_DATA_DIR, &buffer) == ftkError::FTK_OK &&
+          buffer.size >= 1u) {
+        OPT_DIR.assign(reinterpret_cast<const char*>(buffer.data), buffer.size);
+        if (!OPT_DIR.empty() && OPT_DIR.back() == '\0') {
+          OPT_DIR.pop_back();
+        }
+      }
     }
-  }
+  });
 
+  if (FTK_OPT_DATA_DIR == 0u) {
+    std::cerr << "Could not get the data directory option ID\n";
+    return false;
+  }
   if (OPT_DIR.empty()) {
-    ftkBuffer buffer{};
-    if (ftkGetData(lib, 0uLL, FTK_OPT_DATA_DIR, &buffer) != ftkError::FTK_OK ||
-        buffer.size < 1u) {
-      return false;
-    }
-    OPT_DIR.assign(reinterpret_cast<const char*>(buffer.data), buffer.size);
-    if (!OPT_DIR.empty() && OPT_DIR.back() == '\0') {
-      OPT_DIR.pop_back();
-    }
+    return false;
   }
 
-  const string candidate = OPT_DIR + "/" + fileName;
-  ifstream system_input{candidate};
+  const std::string candidate = OPT_DIR + "/" + fileName;
+  std::ifstream system_input{candidate};
   if (system_input.is_open()) {
     fullFilePath = candidate;
     if (fromSystem != nullptr) *fromSystem = true;
@@ -88,31 +92,33 @@ bool getFullFilePath(ftkLibrary lib, const string& fileName, string& fullFilePat
   return false;
 }
 
-bool loadFileInBuffer(const string& fullFilePath, ftkBuffer& buffer) {
-  ifstream input(fullFilePath, ios::binary | ios::ate);
+bool loadFileInBuffer(const std::string& fullFilePath, ftkBuffer& buffer) {
+  std::ifstream input(fullFilePath, std::ios::binary | std::ios::ate);
   if (!input.is_open()) {
-    cerr << "Could not open file '" << fullFilePath << "'\n";
+    std::cerr << "Could not open file '" << fullFilePath << "'\n";
     return false;
   }
 
   buffer.reset();
-  streampos pos = input.tellg();
+  std::streampos pos = input.tellg();
   if (pos < 0 || pos > std::numeric_limits<uint32_t>::max()) {
-    cerr << "File too large for buffer\n";
+    std::cerr << "File too large for buffer\n";
     return false;
   }
 
-  if (static_cast<size_t>(pos) > buffer.capacity) {
-    if (buffer.resize(static_cast<size_t>(pos)) != ftkError::FTK_OK) {
-      cerr << "Failed to resize ftkBuffer for file\n";
-      return false;
-    }
+  const auto file_size = static_cast<size_t>(pos);
+  auto* raw = reinterpret_cast<char*>(std::malloc(file_size));
+  if (!raw) {
+    std::cerr << "Failed to allocate buffer for file\n";
+    return false;
   }
+  buffer.data = raw;
+  buffer.size = 0u;
 
-  input.seekg(0u, ios::beg);
-  input.read(buffer.data, static_cast<streamsize>(pos));
+  input.seekg(0u, std::ios::beg);
+  input.read(buffer.data, static_cast<std::streamsize>(pos));
   if (input.fail()) {
-    cerr << "Could not read file '" << fullFilePath << "'\n";
+    std::cerr << "Could not read file '" << fullFilePath << "'\n";
     return false;
   }
   buffer.size = static_cast<uint32_t>(pos);