Updated Naive implementation with Dependency Injection #19

bgill92 · 2023-09-03T16:25:21Z

Description

Updated the Naive implementation with Dependency Injection.

Testing

Launch the docker container and run:

colcon build --packages-select naive
colcon test --packages-select naive

Make sure he package builds and all the tests pass.

griswaldbrooks

So, I'd like the naive example to stay... naive. Like close to what you would see in a ros tutorial. I have wondered if it should be renamed to something other than naive. Can we make this it's own example?

griswaldbrooks

Ah, I didn't see the superclass. I'd still like to keep the names. One could be naive or a nicer word and the other, dependency_injection or something like folders with pubsub and services under a folder describing the technique

griswaldbrooks · 2023-09-05T22:46:51Z

naive/include/naive/incrementer.hpp

+   */
+  struct MiddlewareHandle {
+    using Callback = std::function<void(const std_msgs::msg::Int64::SharedPtr)>;
+    virtual void registerCallback(Callback cb) = 0;


Can we get doxygen comments on these methods?

griswaldbrooks · 2023-09-05T22:49:17Z

naive/include/naive/incrementer.hpp

-  std::shared_ptr<rclcpp::Node> node_;
-  std::shared_ptr<rclcpp::Publisher<std_msgs::msg::Int64>> pub_;
-  std::shared_ptr<rclcpp::Subscription<std_msgs::msg::Int64>> sub_;
+  std::unique_ptr<MiddlewareHandle> mw_;


Let's keep this as a unique_ptr but we should either have a comment or example mentioning that raw pointers and references will work here too. Holding a reference feels odd to me but other DI examples do it.

griswaldbrooks · 2023-09-05T22:49:29Z

naive/include/naive/ros_middleware.hpp

+/*********************************************************************
+ * Software License Agreement (BSD License)
+ *
+ *  Copyright (c) 2022, PickNik, LLC.


Suggested change

* Copyright (c) 2022, PickNik, LLC.

* Copyright (c) 2023, PickNik, LLC.

griswaldbrooks · 2023-09-05T22:51:40Z

naive/include/naive/ros_middleware.hpp

+  static constexpr int QUEUE_SIZE = 10;
+  rclcpp::Node::SharedPtr node_handle_;
+  std::string in_topic_;
+  rclcpp::Subscription<std_msgs::msg::Int64>::SharedPtr subscriber_;


I would prefer

Suggested change

rclcpp::Subscription<std_msgs::msg::Int64>::SharedPtr subscriber_;

std::shared_ptr<rclcpp::Subscription<std_msgs::msg::Int64>> subscriber_;

but if you think it's more instructive to use the ros types, i'm ok with that.

griswaldbrooks · 2023-09-05T22:52:07Z

naive/include/naive/ros_middleware.hpp

+public: 
+  RosMiddleware(const rclcpp::Node::SharedPtr node_handle, std::string in_topic,
+                const std::string& out_topic);
+  void registerCallback(Callback cb) override;


griswaldbrooks · 2023-09-05T22:54:40Z

naive/src/node.cpp

 int main(int argc, char **argv) {
  rclcpp::init(argc, argv);
  auto const node = std::make_shared<rclcpp::Node>("incrementer");
-  Incrementer incrementer{node, "/in", "/out"};
+  Incrementer incrementer{std::make_unique<RosMiddleware>(node, "/in", "/out")};


Suggested change

Incrementer incrementer{std::make_unique<RosMiddleware>(node, "/in", "/out")};

Incrementer const incrementer{std::make_unique<RosMiddleware>(node, "/in", "/out")};

griswaldbrooks · 2023-09-05T22:58:14Z

naive_superclass/include/naive_superclass/incrementer.hpp

+  /**
+   * @brief      Incrementer constructor
+   *
+   * @param[in]  mw       Unique pointer to a MiddlewareHandle object


griswaldbrooks · 2023-09-05T23:00:18Z

naive_superclass/src/incrementer.cpp

+Incrementer::Incrementer(const std::string node_name, const std::string in_topic, const std::string out_topic)
+	: Node(node_name)
+{
+	publisher_ = this->create_publisher<std_msgs::msg::Int64>(out_topic, QUEUE_SIZE);


ok, well, we probably can still use the initializer list instead of assignment

griswaldbrooks · 2023-09-05T23:03:56Z

naive_superclass/src/incrementer.cpp

+	: Node(node_name)
+{
+	publisher_ = this->create_publisher<std_msgs::msg::Int64>(out_topic, QUEUE_SIZE);
+	subscriber_ = this->create_subscription<std_msgs::msg::Int64>(in_topic, QUEUE_SIZE, std::bind(&Incrementer::callback, this, std::placeholders::_1));


Suggested change

subscriber_ = this->create_subscription<std_msgs::msg::Int64>(in_topic, QUEUE_SIZE, std::bind(&Incrementer::callback, this, std::placeholders::_1));

subscriber_ = this->create_subscription<std_msgs::msg::Int64>(in_topic, QUEUE_SIZE, [this](const std_msgs::msg::Int64::SharedPtr msg){

std_msgs::msg::Int64 incremented;

incremented.data = msg->data + 1;

publisher_->publish(incremented);

});

https://abseil.io/tips/108 (you might even want to add this as a comment)

or

Suggested change

subscriber_ = this->create_subscription<std_msgs::msg::Int64>(in_topic, QUEUE_SIZE, std::bind(&Incrementer::callback, this, std::placeholders::_1));

subscriber_ = this->create_subscription<std_msgs::msg::Int64>(in_topic, QUEUE_SIZE, [this](const std_msgs::msg::Int64::SharedPtr msg){ callback(msg); });

Initial commit of updated naive implementation using DI

01e9dcb

bgill92 requested a review from griswaldbrooks September 3, 2023 16:25

bgill92 assigned griswaldbrooks Sep 3, 2023

Add example of a superclass

bee2351

bgill92 force-pushed the bgill92/update-naive-example branch from d5070d1 to bee2351 Compare September 4, 2023 22:44

griswaldbrooks reviewed Sep 5, 2023

View reviewed changes

griswaldbrooks assigned bgill92 and unassigned griswaldbrooks Sep 5, 2023

Added CI for naive tests

cf3063b

bgill92 closed this Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated Naive implementation with Dependency Injection #19

Updated Naive implementation with Dependency Injection #19

bgill92 commented Sep 3, 2023

griswaldbrooks left a comment

griswaldbrooks left a comment

griswaldbrooks Sep 5, 2023

griswaldbrooks Sep 5, 2023

griswaldbrooks Sep 5, 2023

griswaldbrooks Sep 5, 2023

griswaldbrooks Sep 5, 2023

griswaldbrooks Sep 5, 2023

griswaldbrooks Sep 5, 2023

griswaldbrooks Sep 5, 2023

griswaldbrooks Sep 5, 2023

	* Copyright (c) 2022, PickNik, LLC.
	* Copyright (c) 2023, PickNik, LLC.

	rclcpp::Subscription<std_msgs::msg::Int64>::SharedPtr subscriber_;
	std::shared_ptr<rclcpp::Subscription<std_msgs::msg::Int64>> subscriber_;

	Incrementer incrementer{std::make_unique<RosMiddleware>(node, "/in", "/out")};
	Incrementer const incrementer{std::make_unique<RosMiddleware>(node, "/in", "/out")};

-	subscriber_ = this->create_subscription<std_msgs::msg::Int64>(in_topic, QUEUE_SIZE, std::bind(&Incrementer::callback, this, std::placeholders::_1));
+	subscriber_ = this->create_subscription<std_msgs::msg::Int64>(in_topic, QUEUE_SIZE, [this](const std_msgs::msg::Int64::SharedPtr msg){
+	 std_msgs::msg::Int64 incremented;
+         incremented.data = msg->data + 1;
+         publisher_->publish(incremented);
+	});

	subscriber_ = this->create_subscription<std_msgs::msg::Int64>(in_topic, QUEUE_SIZE, std::bind(&Incrementer::callback, this, std::placeholders::_1));
	subscriber_ = this->create_subscription<std_msgs::msg::Int64>(in_topic, QUEUE_SIZE, [this](const std_msgs::msg::Int64::SharedPtr msg){ callback(msg); });

Updated Naive implementation with Dependency Injection #19

Updated Naive implementation with Dependency Injection #19

Conversation

bgill92 commented Sep 3, 2023

Description

Testing

griswaldbrooks left a comment

Choose a reason for hiding this comment

griswaldbrooks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment