clean up publisher and subscription creation logic (#867)

* streamline creation of publishers after removing deprecated API

Signed-off-by: William Woodall <william@osrfoundation.org>

* use deduced template arguments to cleanup create_subscription

Signed-off-by: William Woodall <william@osrfoundation.org>

* add missing file

Signed-off-by: William Woodall <william@osrfoundation.org>

* streamline creation of subscriptions after removing deprecated API

Signed-off-by: William Woodall <william@osrfoundation.org>

* small subscription code cleanup to match publisher's style

Signed-off-by: William Woodall <william@osrfoundation.org>

* some fixes to rclcpp_lifecycle to match rclcpp

Signed-off-by: William Woodall <william@osrfoundation.org>

* add README to the rclcpp/detail include directory

Signed-off-by: William Woodall <william@osrfoundation.org>

* fixup SubscriptionBase's use of visibility macros

Signed-off-by: William Woodall <william@osrfoundation.org>

* reapply function to create default options, as it is still needed on Windows

Signed-off-by: William Woodall <william@osrfoundation.org>

* address review comments

Signed-off-by: William Woodall <william@osrfoundation.org>

* workaround cppcheck 1.89 syntax error

Signed-off-by: William Woodall <william@osrfoundation.org>

Author: wjwwood

rclcpp/include/rclcpp/callback_group.hpp

@@ -21,9 +21,9 @@
 #include <vector>
 
 #include "rclcpp/client.hpp"
-#include "rclcpp/publisher.hpp"
+#include "rclcpp/publisher_base.hpp"
 #include "rclcpp/service.hpp"
-#include "rclcpp/subscription.hpp"
+#include "rclcpp/subscription_base.hpp"
 #include "rclcpp/timer.hpp"
 #include "rclcpp/visibility_control.hpp"
 #include "rclcpp/waitable.hpp"

rclcpp/include/rclcpp/create_publisher.hpp

@@ -37,7 +37,7 @@ namespace rclcpp
 template<
   typename MessageT,
   typename AllocatorT = std::allocator<void>,
-  typename PublisherT = ::rclcpp::Publisher<MessageT, AllocatorT>,
+  typename PublisherT = rclcpp::Publisher<MessageT, AllocatorT>,
   typename NodeT>
 std::shared_ptr<PublisherT>
 create_publisher(
@@ -46,43 +46,23 @@ create_publisher(
   const rclcpp::QoS & qos,
   const rclcpp::PublisherOptionsWithAllocator<AllocatorT> & options = (
     rclcpp::PublisherOptionsWithAllocator<AllocatorT>()
-))
+  )
+)
 {
+  // Extract the NodeTopicsInterface from the NodeT.
   using rclcpp::node_interfaces::get_node_topics_interface;
   auto node_topics = get_node_topics_interface(node);
 
-  std::shared_ptr<AllocatorT> allocator = options.allocator;
-  if (!allocator) {
-    allocator = std::make_shared<AllocatorT>();
-  }
-
-  bool use_intra_process;
-  switch (options.use_intra_process_comm) {
-    case IntraProcessSetting::Enable:
-      use_intra_process = true;
-      break;
-    case IntraProcessSetting::Disable:
-      use_intra_process = false;
-      break;
-    case IntraProcessSetting::NodeDefault:
-      use_intra_process = node_topics->get_node_base_interface()->get_use_intra_process_default();
-      break;
-    default:
-      throw std::runtime_error("Unrecognized IntraProcessSetting value");
-      break;
-  }
-
-  // TODO(wjwwood): convert all of the interfaces to use QoS and PublisherOptionsBase
+  // Create the publisher.
   auto pub = node_topics->create_publisher(
     topic_name,
-    rclcpp::create_publisher_factory<MessageT, AllocatorT, PublisherT>(
-      options.event_callbacks,
-      allocator
-    ),
-    options.template to_rcl_publisher_options<MessageT>(qos),
-    use_intra_process
+    rclcpp::create_publisher_factory<MessageT, AllocatorT, PublisherT>(options),
+    qos
   );
+
+  // Add the publisher to the node topics interface.
   node_topics->add_publisher(pub, options.callback_group);
+
   return std::dynamic_pointer_cast<PublisherT>(pub);
 }
 

rclcpp/include/rclcpp/create_subscription.hpp

@@ -42,6 +42,10 @@ template<
   typename CallbackMessageT =
   typename rclcpp::subscription_traits::has_message_type<CallbackT>::type,
   typename SubscriptionT = rclcpp::Subscription<CallbackMessageT, AllocatorT>,
+  typename MessageMemoryStrategyT = rclcpp::message_memory_strategy::MessageMemoryStrategy<
+    CallbackMessageT,
+    AllocatorT
+  >,
   typename NodeT>
 typename std::shared_ptr<SubscriptionT>
 create_subscription(
@@ -52,48 +56,21 @@ create_subscription(
   const rclcpp::SubscriptionOptionsWithAllocator<AllocatorT> & options = (
     rclcpp::SubscriptionOptionsWithAllocator<AllocatorT>()
   ),
-  typename rclcpp::message_memory_strategy::MessageMemoryStrategy<
-    CallbackMessageT, AllocatorT>::SharedPtr
-  msg_mem_strat = nullptr)
+  typename MessageMemoryStrategyT::SharedPtr msg_mem_strat = (
+    MessageMemoryStrategyT::create_default()
+  )
+)
 {
   using rclcpp::node_interfaces::get_node_topics_interface;
   auto node_topics = get_node_topics_interface(std::forward<NodeT>(node));
 
-  if (!msg_mem_strat) {
-    using rclcpp::message_memory_strategy::MessageMemoryStrategy;
-    msg_mem_strat = MessageMemoryStrategy<CallbackMessageT, AllocatorT>::create_default();
-  }
+  auto factory = rclcpp::create_subscription_factory<MessageT>(
+    std::forward<CallbackT>(callback),
+    options,
+    msg_mem_strat
+  );
 
-  std::shared_ptr<AllocatorT> allocator = options.allocator;
-  if (!allocator) {
-    allocator = std::make_shared<AllocatorT>();
-  }
-  auto factory = rclcpp::create_subscription_factory
-    <MessageT, CallbackT, AllocatorT, CallbackMessageT, SubscriptionT>(
-    std::forward<CallbackT>(callback), options.event_callbacks, msg_mem_strat, allocator);
-
-  bool use_intra_process;
-  switch (options.use_intra_process_comm) {
-    case IntraProcessSetting::Enable:
-      use_intra_process = true;
-      break;
-    case IntraProcessSetting::Disable:
-      use_intra_process = false;
-      break;
-    case IntraProcessSetting::NodeDefault:
-      use_intra_process = node_topics->get_node_base_interface()->get_use_intra_process_default();
-      break;
-    default:
-      throw std::runtime_error("Unrecognized IntraProcessSetting value");
-      break;
-  }
-
-  // TODO(wjwwood): convert all of the interfaces to use QoS and SubscriptionOptionsBase
-  auto sub = node_topics->create_subscription(
-    topic_name,
-    factory,
-    options.template to_rcl_subscription_options<MessageT>(qos),
-    use_intra_process);
+  auto sub = node_topics->create_subscription(topic_name, factory, qos);
   node_topics->add_subscription(sub, options.callback_group);
   return std::dynamic_pointer_cast<SubscriptionT>(sub);
 }

rclcpp/include/rclcpp/detail/README.md

@@ -0,0 +1,3 @@
+Notice that headers in this folder should only provide symbols in the rclcpp::detail namespace.
+
+Also that these headers are not considered part of the public API and are subject to change without notice.

rclcpp/include/rclcpp/detail/resolve_use_intra_process.hpp

@@ -0,0 +1,55 @@
+// Copyright 2019 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.
+
+#ifndef RCLCPP__DETAIL__RESOLVE_USE_INTRA_PROCESS_HPP_
+#define RCLCPP__DETAIL__RESOLVE_USE_INTRA_PROCESS_HPP_
+
+#include <stdexcept>
+
+#include "rclcpp/intra_process_setting.hpp"
+
+namespace rclcpp
+{
+
+namespace detail
+{
+
+template<typename OptionsT, typename NodeBaseT>
+bool
+resolve_use_intra_process(const OptionsT & options, const NodeBaseT & node_base)
+{
+  bool use_intra_process;
+  switch (options.use_intra_process_comm) {
+    case IntraProcessSetting::Enable:
+      use_intra_process = true;
+      break;
+    case IntraProcessSetting::Disable:
+      use_intra_process = false;
+      break;
+    case IntraProcessSetting::NodeDefault:
+      use_intra_process = node_base.get_use_intra_process_default();
+      break;
+    default:
+      throw std::runtime_error("Unrecognized IntraProcessSetting value");
+      break;
+  }
+
+  return use_intra_process;
+}
+
+}  // namespace detail
+
+}  // namespace rclcpp
+
+#endif  // RCLCPP__DETAIL__RESOLVE_USE_INTRA_PROCESS_HPP_

rclcpp/include/rclcpp/node.hpp

@@ -192,27 +192,29 @@ class Node : public std::enable_shared_from_this<Node>
    * \param[in] msg_mem_strat The message memory strategy to use for allocating messages.
    * \return Shared pointer to the created subscription.
    */
-  /* TODO(jacquelinekay):
-     Windows build breaks when static member function passed as default
-     argument to msg_mem_strat, nullptr is a workaround.
-   */
   template<
     typename MessageT,
     typename CallbackT,
     typename AllocatorT = std::allocator<void>,
-    typename SubscriptionT = rclcpp::Subscription<
-      typename rclcpp::subscription_traits::has_message_type<CallbackT>::type, AllocatorT>>
+    typename CallbackMessageT =
+    typename rclcpp::subscription_traits::has_message_type<CallbackT>::type,
+    typename SubscriptionT = rclcpp::Subscription<CallbackMessageT, AllocatorT>,
+    typename MessageMemoryStrategyT = rclcpp::message_memory_strategy::MessageMemoryStrategy<
+      CallbackMessageT,
+      AllocatorT
+    >
+  >
   std::shared_ptr<SubscriptionT>
   create_subscription(
     const std::string & topic_name,
     const rclcpp::QoS & qos,
     CallbackT && callback,
     const SubscriptionOptionsWithAllocator<AllocatorT> & options =
     SubscriptionOptionsWithAllocator<AllocatorT>(),
-    typename rclcpp::message_memory_strategy::MessageMemoryStrategy<
-      typename rclcpp::subscription_traits::has_message_type<CallbackT>::type, AllocatorT
-    >::SharedPtr
-    msg_mem_strat = nullptr);
+    typename MessageMemoryStrategyT::SharedPtr msg_mem_strat = (
+      MessageMemoryStrategyT::create_default()
+    )
+  );
 
   /// Create a timer.
   /**

rclcpp/include/rclcpp/node_impl.hpp

@@ -83,16 +83,16 @@ template<
   typename MessageT,
   typename CallbackT,
   typename AllocatorT,
-  typename SubscriptionT>
+  typename CallbackMessageT,
+  typename SubscriptionT,
+  typename MessageMemoryStrategyT>
 std::shared_ptr<SubscriptionT>
 Node::create_subscription(
   const std::string & topic_name,
   const rclcpp::QoS & qos,
   CallbackT && callback,
   const SubscriptionOptionsWithAllocator<AllocatorT> & options,
-  typename rclcpp::message_memory_strategy::MessageMemoryStrategy<
-    typename rclcpp::subscription_traits::has_message_type<CallbackT>::type, AllocatorT>::SharedPtr
-  msg_mem_strat)
+  typename MessageMemoryStrategyT::SharedPtr msg_mem_strat)
 {
   return rclcpp::create_subscription<MessageT>(
     *this,

rclcpp/include/rclcpp/node_interfaces/node_topics.hpp

@@ -49,8 +49,7 @@ class NodeTopics : public NodeTopicsInterface
   create_publisher(
     const std::string & topic_name,
     const rclcpp::PublisherFactory & publisher_factory,
-    const rcl_publisher_options_t & publisher_options,
-    bool use_intra_process) override;
+    const rclcpp::QoS & qos) override;
 
   RCLCPP_PUBLIC
   void
@@ -63,8 +62,7 @@ class NodeTopics : public NodeTopicsInterface
   create_subscription(
     const std::string & topic_name,
     const rclcpp::SubscriptionFactory & subscription_factory,
-    const rcl_subscription_options_t & subscription_options,
-    bool use_intra_process) override;
+    const rclcpp::QoS & qos) override;
 
   RCLCPP_PUBLIC
   void

rclcpp/include/rclcpp/node_interfaces/node_topics_interface.hpp

@@ -50,8 +50,7 @@ class NodeTopicsInterface
   create_publisher(
     const std::string & topic_name,
     const rclcpp::PublisherFactory & publisher_factory,
-    const rcl_publisher_options_t & publisher_options,
-    bool use_intra_process) = 0;
+    const rclcpp::QoS & qos) = 0;
 
   RCLCPP_PUBLIC
   virtual
@@ -66,8 +65,7 @@ class NodeTopicsInterface
   create_subscription(
     const std::string & topic_name,
     const rclcpp::SubscriptionFactory & subscription_factory,
-    const rcl_subscription_options_t & subscription_options,
-    bool use_intra_process) = 0;
+    const rclcpp::QoS & qos) = 0;
 
   RCLCPP_PUBLIC
   virtual