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 <[email protected]>
Signed-off-by: artrittelaku-wayland <[email protected]>

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,24 @@ 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 = env_path;
+      } 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,6 +183,19 @@ void AtracsysMasterSourceOp::setup(holoscan::OperatorSpec& spec) {
 
 void AtracsysMasterSourceOp::start() {
   reset_state();
+
+  for (auto& [name, cond] : conditions()) {
+    if (auto async_cond = std::dynamic_pointer_cast<holoscan::AsynchronousCondition>(cond)) {
+      async_cond_ = async_cond;
+      break;
+    }
+  }
+  if (!async_cond_) {
+    throw std::runtime_error(
+        "AtracsysMasterSourceOp: AsyncCondition not found. The app must provide an "
+        "AsynchronousCondition.");
+  }
+
   frame_timeout_count_ = 0;
   first_frame_logged_ = false;
   first_structured_cloud_logged_ = false;
@@ -222,18 +235,6 @@ void AtracsysMasterSourceOp::start() {
     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)) {
-      async_cond_ = async_cond;
-      break;
-    }
-  }
-  if (!async_cond_) {
-    throw std::runtime_error(
-        "AtracsysMasterSourceOp: AsyncCondition not found. The app must provide an "
-        "AsynchronousCondition.");
-  }
-
   is_running_ = true;
   async_cond_->event_state(holoscan::AsynchronousEventState::EVENT_WAITING);
   capture_thread_ = std::thread(&AtracsysMasterSourceOp::capture_loop, this);

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

@@ -23,14 +23,13 @@
 #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 +47,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 +91,31 @@ 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";
+      std::cerr << "Failed to resize ftkBuffer for file\n";
       return false;
     }
   }
 
-  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);

operators/atracsys_camera/sdk/helpers.cpp

@@ -56,7 +56,7 @@ DeviceData retrieveLastDevice(ftkLibrary lib, bool allowSimulator, bool quiet,
     err = ftkEnumerateDevices(lib, fusionTrackEnumerator, &device);
   }
 
-  if (err > ftkError::FTK_OK) {
+  if (err != ftkError::FTK_OK) {
     throw std::runtime_error("ftkEnumerateDevices returned an error");
   }
 

operators/atracsys_camera/sdk/s3dk_wrapper.hpp

@@ -26,11 +26,11 @@
 #include "helpers.hpp"
 #include "s3dk_interface.hpp"
 
-namespace {
+namespace atracsys::sdk::detail {
 
 inline std::map<std::string, uint32_t> enumerate_sdk_options(ftkLibrary lib, uint64_t sn) {
   std::map<std::string, uint32_t> options;
-  if (ftkEnumerateOptions(lib, sn, optionEnumerator, &options) != ftkError::FTK_OK ||
+  if (ftkEnumerateOptions(lib, sn, atracsys::sdk::optionEnumerator, &options) != ftkError::FTK_OK ||
       options.empty()) {
     throw std::runtime_error("Cannot retrieve Atracsys SDK options.");
   }
@@ -53,29 +53,30 @@ inline std::string device_type_string(ftkLibrary lib, uint64_t sn,
 
 inline void set_device_option(ftkLibrary lib, uint64_t sn,
                               const std::map<std::string, uint32_t>& options, const char* name,
-                              int32_t value, bool required = true) {
+                              int32_t value) {
   auto it = options.find(name);
   if (it == options.end()) {
-    if (required)
-      throw std::runtime_error(std::string("Missing Atracsys option: ") + name);
     return;
   }
 
-  if (ftkSetInt32(lib, sn, it->second, value) != ftkError::FTK_OK && required) {
-    throw std::runtime_error(std::string("Failed to set Atracsys option: ") + name);
+  const ftkError status = ftkSetInt32(lib, sn, it->second, value);
+  if (status != ftkError::FTK_OK) {
+    throw std::runtime_error(
+        std::string("Failed to set Atracsys option: ") + name +
+        ", status: " + std::to_string(status));
   }
 }
 
 inline void configure_device_defaults(ftkLibrary lib, uint64_t sn,
                                       const std::map<std::string, uint32_t>& options) {
   set_device_option(lib, sn, options, "Enable embedded processing", 1);
   set_device_option(lib, sn, options, "Enable images sending", 1);
-  set_device_option(lib, sn, options, "Calibration export", 1, false);
-  set_device_option(lib, sn, options, "Embedded Symmetrise coordinates", 0, false);
-  set_device_option(lib, sn, options, "Enable 16 bits pictures", 0, false);
+  set_device_option(lib, sn, options, "Calibration export", 1);
+  set_device_option(lib, sn, options, "Embedded Symmetrise coordinates", 0);
+  set_device_option(lib, sn, options, "Enable 16 bits pictures", 0);
 }
 
-}  // namespace
+}  // namespace atracsys::sdk::detail
 
 class RealS3DKWrapper : public atracsys::IS3DKInterface {
  public:
@@ -86,14 +87,16 @@ class RealS3DKWrapper : public atracsys::IS3DKInterface {
     if (!device_sn || !lib || !image_type) {
       throw std::runtime_error("initializeDeviceHelper: invalid arguments");
     }
-    auto options = enumerate_sdk_options(lib, *device_sn);
-    const std::string device_type = device_type_string(lib, *device_sn, options);
+    auto options =
+        atracsys::sdk::detail::enumerate_sdk_options(lib, *device_sn);
+    const std::string device_type =
+        atracsys::sdk::detail::device_type_string(lib, *device_sn, options);
     bool valid = false;
     *image_type = convert_string_image_type(device_type, &valid);
     if (!valid) {
       throw std::runtime_error("Unsupported Atracsys device type for S3DK: " + device_type);
     }
-    configure_device_defaults(lib, *device_sn, options);
+    atracsys::sdk::detail::configure_device_defaults(lib, *device_sn, options);
     return true;
   }
 

operators/atracsys_camera/sdk/sdk_wrapper.hpp

@@ -17,6 +17,9 @@
 
 #pragma once
 
+#include <stdexcept>
+#include <string>
+
 #include "sdk_interface.hpp"
 
 class RealSDKWrapper : public atracsys::ISDKInterface {