anqixu / ueye_cam

A ROS nodelet and node that wraps the driver API for UEye cameras by IDS Imaging Development Systems GMBH.
Other
60 stars 102 forks source link

Added check if default file ini exists at all #98

Closed nullket closed 2 years ago

nullket commented 3 years ago

Starting the development on this project I made the mistake to forget making a branch for this very first feature I implemented. I had to do a rebase last night in order to clean my forked master. This caused https://github.com/anqixu/ueye_cam/pull/88 to automatically close. This is the replacement PR.

What it does:

Before If no .ini file is stated in the launch file the driver tries to load a default file resulting in an error if this file does not exist

Could not load [camera]'s sensor parameters file /home/user/.ros/camera_conf/camera.ini

With Patch I do not use .ini files at all. The driver will now check if the default (fallback) .ini exists at all before trying to load it. This avoids "errors" that are none by design.

stonier commented 3 years ago

+1 for this in the ROS1 version. I unwittingly implemented the same in the ROS2 port (#110).

  // Camera Parameterisation
  CameraParameters default_camera_parameters, camera_parameters;
  if (std::ifstream(node_parameters_.ids_configuration_filename.c_str()).good()) {
    // If using an IDS configuration, load that and read it back first before engaging further
    try {
      loadCamConfig(node_parameters_.ids_configuration_filename);
    } catch (const std::runtime_error& e) {
      std::ostringstream ostream;
      ostream << "failed to load IDS configuration on camera '" << node_parameters_.camera_name << "', aborting.";
      log_nested_exception(FATAL, this->get_logger(), ostream.str());
      rclcpp::shutdown();
      return;
    }
    // Pull back the configuration, use it as defaults for the ros parameterisation
    default_camera_parameters = camera_parameters_;
  }
nullket commented 3 years ago

+1 for this in the ROS1 version. I unwittingly implemented the same in the ROS2 port (#110).

The two branches will diverge, but that is totally okay as the ROS1 Version will die out one day. But should we try to agree on one code base for this matter or at least behave/warn in the same way? Whats you opinion?

stonier commented 2 years ago

+1 for common behaviour

I personally feel like we've two kinds of users.

Why do you use a try/catch? Isn't the ifstream good() function already enough with checking if the files existence?

My loadCamConfig() method in the driver is checking for a few additional things - camera not connected, file loads correctly, sync works.

Also, the exception approach lets the underlying c++ library leave the details of handling the error to the user of the library - in this case the ros node which does some ros style logging before shutting down. You could get the same effect with return codes instead of exceptions, so the exception is not special in that regard.

nullket commented 2 years ago

what do you prefer?