neufieldrobotics / spinnaker_sdk_camera_driver

Point Grey (FLIR) Spinnaker based camera driver (Blackfly S etc.)
MIT License
125 stars 90 forks source link

General Clean-up and Fix for #7 #14

Closed JWhitleyWork closed 5 years ago

JWhitleyWork commented 5 years ago

This PR generally cleans up package.xml and brings it up to format 2 (defined in http://wiki.ros.org/catkin/package.xml#Format_2_.28Recommended.29); cleans up CMakeLists.txt and applies best-practices produced by the ROS community with regard to ordering and content of add_dependencies, target_link_libraries, and add_compile_options; and renames the message file per ROS naming conventions. Also fixes #7.

JWhitleyWork commented 5 years ago

Now also includes correct CMake dependencies for OpenMP, libunwind, OpenCV, and others. Includes package.xml dependency on libgomp1 - will add libunwind-dev once ros/rosdistro#19615 is approved.

ghost commented 5 years ago

When doing a fresh catkin make, I see the error

CMake Error at spinnaker_sdk_camera_driver/CMakeLists.txt:42 (find_package):
  By not providing "FindLibUnwind.cmake" in CMAKE_MODULE_PATH this project
  has asked CMake to find a package configuration file provided by
  "LibUnwind", but CMake did not find one.

  Could not find a package configuration file provided by "LibUnwind" with
  any of the following names:

    LibUnwindConfig.cmake
    libunwind-config.cmake

  Add the installation prefix of "LibUnwind" to CMAKE_PREFIX_PATH or set
  "LibUnwind_DIR" to a directory containing one of the above files.  If
  "LibUnwind" provides a separate development package or SDK, be sure it has
  been installed.

-- Configuring incomplete, errors occurred!
See also "/home/vik748/apps/spinnaker_ws/build/CMakeFiles/CMakeOutput.log".
See also "/home/vik748/apps/spinnaker_ws/build/CMakeFiles/CMakeError.log".
Invoking "cmake" failed
JWhitleyWork commented 5 years ago

@shahvi - Try pulling, cleaning your catkin workspace, and building again. I added some additional CMake rules just a few minutes ago.

ghost commented 5 years ago

@JWhitleyAStuff still the same error. I am no expert in CMake, but what we had was working for libunwind. How does adding all this extra stuff help? Another point, we did discover that we need to comment out libunwind for an ARM architecture like for a Jetson TX1. Is this something that can be handled in the CMake rules?

JWhitleyWork commented 5 years ago

@shahvi - The reason these additions are necessary is so that, if libunwind (or one of the other libraries) doesn't exist on the user's system, it is caught at CMake time with a human-readable error instead of by the linker failing with a more cryptic error. This is common practice with ROS as well. The reason for the additions to package.xml is so that rosdep can find the necessary dependencies. This allows clean environments (like CI builds or the ROS build farm) to find the dependencies before compiling. Even though you won't be able to submit this package to the build farm right now (because Spinnaker isn't available as a ROS or Ubuntu package), you can still generate .deb files using CI and use CI to make sure everything builds correctly on each push or PR.

Re: LibUnwind CMake search process, try the latest push. Re: architecture-specific stuff, the FindLibUnwind.cmake seems to take care of that - though, admittedly, I have not tested it in an ARM environment.

ghost commented 5 years ago

@JWhitleyAStuff I still have the same error when running catkin_make.

JWhitleyWork commented 5 years ago

@shahvi - Thanks for clarifying that you were using catkin_make. I usually use catkin build and am generally in the spinnaker_sdk_camera_driver folder when I do so. That's why this wasn't showing up for me - I was using CMAKE_SOURCE_DIR which refers to the base of the CMake source infrastructure. In my case, this was the spinnaker folder. In your case, it was the root of the workspace. Using CMAKE_CURRENT_SOURCE_DIR fixed the issue. Please try again.

ghost commented 5 years ago

@JWhitleyAStuff , yep I can compile just fine now, thanks for fixing that. I have pushed a commit to the master branch that addresses all the warning that show up with C++11. Could you please pull forward your PR. Also, I see that you are depending on OpenMP now, what is that needed for?

JWhitleyWork commented 5 years ago

@shahvi - Strange, I could have sworn that OpenMP was a requirement before. OK. I've removed it and fixed one last C++11 bug in camera.cpp (there is no need to set the CameraPtr object to null - it has a destructor and should take care of cleaning up after itself) and a parameter error in a ROS_INFO call in capture.cpp.

JWhitleyWork commented 5 years ago

Very sorry for all of the back-and-forth. I'm going to create a new PR. There are some changes showing up in the PR that shouldn't be there (specifically in camera.cpp). I'll re-apply the changes that actually matter in the new PR.