Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated Naive implementation with Dependency Injection #19

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion naive/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ find_package(ament_cmake REQUIRED)
find_package(rclcpp REQUIRED)
find_package(std_msgs REQUIRED)

add_executable(naive src/node.cpp src/incrementer.cpp)
add_executable(naive src/node.cpp src/incrementer.cpp src/ros_middleware.cpp)
target_include_directories(naive PRIVATE include)
ament_target_dependencies(naive rclcpp std_msgs)

Expand All @@ -19,4 +19,11 @@ install(TARGETS
DESTINATION lib/${PROJECT_NAME}
)

if(BUILD_TESTING)
find_package(ament_cmake_gmock REQUIRED)
ament_add_gmock(incrementer_test test/incrementer_test.cpp src/incrementer.cpp src/ros_middleware.cpp TIMEOUT 5)
target_include_directories(incrementer_test PRIVATE include)
ament_target_dependencies(incrementer_test rclcpp std_msgs)
endif()

ament_package()
22 changes: 14 additions & 8 deletions naive/include/naive/incrementer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,18 +42,24 @@

class Incrementer {
public:

/**
* @brief An injectable Middleware Object
*/
struct MiddlewareHandle {
using Callback = std::function<void(const std_msgs::msg::Int64::SharedPtr)>;
virtual void registerCallback(Callback cb) = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get doxygen comments on these methods?

virtual void publish(std_msgs::msg::Int64 msg) = 0;
virtual ~MiddlewareHandle() = default;
};

/**
* @brief Incrementer constructor
*
* @param[in] node Shared pointer to a ROS node
* @param[in] in_topic ROS topic to the Incrementer subscribes to
* @param[in] out_topic ROS topic that the Incrementer publishes to
* @param[in] mw Unique pointer to a MiddlewareHandle object
*/
Incrementer(std::shared_ptr<rclcpp::Node> node, std::string const& in_topic,
std::string const& out_topic);
Incrementer(std::unique_ptr<MiddlewareHandle> mw);

private:
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_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
56 changes: 56 additions & 0 deletions naive/include/naive/ros_middleware.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*********************************************************************
* Software License Agreement (BSD License)
*
* Copyright (c) 2022, PickNik, LLC.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following
* disclaimer in the documentation and/or other materials provided
* with the distribution.
* * Neither the name of PickNik nor the names of its
* contributors may be used to endorse or promote products derived
* from this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
* FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
* COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
* BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
* CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*********************************************************************/
#pragma once

#include <string>

#include <rclcpp/rclcpp.hpp>
#include <naive/incrementer.hpp>

#include <std_msgs/msg/int64.hpp>

class RosMiddleware : public Incrementer::MiddlewareHandle {
public:
RosMiddleware(const rclcpp::Node::SharedPtr node_handle, std::string in_topic,
const std::string& out_topic);
void registerCallback(Callback cb) override;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doxygen

void publish(std_msgs::msg::Int64 msg) override;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rclcpp::Publisher<std_msgs::msg::Int64>::SharedPtr publisher_;
};
23 changes: 8 additions & 15 deletions naive/src/incrementer.cpp
Original file line number Diff line number Diff line change
@@ -1,25 +1,18 @@
#include "naive/incrementer.hpp"
#include "naive/ros_middleware.hpp"

#include <memory>

#include <rclcpp/rclcpp.hpp>

#include <std_msgs/msg/int64.hpp>

namespace {
constexpr auto queue_size = 10;
}

Incrementer::Incrementer(std::shared_ptr<rclcpp::Node> node,
std::string const& in_topic,
std::string const& out_topic)
: node_{std::move(node)},
pub_{
node_->create_publisher<std_msgs::msg::Int64>(out_topic, queue_size)},
sub_{node_->create_subscription<std_msgs::msg::Int64>(
in_topic, queue_size,
[this](std_msgs::msg::Int64::UniquePtr const msg) {
Incrementer::Incrementer(std::unique_ptr<MiddlewareHandle> mw)
: mw_{std::move(mw)}
{
mw_->registerCallback([this](const std_msgs::msg::Int64::SharedPtr msg) {
std_msgs::msg::Int64 incremented;
incremented.data = msg->data + 1;
pub_->publish(incremented);
})} {}
mw_->publish(incremented);
});
}
7 changes: 6 additions & 1 deletion naive/src/node.cpp
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
#include "naive/incrementer.hpp"
#include "naive/ros_middleware.hpp"

#include <rclcpp/rclcpp.hpp>

#include <memory>

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")};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rclcpp::spin(node);
rclcpp::shutdown();

return 0;
}
18 changes: 18 additions & 0 deletions naive/src/ros_middleware.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#include <string>

#include <rclcpp/rclcpp.hpp>
#include <std_msgs/msg/int64.hpp>

#include <naive/ros_middleware.hpp>

RosMiddleware::RosMiddleware(const rclcpp::Node::SharedPtr node_handle, std::string in_topic,
const std::string& out_topic)
: node_handle_{node_handle},
in_topic_{std::move(in_topic)},
publisher_{node_handle_->create_publisher<std_msgs::msg::Int64>(out_topic, QUEUE_SIZE)} {}

void RosMiddleware::registerCallback(Callback cb) {
subscriber_ = node_handle_->create_subscription<std_msgs::msg::Int64>(in_topic_, QUEUE_SIZE, cb);
}

void RosMiddleware::publish(std_msgs::msg::Int64 msg) { publisher_->publish(msg); }
67 changes: 67 additions & 0 deletions naive/test/incrementer_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// C++ Standard Library
#include <memory>

// Gtest
#include <gmock/gmock.h>
#include <gtest/gtest.h>

// ROS
#include <std_msgs//msg/int64.hpp>

// Code being tested
#include "naive/incrementer.hpp"

using ::testing::_;
using ::testing::Invoke;

/**
\brief Mock middleware handle, allowing us to define test behavior.
*/
struct MockMiddleware : public Incrementer::MiddlewareHandle {
MOCK_METHOD1(registerCallback, void(Callback cb));
MOCK_METHOD1(publish, void(std_msgs::msg::Int64 msg));
};

/** \brief Callback is given to subscriber on construction. */
TEST(IncrementerTests, RegisterSubscriber) {
// GIVEN a middleware handle
auto mw = std::make_unique<MockMiddleware>();
// THEN a callback should be registered for topic subscription.
EXPECT_CALL(*mw, registerCallback(_)).Times(1);
// WHEN the incrementer is created with that handle
Incrementer incrementer{std::move(mw)};
}

/** \brief Incremented number is published when a message is received. */
TEST(IncrementerTests, PublishIncrement) {
// GIVEN a middleware handle
auto mw = std::make_unique<MockMiddleware>();
/*
Configure the mock handle to capture the incrementer callback so we can
publish a message to it as the test trigger event. This is analogous to ROS
publishing a message to a topic if the ROS middleware handle was used.
We know that registerCallback will be called from a previous test so we
do not need to test that again, only define the behavior.
*/
Incrementer::MiddlewareHandle::Callback callback;
ON_CALL(*mw, registerCallback(_))
.WillByDefault(Invoke(
[&](Incrementer::MiddlewareHandle::Callback cb) { callback = cb; }));

// THEN the incrementer should publish an incremented message.
std_msgs::msg::Int64 expected;
expected.data = 8;
EXPECT_CALL(*mw, publish(expected)).Times(1);
/* Create the System Under Test (SUT)*/
Incrementer incrementer{std::move(mw)};

// WHEN the middleware publishes a message to the callback
std_msgs::msg::Int64::SharedPtr msg = std::make_shared<std_msgs::msg::Int64>();
msg->data = 7;
callback(msg);
}

int main(int argc, char **argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
}
29 changes: 29 additions & 0 deletions naive_superclass/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
cmake_minimum_required(VERSION 3.16)
project(naive_superclass)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra -Wpedantic -Werror)
endif()

# find dependencies
find_package(ament_cmake REQUIRED)
find_package(rclcpp REQUIRED)
find_package(std_msgs REQUIRED)

add_executable(naive_superclass src/node.cpp src/incrementer.cpp)
target_include_directories(naive_superclass PRIVATE include)
ament_target_dependencies(naive_superclass rclcpp std_msgs)

install(TARGETS
naive_superclass
DESTINATION lib/${PROJECT_NAME}
)

if(BUILD_TESTING)
find_package(ament_cmake_gtest REQUIRED)
ament_add_gtest(incrementer_test test/incrementer_test.cpp src/incrementer.cpp TIMEOUT 5)
target_include_directories(incrementer_test PRIVATE include)
ament_target_dependencies(incrementer_test rclcpp std_msgs)
endif()

ament_package()
60 changes: 60 additions & 0 deletions naive_superclass/include/naive_superclass/incrementer.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*********************************************************************
* Software License Agreement (BSD License)
*
* Copyright (c) 2022, PickNik, LLC.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following
* disclaimer in the documentation and/or other materials provided
* with the distribution.
* * Neither the name of PickNik nor the names of its
* contributors may be used to endorse or promote products derived
* from this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
* FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
* COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
* BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
* CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*********************************************************************/
#pragma once

#include <memory>
#include <string>

#include <rclcpp/rclcpp.hpp>

#include <std_msgs/msg/int64.hpp>

class Incrementer : public rclcpp::Node {
public:
/**
* @brief Incrementer constructor
*
* @param[in] mw Unique pointer to a MiddlewareHandle object
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update

*/
Incrementer(const std::string node_name, const std::string in_topic, const std::string out_topic);

void callback(const std_msgs::msg::Int64::SharedPtr msg);

int getQueueSize(){return QUEUE_SIZE;}

private:
static constexpr int QUEUE_SIZE = 10;
rclcpp::Publisher<std_msgs::msg::Int64>::SharedPtr publisher_;
rclcpp::Subscription<std_msgs::msg::Int64>::SharedPtr subscriber_;
};
18 changes: 18 additions & 0 deletions naive_superclass/package.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?xml version="1.0"?>
<?xml-model href="http://download.ros.org/schema/package_format3.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
<package format="3">
<name>naive_superclass</name>
<version>0.0.0</version>
<description>TODO: Package description</description>
<maintainer email="[email protected]">bilal</maintainer>
<license>TODO: License declaration</license>

<buildtool_depend>ament_cmake</buildtool_depend>

<test_depend>ament_lint_auto</test_depend>
<test_depend>ament_lint_common</test_depend>

<export>
<build_type>ament_cmake</build_type>
</export>
</package>
18 changes: 18 additions & 0 deletions naive_superclass/src/incrementer.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#include "naive_superclass/incrementer.hpp"

#include <rclcpp/rclcpp.hpp>

#include <std_msgs/msg/int64.hpp>

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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); });

}

void Incrementer::callback(const std_msgs::msg::Int64::SharedPtr msg) {
std_msgs::msg::Int64 incremented;
incremented.data = msg->data + 1;
publisher_->publish(incremented);
}
14 changes: 14 additions & 0 deletions naive_superclass/src/node.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#include "naive_superclass/incrementer.hpp"

#include <rclcpp/rclcpp.hpp>

#include <memory>

int main(int argc, char **argv) {
rclcpp::init(argc, argv);
auto const node = std::make_shared<Incrementer>("incrementer", "in", "out");
rclcpp::spin(node);
rclcpp::shutdown();

return 0;
}
Loading
Loading