From c00bf42186e5576a73a9a12befba125389adce7f Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Tue, 10 Dec 2024 00:05:18 -0800 Subject: [PATCH] Review fixes in rosbag2 composable player - Cleanups in CMakeLists.txt - Expose only the necessary constructors in rosbag2::player - Add visibility control - Add missing dependencies in package.xml - Add component_player.hpp file Signed-off-by: Michael Orlov --- rosbag2/CMakeLists.txt | 47 ++++++---------- rosbag2/include/rosbag2/component_player.hpp | 56 +++++++++++++++++++ .../include/rosbag2/visibility_control.hpp | 56 +++++++++++++++++++ rosbag2/package.xml | 5 +- rosbag2/src/rosbag2/component_player.cpp | 41 +++++++++++--- rosbag2_transport/CMakeLists.txt | 2 + 6 files changed, 167 insertions(+), 40 deletions(-) create mode 100644 rosbag2/include/rosbag2/component_player.hpp create mode 100644 rosbag2/include/rosbag2/visibility_control.hpp diff --git a/rosbag2/CMakeLists.txt b/rosbag2/CMakeLists.txt index e2e493286..41624ec94 100644 --- a/rosbag2/CMakeLists.txt +++ b/rosbag2/CMakeLists.txt @@ -12,52 +12,37 @@ if(NOT CMAKE_CXX_STANDARD) set(CMAKE_CXX_STANDARD_REQUIRED ON) endif() -if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang") - add_compile_options(-Wall -Wextra -Wpedantic) -endif() -if(CMAKE_BUILD_TYPE STREQUAL "Debug" AND MSVC) - # /bigobj is needed to avoid error C1128: - # https://msdn.microsoft.com/en-us/library/8578y171.aspx - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj") -endif() - -# Windows supplies macros for min and max by default. We should only use min and max from stl -if(WIN32) - add_definitions(-DNOMINMAX) -endif() - find_package(ament_cmake REQUIRED) +find_package(rclcpp REQUIRED) find_package(rclcpp_components REQUIRED) find_package(rosbag2_transport REQUIRED) -add_library(${PROJECT_NAME} SHARED +# Library for Rosbag2 composable nodes +add_library(${PROJECT_NAME}_nodes SHARED src/rosbag2/component_player.cpp +# src/rosbag2/composable_recorder.cpp ) +target_include_directories(${PROJECT_NAME}_nodes PUBLIC + $ + $) -target_link_libraries(${PROJECT_NAME} - rclcpp::rclcpp - ${rclcpp_components_TARGETS} - rosbag2_transport::rosbag2_transport -) +ament_target_dependencies(${PROJECT_NAME}_nodes rclcpp rclcpp_components rosbag2_transport) + +rclcpp_components_register_node(${PROJECT_NAME}_nodes PLUGIN "rosbag2::Player" EXECUTABLE player) -rclcpp_components_register_node(${PROJECT_NAME} - PLUGIN "rosbag2::Player" - EXECUTABLE player +install(DIRECTORY include/ + DESTINATION include/${PROJECT_NAME} ) install( - TARGETS ${PROJECT_NAME} - EXPORT export_${PROJECT_NAME} + TARGETS ${PROJECT_NAME}_nodes + EXPORT export_${PROJECT_NAME}_nodes ARCHIVE DESTINATION lib LIBRARY DESTINATION lib RUNTIME DESTINATION bin ) -ament_export_libraries(${PROJECT_NAME}) - -ament_export_dependencies( - rclcpp_components - rosbag2_transport -) +# Export modern CMake targets +ament_export_targets(export_${PROJECT_NAME}_nodes) ament_package() diff --git a/rosbag2/include/rosbag2/component_player.hpp b/rosbag2/include/rosbag2/component_player.hpp new file mode 100644 index 000000000..878954cb5 --- /dev/null +++ b/rosbag2/include/rosbag2/component_player.hpp @@ -0,0 +1,56 @@ +// Copyright 2024 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/* This header must be included by all rclcpp headers which declare symbols + * which are defined in the rclcpp library. When not building the rclcpp + * library, i.e. when using the headers in other package's code, the contents + * of this header change the visibility of certain symbols which the rclcpp + * library cannot have, but the consuming code must have inorder to link. + */ + +#ifndef ROSBAG2__COMPONENT_PLAYER_HPP_ +#define ROSBAG2__COMPONENT_PLAYER_HPP_ + +#include + +#include "rclcpp/node_options.hpp" +#include "rosbag2/visibility_control.hpp" +#include "rosbag2_transport/player.hpp" + +namespace rosbag2 +{ +class Player : public rosbag2_transport::Player { +public: + /// \brief Constructor and entry point for the composable player. + /// Will call Player(node_name, node_options) constructor with node_name = "rosbag2_player". + /// \param node_options Node options which will be used during construction of the underlying + /// node. + ROSBAG2_PUBLIC + explicit Player(const rclcpp::NodeOptions & node_options); + + /// \brief Default constructor and entry point for the composable player. + /// Will construct Player class and initialize play_options, storage_options from node + /// parameters. At the end will call Player::play() to automatically start playback in a + /// separate thread. + /// \param node_name Name for the underlying node. + /// \param node_options Node options which will be used during construction of the underlying + /// node. + ROSBAG2_PUBLIC + explicit Player( + const std::string & node_name = "rosbag2_player", + const rclcpp::NodeOptions & node_options = rclcpp::NodeOptions()); +}; +} // namespace rosbag2 + +#endif // ROSBAG2__COMPONENT_PLAYER_HPP_ diff --git a/rosbag2/include/rosbag2/visibility_control.hpp b/rosbag2/include/rosbag2/visibility_control.hpp new file mode 100644 index 000000000..6070ae721 --- /dev/null +++ b/rosbag2/include/rosbag2/visibility_control.hpp @@ -0,0 +1,56 @@ +// Copyright 2024 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/* This header must be included by all rclcpp headers which declare symbols + * which are defined in the rclcpp library. When not building the rclcpp + * library, i.e. when using the headers in other package's code, the contents + * of this header change the visibility of certain symbols which the rclcpp + * library cannot have, but the consuming code must have inorder to link. + */ + +#ifndef ROSBAG2__VISIBILITY_CONTROL_HPP_ +#define ROSBAG2__VISIBILITY_CONTROL_HPP_ + +// This logic was borrowed (then namespaced) from the examples on the gcc wiki: +// https://gcc.gnu.org/wiki/Visibility + +#if defined _WIN32 || defined __CYGWIN__ +#ifdef __GNUC__ + #define ROSBAG2_EXPORT __attribute__ ((dllexport)) + #define ROSBAG2_IMPORT __attribute__ ((dllimport)) + #else + #define ROSBAG2_EXPORT __declspec(dllexport) + #define ROSBAG2_IMPORT __declspec(dllimport) + #endif + #ifdef ROSBAG2_BUILDING_LIBRARY + #define ROSBAG2_PUBLIC ROSBAG2_EXPORT + #else + #define ROSBAG2_PUBLIC ROSBAG2_IMPORT + #endif + #define ROSBAG2_PUBLIC_TYPE ROSBAG2_PUBLIC + #define ROSBAG2_LOCAL +#else +#define ROSBAG2_EXPORT __attribute__ ((visibility("default"))) +#define ROSBAG2_IMPORT +#if __GNUC__ >= 4 +#define ROSBAG2_PUBLIC __attribute__ ((visibility("default"))) +#define ROSBAG2_LOCAL __attribute__ ((visibility("hidden"))) +#else +#define ROSBAG2_PUBLIC + #define ROSBAG2_LOCAL +#endif +#define ROSBAG2_PUBLIC_TYPE +#endif + +#endif // ROSBAG2__VISIBILITY_CONTROL_HPP_ diff --git a/rosbag2/package.xml b/rosbag2/package.xml index 70c4d6252..34202a1aa 100644 --- a/rosbag2/package.xml +++ b/rosbag2/package.xml @@ -12,12 +12,15 @@ ament_cmake + rclcpp + rclcpp_components + rosbag2_transport + ros2bag rosbag2_compression rosbag2_cpp rosbag2_py rosbag2_storage - rosbag2_transport rosbag2_compression_zstd diff --git a/rosbag2/src/rosbag2/component_player.cpp b/rosbag2/src/rosbag2/component_player.cpp index 82235b015..5af111412 100644 --- a/rosbag2/src/rosbag2/component_player.cpp +++ b/rosbag2/src/rosbag2/component_player.cpp @@ -1,14 +1,39 @@ -#include "rosbag2_transport/player.hpp" +// Copyright 2024 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. -namespace rosbag2 { -class Player : public rosbag2_transport::Player { - using rosbag2_transport::Player::Player; -}; -} +/* This header must be included by all rclcpp headers which declare symbols + * which are defined in the rclcpp library. When not building the rclcpp + * library, i.e. when using the headers in other package's code, the contents + * of this header change the visibility of certain symbols which the rclcpp + * library cannot have, but the consuming code must have inorder to link. + */ #include "rclcpp_components/register_node_macro.hpp" +#include "rosbag2/component_player.hpp" + +namespace rosbag2 +{ +Player::Player(const rclcpp::NodeOptions & node_options) +: Player("rosbag2_player", node_options) {} + +Player::Player( + const std::string & node_name, + const rclcpp::NodeOptions & node_options) +: rosbag2_transport::Player(node_name, node_options) {} +} // namespace rosbag2 // Register the component with class_loader. -// This acts as a sort of entry point, allowing the component to be +// This acts as an entry point, allowing the component to be // discoverable when its library is being loaded into a running process. -RCLCPP_COMPONENTS_REGISTER_NODE(rosbag2::Player) \ No newline at end of file +RCLCPP_COMPONENTS_REGISTER_NODE(rosbag2::Player) diff --git a/rosbag2_transport/CMakeLists.txt b/rosbag2_transport/CMakeLists.txt index 318367255..e3670d7c6 100644 --- a/rosbag2_transport/CMakeLists.txt +++ b/rosbag2_transport/CMakeLists.txt @@ -72,9 +72,11 @@ target_link_libraries(${PROJECT_NAME} PRIVATE yaml-cpp::yaml-cpp ) +# rosbag2_transport::Player composable node is deprecated. Please use rosbag2::Player instead rclcpp_components_register_node( ${PROJECT_NAME} PLUGIN "rosbag2_transport::Player" EXECUTABLE player) +# rosbag2_transport::Recorder composable node is deprecated. Please use rosbag2::Recorder instead rclcpp_components_register_node( ${PROJECT_NAME} PLUGIN "rosbag2_transport::Recorder" EXECUTABLE recorder)