Skip to content

expose complex data types through interfaces #490

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

Closed
Karsten1987 wants to merge 2 commits into from
Closed

expose complex data types through interfaces #490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd371a1
expose complex data types through interfaces
Karsten1987 Aug 10, 2021
d33ece9
linters
Karsten1987 Aug 10, 2021
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
47 changes: 32 additions & 15 deletions hardware_interface/include/hardware_interface/handle.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,17 @@ class ReadOnlyHandle
ReadOnlyHandle(

@destogl destogl Aug 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not also add "type" string as a member of handles so we can check when creating "Loaned*Interfaces" that the data types are not mixed. See also comment below.

const std::string & name,
const std::string & interface_name,
double * value_ptr = nullptr)
void * value_ptr = nullptr)
: name_(name), interface_name_(interface_name), value_ptr_(value_ptr)
{
}
{}

explicit ReadOnlyHandle(const std::string & interface_name)
: interface_name_(interface_name), value_ptr_(nullptr)
{
}
{}

explicit ReadOnlyHandle(const char * interface_name)
: interface_name_(interface_name), value_ptr_(nullptr)
{
}
{}

ReadOnlyHandle(const ReadOnlyHandle & other) = default;

Expand Down Expand Up @@ -73,16 +70,17 @@ class ReadOnlyHandle
return name_ + "/" + interface_name_;
}

double get_value() const
template<class DataT>
DataT get_value() const

@destogl destogl Aug 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably limit this method to specific types (as described in overview). This means writing each one of those (it should be only +30 lines of code)

{
THROW_ON_NULLPTR(value_ptr_);
return *value_ptr_;
return *static_cast<DataT *>(value_ptr_);
}

protected:
std::string name_;
std::string interface_name_;
double * value_ptr_;
void * value_ptr_;
};

class ReadWriteHandle : public ReadOnlyHandle
Expand All @@ -91,7 +89,7 @@ class ReadWriteHandle : public ReadOnlyHandle
ReadWriteHandle(
const std::string & name,
const std::string & interface_name,
double * value_ptr = nullptr)
void * value_ptr = nullptr)
: ReadOnlyHandle(name, interface_name, value_ptr)
{}

Expand All @@ -113,26 +111,35 @@ class ReadWriteHandle : public ReadOnlyHandle

virtual ~ReadWriteHandle() = default;

void set_value(double value)
template<class DataT>
void set_value(DataT value)
{
THROW_ON_NULLPTR(this->value_ptr_);
*this->value_ptr_ = value;
*static_cast<DataT *>(this->value_ptr_) = value;
}
};

class StateInterface : public ReadOnlyHandle
{
public:
using ReadOnlyHandle::ReadOnlyHandle;

StateInterface(const StateInterface & other) = default;

StateInterface(StateInterface && other) = default;

using ReadOnlyHandle::ReadOnlyHandle;
template<class DataT = double>
DataT get_value() const
{
return ReadOnlyHandle::get_value<DataT>();
}
};

class CommandInterface : public ReadWriteHandle
{
public:
using ReadWriteHandle::ReadWriteHandle;

/// CommandInterface copy constructor is actively deleted.
/**
* Command interfaces are having a unique ownership and thus
Expand All @@ -143,7 +150,17 @@ class CommandInterface : public ReadWriteHandle

CommandInterface(CommandInterface && other) = default;

using ReadWriteHandle::ReadWriteHandle;
template<class DataT = double>
DataT get_value() const
{
return ReadWriteHandle::get_value<DataT>();
}

template<class DataT = double>
void set_value(DataT value)
{
return ReadWriteHandle::set_value<DataT>(value);
}
};

} // namespace hardware_interface
Expand Down
10 changes: 6 additions & 4 deletions hardware_interface/include/hardware_interface/loaned_command_interface.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,16 @@ class LoanedCommandInterface
return command_interface_.get_full_name();
}

void set_value(double val)
template<class DataT = double>

@destogl destogl Aug 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the constructor of "Loaned*Interfaces" we should also provide a type as a string and cross-check it with the Handle type. That way we are sure there is no access violation (double - int example)

void set_value(DataT val)
{
command_interface_.set_value(val);
command_interface_.set_value<DataT>(val);
}

double get_value() const
template<class DataT = double>
DataT get_value() const
{
return command_interface_.get_value();
return command_interface_.get_value<DataT>();
}

protected:
Expand Down
5 changes: 3 additions & 2 deletions hardware_interface/include/hardware_interface/loaned_state_interface.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,10 @@ class LoanedStateInterface
return state_interface_.get_full_name();
}

double get_value() const
template<class DataT = double>
DataT get_value() const
{
return state_interface_.get_value();
return state_interface_.get_value<DataT>();
}

protected:
Expand Down
19 changes: 18 additions & 1 deletion hardware_interface/test/test_component_interfaces.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ using namespace ::testing; // NOLINT
namespace test_components
{

struct POD

@bmagyar bmagyar Aug 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd really like to avoid this exact use case. If we start supporting random types, hardware manufacturers will be inclined to start baking their own custom structs into their system components which will force users to pair their controllers with that specific hardware. The type erasure technique is nice but I think the framework should have tight control over what exactly is supported and what isn't.

Parsing "custom_complex_pod" as interface type should fail upfront IMO and

 template<class DataT = double>
  DataT get_value() const
  {
    return ReadWriteHandle::get_value<DataT>();
  }

should only be allowed if DataT is in an approved set. Probably at the C++ level this means copying that code 4-5 times but that doesn't sound too bad.

{
std::string str;
int i;
float f;
};

Comment on lines +39 to +45

@destogl destogl Aug 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should allow to create such structures in the framework. I see potentially very-many problems is users can put anything as the interface values.

We could end up with zillion different structures for each industrial-manipulator realizing actually simple "position-velocity-acceleration" combination of interfaces.

This kind of complex structures (without strings) should be realized by using Semantic Components. This was we make them very visible and explicit.

class DummyActuator : public hardware_interface::ActuatorInterface
{
hardware_interface::return_type configure(
Expand Down Expand Up @@ -125,6 +132,10 @@ class DummySensor : public hardware_interface::SensorInterface
std::vector<hardware_interface::StateInterface> state_interfaces;
state_interfaces.emplace_back(
hardware_interface::StateInterface("joint1", "voltage", &voltage_level_));
// Any non-double data type can be exposed
state_interfaces.emplace_back(
hardware_interface::StateInterface(
"joint1", "custom_complex_pod", &complex_data_));

return state_interfaces;
}
Expand Down Expand Up @@ -157,6 +168,7 @@ class DummySensor : public hardware_interface::SensorInterface

private:
double voltage_level_ = 0x666;
POD complex_data_ = {"abc", 123, 456.f};
};

class DummySystem : public hardware_interface::SystemInterface
Expand Down Expand Up @@ -383,10 +395,15 @@ TEST(TestComponentInterfaces, dummy_sensor)
EXPECT_EQ(hardware_interface::return_type::OK, sensor_hw.configure(mock_hw_info));

auto state_interfaces = sensor_hw.export_state_interfaces();
ASSERT_EQ(1u, state_interfaces.size());
ASSERT_EQ(2u, state_interfaces.size());
EXPECT_EQ("joint1", state_interfaces[0].get_name());
EXPECT_EQ("voltage", state_interfaces[0].get_interface_name());
EXPECT_EQ(0x666, state_interfaces[0].get_value());
EXPECT_EQ("joint1", state_interfaces[1].get_name());
EXPECT_EQ("custom_complex_pod", state_interfaces[1].get_interface_name());
EXPECT_EQ("abc", state_interfaces[1].get_value<test_components::POD>().str);
EXPECT_EQ(123, state_interfaces[1].get_value<test_components::POD>().i);
EXPECT_FLOAT_EQ(456.f, state_interfaces[1].get_value<test_components::POD>().f);
}

TEST(TestComponentInterfaces, dummy_system)
Expand Down
23 changes: 23 additions & 0 deletions hardware_interface/test/test_handle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
// limitations under the License.

#include <gmock/gmock.h>
#include <string>

#include "hardware_interface/handle.hpp"

using hardware_interface::CommandInterface;
Expand All @@ -33,6 +35,27 @@ TEST(TestHandle, command_interface)
EXPECT_DOUBLE_EQ(interface.get_value(), 0.0);
}

TEST(TestHandle, complex_command_interface)
{
struct Complex
{
std::string str;
};

std::string value = "Hello Complex Interface";
Complex c{value};

CommandInterface interface{JOINT_NAME, FOO_INTERFACE, &c};
EXPECT_EQ(interface.get_value<std::string>(), value);
// TODO(karsten1987): Make get_value (implicit as double) type safe on caller side.
// interface.get_value() is now undefined behavior.
// EXPECT_DOUBLE_EQ(interface.get_value(), 0.0);

value = "Hello Modified Complex Interface";
EXPECT_NO_THROW(interface.set_value<std::string>(value));
EXPECT_EQ(interface.get_value<std::string>(), value);
}

TEST(TestHandle, state_interface)
{
double value = 1.337;
Expand Down
15 changes: 10 additions & 5 deletions transmission_interface/include/transmission_interface/handle.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,20 @@ namespace transmission_interface
class ActuatorHandle : public hardware_interface::ReadWriteHandle
{
public:
void set_value(double value)
{
return hardware_interface::ReadWriteHandle::set_value<double>(value);
}
double get_value()
{
return hardware_interface::ReadWriteHandle::get_value<double>();
}

using hardware_interface::ReadWriteHandle::ReadWriteHandle;
};

/** A handle used to get and set a value on a given joint interface. */
class JointHandle : public hardware_interface::ReadWriteHandle
{
public:
using hardware_interface::ReadWriteHandle::ReadWriteHandle;
};
using JointHandle = ActuatorHandle;

} // namespace transmission_interface

Expand Down