Skip to content

Add unit testing to robot hardware #109

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.

Sign up for GitHub

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

Merged
bmagyar merged 15 commits into from
Jun 25, 2020
Merged

Add unit testing to robot hardware #109

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4f0e844
Use gmock
bmagyar Jun 20, 2020
bfb3b20
Add Bence as maintainer
bmagyar Jun 20, 2020
14771a0
Test invalid handles
bmagyar Jun 20, 2020
c96b257
Add handle validation to RobotHardware
bmagyar Jun 20, 2020
5f78e12
Test proper handles and double registering handles
bmagyar Jun 20, 2020
5c47656
Test name and blanket handle getters
bmagyar Jun 20, 2020
b09755d
Test named handle getters
bmagyar Jun 20, 2020
fdb6354
Refactor registering second handles in tests, add check for getting s…
bmagyar Jun 20, 2020
ca06e6b
Make linters happy
bmagyar Jun 20, 2020
13c6a53
Check that returned pointers are not null
bmagyar Jun 25, 2020
4614ce8
Revert unsollicited style changes
bmagyar Jun 25, 2020
32fc92e
Fix license in test
bmagyar Jun 25, 2020
c561815
Update hardware_interface/CMakeLists.txt
bmagyar Jun 25, 2020
7e4aa2a
Update ros2ci workflow versions
bmagyar Jun 25, 2020
30476af
use all latest on workflow
bmagyar Jun 25, 2020
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
8 changes: 7 additions & 1 deletion hardware_interface/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,15 @@ if(BUILD_TESTING)
find_package(ament_lint_auto REQUIRED)
ament_lint_auto_find_test_dependencies()

ament_add_gtest(test_macros test/test_macros.cpp)
find_package(ament_cmake_gmock REQUIRED)
ament_add_gmock(test_macros test/test_macros.cpp)
target_include_directories(test_macros PRIVATE include)
ament_target_dependencies(test_macros rcpputils)

ament_add_gmock(test_robot_hardware_interfaces test/test_robot_hardware_interface.cpp)
target_include_directories(test_robot_hardware_interfaces PRIVATE include)
target_link_libraries(test_robot_hardware_interfaces hardware_interface)

Comment thread
bmagyar marked this conversation as resolved.
Outdated
endif()

ament_export_include_directories(
Expand Down
16 changes: 7 additions & 9 deletions hardware_interface/include/hardware_interface/joint_command_handle.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,21 +39,19 @@ class JointCommandHandle
* \param cmd A pointer to the storage for this joint's command
*/
HARDWARE_INTERFACE_PUBLIC
JointCommandHandle(
const std::string & name,
double * cmd);
JointCommandHandle(const std::string & name, double * cmd);

HARDWARE_INTERFACE_PUBLIC
const std::string &
get_name() const;
const std::string & get_name() const;

HARDWARE_INTERFACE_PUBLIC
double
get_cmd() const;
double get_cmd() const;

HARDWARE_INTERFACE_PUBLIC
void
set_cmd(double cmd);
void set_cmd(double cmd);

HARDWARE_INTERFACE_PUBLIC
bool valid_pointers() const;

@Karsten1987 Karsten1987 Jun 25, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

that function name should be more precise. valid_pointers doesn't really say anything about what that function does, meaning which pointers are being checked.

@Karsten1987 Karsten1987 Jun 25, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also, the function states pointers as in multiple pointers, the implementation does however only check a single cmd_.

@bmagyar bmagyar Jun 25, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have the same function implemented in all the other handles and I call this from the template function so they should all be called the same. TBH a proper solution would see inheriting such an interface to really ensure this function existing the same way for all


private:
std::string name_;
Expand Down
19 changes: 8 additions & 11 deletions hardware_interface/include/hardware_interface/joint_state_handle.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,26 +41,23 @@ class JointStateHandle
*/
HARDWARE_INTERFACE_PUBLIC
JointStateHandle(
const std::string & name,
const double * pos,
const double * vel,
const std::string & name, const double * pos, const double * vel,
const double * eff);

HARDWARE_INTERFACE_PUBLIC
const std::string &
get_name() const;
const std::string & get_name() const;

HARDWARE_INTERFACE_PUBLIC
double
get_position() const;
double get_position() const;

HARDWARE_INTERFACE_PUBLIC
double
get_velocity() const;
double get_velocity() const;

HARDWARE_INTERFACE_PUBLIC
double
get_effort() const;
double get_effort() const;

HARDWARE_INTERFACE_PUBLIC
bool valid_pointers() const;

private:
std::string name_;
Expand Down
13 changes: 6 additions & 7 deletions hardware_interface/include/hardware_interface/operation_mode_handle.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,16 @@ class OperationModeHandle
* \param mode A pointer to the storage for this hardware's operation mode
*/
HARDWARE_INTERFACE_PUBLIC
OperationModeHandle(
const std::string & name,
OperationMode * mode);
OperationModeHandle(const std::string & name, OperationMode * mode);

HARDWARE_INTERFACE_PUBLIC
const std::string &
get_name() const;
const std::string & get_name() const;

HARDWARE_INTERFACE_PUBLIC
void
set_mode(OperationMode mode);
void set_mode(OperationMode mode);

HARDWARE_INTERFACE_PUBLIC
bool valid_pointers() const;

private:
std::string name_;
Expand Down
3 changes: 2 additions & 1 deletion hardware_interface/package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@
<description>ROS2 ros_control hardware interface</description>

<maintainer email="karsten@osrfoundation.org">Karsten Knese</maintainer>
<maintainer email="bence.magyar.robotics@gmail.com">Bence Magyar</maintainer>
<license>Apache License 2.0</license>

<buildtool_depend>ament_cmake</buildtool_depend>

<depend>rclcpp</depend>
<depend>rcpputils</depend>

<test_depend>ament_cmake_gtest</test_depend>
<test_depend>ament_cmake_gmock</test_depend>
<test_depend>ament_lint_auto</test_depend>
<test_depend>ament_lint_common</test_depend>

Expand Down
27 changes: 14 additions & 13 deletions hardware_interface/src/joint_command_handle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,38 +21,39 @@

namespace hardware_interface
{

JointCommandHandle::JointCommandHandle()
: name_(),
cmd_(nullptr)
{}
Comment on lines -26 to -28

@Karsten1987 Karsten1987 Jun 23, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why these changes? They seem unrelated

: name_(), cmd_(nullptr)
{
}

JointCommandHandle::JointCommandHandle(
const std::string & name,
double * cmd)
Comment thread
bmagyar marked this conversation as resolved.
JointCommandHandle::JointCommandHandle(const std::string & name, double * cmd)
: name_(name), cmd_(cmd)
{
THROW_ON_NULLPTR(cmd)
}

const std::string &
JointCommandHandle::get_name() const
const std::string & JointCommandHandle::get_name() const
{
return name_;
}

double
JointCommandHandle::get_cmd() const
double JointCommandHandle::get_cmd() const
{
THROW_ON_NULLPTR(cmd_)
Comment thread
bmagyar marked this conversation as resolved.

return *cmd_;
}

void
JointCommandHandle::set_cmd(double cmd)
void JointCommandHandle::set_cmd(double cmd)
{
THROW_ON_NULLPTR(cmd_)

* cmd_ = cmd;
}

bool JointCommandHandle::valid_pointers() const
{
return cmd_;
}

} // namespace hardware_interface
31 changes: 14 additions & 17 deletions hardware_interface/src/joint_state_handle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,13 @@

namespace hardware_interface
{

JointStateHandle::JointStateHandle()
: name_(),
pos_(nullptr),
vel_(nullptr),
eff_(nullptr)
{}
Comment on lines -26 to -30

@Karsten1987 Karsten1987 Jun 23, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why these changes? They seem unrelated

@bmagyar bmagyar Jun 25, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I really don't know, as I put in the opening post,

There are some changes in this PR that should be credited to uncrustify...

Sure, I'll revert 'em

: name_(), pos_(nullptr), vel_(nullptr), eff_(nullptr)
{
}

JointStateHandle::JointStateHandle(
const std::string & name,
const double * pos,
const double * vel,
const std::string & name, const double * pos, const double * vel,
Comment thread
bmagyar marked this conversation as resolved.
Outdated
const double * eff)
: name_(name), pos_(pos), vel_(vel), eff_(eff)
{
Expand All @@ -41,33 +36,35 @@ JointStateHandle::JointStateHandle(
THROW_ON_NULLPTR(eff)
}

const std::string &
JointStateHandle::get_name() const
const std::string & JointStateHandle::get_name() const
{
return name_;
}

double
JointStateHandle::get_position() const
double JointStateHandle::get_position() const
{
THROW_ON_NULLPTR(pos_)

return *pos_;
}

double
JointStateHandle::get_velocity() const
double JointStateHandle::get_velocity() const
{
THROW_ON_NULLPTR(vel_)

return *vel_;
}

double
JointStateHandle::get_effort() const
double JointStateHandle::get_effort() const
{
THROW_ON_NULLPTR(eff_)

return *eff_;
}

bool JointStateHandle::valid_pointers() const
{
return pos_ && vel_ && eff_;
}

} // namespace hardware_interface
27 changes: 13 additions & 14 deletions hardware_interface/src/operation_mode_handle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,33 +21,32 @@

namespace hardware_interface
{

OperationModeHandle::OperationModeHandle()
: name_(),
mode_(nullptr)
{}

OperationModeHandle::OperationModeHandle(
const std::string & name,
OperationMode * mode)
: name_(name),
mode_(mode)
: name_(), mode_(nullptr)
{
}

OperationModeHandle::OperationModeHandle(const std::string & name, OperationMode * mode)
: name_(name), mode_(mode)
{
THROW_ON_NULLPTR(mode)
}

const std::string &
OperationModeHandle::get_name() const
const std::string & OperationModeHandle::get_name() const
{
return name_;
}

void
OperationModeHandle::set_mode(OperationMode mode)
void OperationModeHandle::set_mode(OperationMode mode)
{
THROW_ON_NULLPTR(mode_)

* mode_ = mode;
}

bool OperationModeHandle::valid_pointers() const
{
return mode_;
}

} // namespace hardware_interface
Loading