Address review comments and remove change for time_source

Signed-off-by: Barry Xu <[email protected]>

Author: Barry-Xu-2018

rclcpp/CMakeLists.txt

@@ -73,8 +73,8 @@ set(${PROJECT_NAME}_SRCS
   src/rclcpp/message_info.cpp
   src/rclcpp/network_flow_endpoint.cpp
   src/rclcpp/node.cpp
+  src/rclcpp/node_builtin_executor.cpp
   src/rclcpp/node_interfaces/node_base.cpp
-  src/rclcpp/node_interfaces/node_builtin_executor.cpp
   src/rclcpp/node_interfaces/node_clock.cpp
   src/rclcpp/node_interfaces/node_graph.cpp
   src/rclcpp/node_interfaces/node_logging.cpp

rclcpp/include/rclcpp/node.hpp

@@ -47,8 +47,8 @@
 #include "rclcpp/logger.hpp"
 #include "rclcpp/macros.hpp"
 #include "rclcpp/message_memory_strategy.hpp"
+#include "rclcpp/node_builtin_executor.hpp"
 #include "rclcpp/node_interfaces/node_base_interface.hpp"
-#include "rclcpp/node_interfaces/node_builtin_executor.hpp"
 #include "rclcpp/node_interfaces/node_clock_interface.hpp"
 #include "rclcpp/node_interfaces/node_graph_interface.hpp"
 #include "rclcpp/node_interfaces/node_logging_interface.hpp"
@@ -1588,7 +1588,8 @@ class Node : public std::enable_shared_from_this<Node>
   rclcpp::node_interfaces::NodeParametersInterface::SharedPtr node_parameters_;
   rclcpp::node_interfaces::NodeTimeSourceInterface::SharedPtr node_time_source_;
   rclcpp::node_interfaces::NodeWaitablesInterface::SharedPtr node_waitables_;
-  rclcpp::node_interfaces::NodeBuiltinExecutor::SharedPtr node_builtin_executor_;
+
+  rclcpp::NodeBuiltinExecutor::SharedPtr node_builtin_executor_;
 
   const rclcpp::NodeOptions node_options_;
   const std::string sub_namespace_;

rclcpp/include/rclcpp/node_interfaces/node_builtin_executor.hpp renamed to rclcpp/include/rclcpp/node_builtin_executor.hpp

@@ -12,8 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#ifndef RCLCPP__NODE_INTERFACES__NODE_BUILTIN_EXECUTOR_HPP_
-#define RCLCPP__NODE_INTERFACES__NODE_BUILTIN_EXECUTOR_HPP_
+#ifndef RCLCPP__NODE_BUILTIN_EXECUTOR_HPP_
+#define RCLCPP__NODE_BUILTIN_EXECUTOR_HPP_
+
+#include <memory>
 
 #include "rclcpp/executor.hpp"
 #include "rclcpp/macros.hpp"
@@ -25,11 +27,11 @@
 #include "rclcpp/node_options.hpp"
 #include "rclcpp/visibility_control.hpp"
 
+#include "rcl_interfaces/srv/get_logger_levels.hpp"
+#include "rcl_interfaces/srv/set_logger_levels.hpp"
+
 namespace rclcpp
 {
-namespace node_interfaces
-{
-
 class NodeBuiltinExecutor
 {
 public:
@@ -46,23 +48,12 @@ class NodeBuiltinExecutor
   RCLCPP_PUBLIC
   ~NodeBuiltinExecutor();
 
-  RCLCPP_PUBLIC
-  bool builtin_thread_joinable();
-
 private:
   RCLCPP_DISABLE_COPY(NodeBuiltinExecutor)
-
-  node_interfaces::NodeBaseInterface::SharedPtr node_base_;
-  node_interfaces::NodeTopicsInterface::SharedPtr node_topics_;
-  node_interfaces::NodeServicesInterface::SharedPtr node_services_;
-  node_interfaces::NodeLoggingInterface::SharedPtr node_logging_;
-
-  rclcpp::Executor::SharedPtr executor_;
-  std::promise<void> executor_promise_;
-  std::thread thread_;
+  class NodeBuiltinExecutorImpl;
+  std::shared_ptr<NodeBuiltinExecutorImpl> impl_;
 };
 
-}  // namespace node_interfaces
 }  // namespace rclcpp
 
-#endif  // RCLCPP__NODE_INTERFACES__NODE_BUILTIN_EXECUTOR_HPP_
+#endif  // RCLCPP__NODE_BUILTIN_EXECUTOR_HPP_

rclcpp/include/rclcpp/node_interfaces/node_base.hpp

@@ -24,7 +24,6 @@
 #include "rcl/node.h"
 #include "rclcpp/callback_group.hpp"
 #include "rclcpp/context.hpp"
-#include "rclcpp/executors.hpp"
 #include "rclcpp/macros.hpp"
 #include "rclcpp/node_interfaces/node_base_interface.hpp"
 #include "rclcpp/visibility_control.hpp"
@@ -99,14 +98,6 @@ class NodeBase : public NodeBaseInterface, public std::enable_shared_from_this<N
     rclcpp::CallbackGroupType group_type,
     bool automatically_add_to_executor_with_node = true) override;
 
-  RCLCPP_PUBLIC
-  rclcpp::CallbackGroup::SharedPtr
-  get_builtin_performance_callback_group() override;
-
-  RCLCPP_PUBLIC
-  rclcpp::CallbackGroup::SharedPtr
-  get_builtin_callback_group() override;
-
   RCLCPP_PUBLIC
   rclcpp::CallbackGroup::SharedPtr
   get_default_callback_group() override;
@@ -164,9 +155,6 @@ class NodeBase : public NodeBaseInterface, public std::enable_shared_from_this<N
   mutable std::recursive_mutex notify_guard_condition_mutex_;
   rclcpp::GuardCondition notify_guard_condition_;
   bool notify_guard_condition_is_valid_;
-
-  rclcpp::CallbackGroup::SharedPtr builtin_performance_callback_group_ = nullptr;
-  rclcpp::CallbackGroup::SharedPtr builtin_callback_group_ = nullptr;
 };
 
 }  // namespace node_interfaces

rclcpp/include/rclcpp/node_interfaces/node_base_interface.hpp

@@ -112,22 +112,6 @@ class NodeBaseInterface
     rclcpp::CallbackGroupType group_type,
     bool automatically_add_to_executor_with_node = true) = 0;
 
-  /// Return a callback group for the time-sensitive tasks such as clock.
-  /**
-   * This callback group is executed in internal executor.
-   * Normal tasks use get_builtin_callback_group().
-   */
-  RCLCPP_PUBLIC
-  virtual
-  rclcpp::CallbackGroup::SharedPtr
-  get_builtin_performance_callback_group() = 0;
-
-  /// Return a callback group which is executed in internal executor.
-  RCLCPP_PUBLIC
-  virtual
-  rclcpp::CallbackGroup::SharedPtr
-  get_builtin_callback_group() = 0;
-
   /// Return the default callback group.
   RCLCPP_PUBLIC
   virtual

rclcpp/include/rclcpp/node_interfaces/node_logging.hpp

@@ -19,13 +19,9 @@
 
 #include "rclcpp/logger.hpp"
 #include "rclcpp/macros.hpp"
-#include "rclcpp/node.hpp"
 #include "rclcpp/node_interfaces/node_base_interface.hpp"
 #include "rclcpp/node_interfaces/node_logging_interface.hpp"
-#include "rclcpp/node_interfaces/node_services_interface.hpp"
 #include "rclcpp/visibility_control.hpp"
-#include "rcl_interfaces/srv/get_logger_levels.hpp"
-#include "rcl_interfaces/srv/set_logger_levels.hpp"
 
 namespace rclcpp
 {
@@ -53,22 +49,13 @@ class NodeLogging : public NodeLoggingInterface
   const char *
   get_logger_name() const override;
 
-  RCLCPP_LOCAL
-  void
-  add_logger_service(
-    rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base,
-    rclcpp::node_interfaces::NodeServicesInterface::SharedPtr node_services);
-
 private:
   RCLCPP_DISABLE_COPY(NodeLogging)
 
   /// Handle to the NodeBaseInterface given in the constructor.
   rclcpp::node_interfaces::NodeBaseInterface * node_base_;
 
   rclcpp::Logger logger_;
-
-  rclcpp::Service<rcl_interfaces::srv::GetLoggerLevels>::SharedPtr get_loggers_service_;
-  rclcpp::Service<rcl_interfaces::srv::SetLoggerLevels>::SharedPtr set_loggers_service_;
 };
 
 }  // namespace node_interfaces

rclcpp/include/rclcpp/node_interfaces/node_logging_interface.hpp

@@ -20,7 +20,6 @@
 #include "rclcpp/logger.hpp"
 #include "rclcpp/macros.hpp"
 #include "rclcpp/node_interfaces/detail/node_interfaces_helpers.hpp"
-#include "rclcpp/node_options.hpp"
 #include "rclcpp/visibility_control.hpp"
 
 namespace rclcpp

rclcpp/include/rclcpp/time_source.hpp

@@ -25,7 +25,7 @@
 #include "rcl_interfaces/msg/parameter_event.hpp"
 
 #include "rclcpp/node.hpp"
-#include "rclcpp/node_interfaces/node_base.hpp"
+#include "rclcpp/executors.hpp"
 #include "rclcpp/node_interfaces/node_parameters_interface.hpp"
 
 
@@ -138,6 +138,10 @@ class TimeSource
   RCLCPP_PUBLIC
   void set_use_clock_thread(bool use_clock_thread);
 
+  /// Check if the clock thread is joinable
+  RCLCPP_PUBLIC
+  bool clock_thread_is_joinable();
+
   /// TimeSource Destructor
   RCLCPP_PUBLIC
   ~TimeSource();

rclcpp/src/rclcpp/node.cpp

@@ -27,8 +27,8 @@
 #include "rclcpp/exceptions.hpp"
 #include "rclcpp/graph_listener.hpp"
 #include "rclcpp/node.hpp"
+#include "rclcpp/node_builtin_executor.hpp"
 #include "rclcpp/node_interfaces/node_base.hpp"
-#include "rclcpp/node_interfaces/node_builtin_executor.hpp"
 #include "rclcpp/node_interfaces/node_clock.hpp"
 #include "rclcpp/node_interfaces/node_graph.hpp"
 #include "rclcpp/node_interfaces/node_logging.hpp"
@@ -208,6 +208,13 @@ Node::Node(
       options.use_clock_thread()
     )),
   node_waitables_(new rclcpp::node_interfaces::NodeWaitables(node_base_.get())),
+  node_builtin_executor_(new rclcpp::NodeBuiltinExecutor(
+      node_base_,
+      node_topics_,
+      node_services_,
+      node_logging_,
+      options
+    )),
   node_options_(options),
   sub_namespace_(""),
   effective_namespace_(create_effective_namespace(this->get_namespace(), sub_namespace_))
@@ -226,9 +233,6 @@ Node::Node(
     node_topics_->resolve_topic_name("/parameter_events"),
     options.parameter_event_qos(),
     rclcpp::detail::PublisherQosParametersTraits{});
-
-  node_builtin_executor_ = std::make_shared<rclcpp::node_interfaces::NodeBuiltinExecutor>(
-    node_base_, node_topics_, node_services_, node_logging_, options);
 }
 
 Node::Node(