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

Automatic Creation of Handles in HW, Adding Getters/Setters (variant support) #1688

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

mamueluth
Copy link
Member

@mamueluth mamueluth commented Aug 16, 2024

This PR is the fourth part of multiple breaking down #1240 in smaller changes. For an overview explanation and a lot of comments/discussion, please refer to #1240.
This PR is the main work of preparing the Handles, ControllerManager (cm) and ResourceManager (rm) for the full variant support. This PR introduces the creation of Command-/StateInterfaces (ci/si) by the Framework itself rather than by the a user who want to implement a HardwareInterface. The ci/si are created by the base class e.g. ActuatorInterface, SensorInterface or SystemInterface. The ci/si are now created as shared_ptr and a shared_ptr is passed to rm. This needs to be done since in future the pointer of the value from each ci/si is replaced by a value inside the handle itself. In order for the hardware still be able to set/get values from a handle they need to have access to them. Since we decided not to share back loans to the hardware as is done with controllers we now pass shared_ptrs to rm and keep a shared_ptr of the ci/si in the hardwares base class (ActuatorInterface, SensorInterface or SystemInterface) inside a unordered_map. The hardware can then access the ci/si through get_state() and set_state()methods based on the interfaces full name.

NOTE: Everything is fully backward compatible at this point. The old way of exporting and creating handles with pointers is still available. Variant supports only double at this point. Only if InterfaceDescription for creation of Hanles is used the ptr is set to the internal variant.

  • @bmagyar it will get smaller as soon as the parts before is merged but it will still remain a quite big PR. Can you please check if you think we should split this up further?

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

@mamueluth I have a question wrt to new way to exporting interfaces, how are they going to integrate now if we use Transmissions?, because earlier, the JointHandle of Transmission and that of the StateInterface or CommandInterface share the same variable pointer, so, Just calling the methods actuator_to_joint and joint_to_actuator to have the transformations and it would be directly reflected on the respective interface data I believe the current approach is they have pointers to separate variables and then they are swapped later, so I believe we do it same way but using the getter and setter methods?

Could you please explain what would be the procedure for those who use transmissions?. I'm asking because we are one of the active users of Transmissions on our robots

Copy link
Contributor

mergify bot commented Aug 21, 2024

This pull request is in conflict. Could you fix it @mamueluth?

doc/Iron.rst Outdated Show resolved Hide resolved
doc/Iron.rst Outdated Show resolved Hide resolved
doc/Iron.rst Outdated

* ``Command-/StateInterfaces`` are now created and exported automatically by the framework via the ``on_export_command_interfaces()`` or ``on_export_state_interfaces()`` methods based on the interfaces defined in the ``ros2_control`` XML-tag, which gets parsed and the ``InterfaceDescription`` is created accordingly (check the `hardware_info.hpp <https://github.com/ros-controls/ros2_control/tree/{REPOS_FILE_BRANCH}/hardware_interface/include/hardware_interface/hardware_info.hpp>`__).
* The memory for storing the value of a ``Command-/StateInterfaces`` is no longer allocated in the hardware but instead in the ``Command-/StateInterfaces`` itself.
* To access the automatically created ``Command-/StateInterfaces`` we provide the ``std::unordered_map<std::string, InterfaceDescription>``, where the string is the fully qualified name of the interface and the ``InterfaceDescription`` is the configuration of the interface. The ``std::unordered_map<>`` are divided into ``type_state_interfaces_`` and ``type_command_interfaces_`` where type can be: ``joint``, ``sensor``, ``gpio`` and ``unlisted``. E.g. the ``CommandInterfaces`` for all joints can be found in the ``joint_command_interfaces_`` map. The ``unlisted`` includes all interfaces not listed in the ``ros2_control`` XML-tag but were created by overriding the ``export_command_interfaces_2()`` or ``export_state_interfaces_2()`` function by creating some custom ``Command-/StateInterfaces``.
Copy link
Member

@bmagyar bmagyar Aug 26, 2024

Choose a reason for hiding this comment

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

The _2 seems fishy here. There are more occurences of it below

Copy link
Member Author

Choose a reason for hiding this comment

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

Have to change this

doc/Iron.rst Outdated Show resolved Hide resolved
doc/Iron.rst Outdated Show resolved Hide resolved
doc/Iron.rst Outdated Show resolved Hide resolved
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 93.04590% with 50 lines in your changes missing coverage. Please review.

Project coverage is 87.16%. Comparing base (eb4c19d) to head (b1e490b).

Files with missing lines Patch % Lines
...ce/include/hardware_interface/system_interface.hpp 85.05% 12 Missing and 1 partial ⚠️
...e/test/test_component_interfaces_custom_export.cpp 88.88% 13 Missing ⚠️
hardware_interface/src/resource_manager.cpp 75.00% 10 Missing and 2 partials ⚠️
...dware_interface/test/test_component_interfaces.cpp 98.11% 6 Missing ⚠️
.../include/hardware_interface/actuator_interface.hpp 93.75% 0 Missing and 4 partials ⚠️
...ce/include/hardware_interface/sensor_interface.hpp 93.54% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1688      +/-   ##
==========================================
+ Coverage   86.77%   87.16%   +0.38%     
==========================================
  Files         116      118       +2     
  Lines       10703    11360     +657     
  Branches      981     1050      +69     
==========================================
+ Hits         9288     9902     +614     
- Misses       1062     1096      +34     
- Partials      353      362       +9     
Flag Coverage Δ
unittests 87.16% <93.04%> (+0.38%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
..._interface/include/hardware_interface/actuator.hpp 100.00% <ø> (ø)
...re_interface/include/hardware_interface/handle.hpp 100.00% <100.00%> (ø)
...re_interface/include/hardware_interface/sensor.hpp 50.00% <ø> (ø)
...re_interface/include/hardware_interface/system.hpp 100.00% <ø> (ø)
hardware_interface/src/actuator.cpp 74.82% <100.00%> (+2.64%) ⬆️
hardware_interface/src/sensor.cpp 72.26% <100.00%> (+1.73%) ⬆️
hardware_interface/src/system.cpp 74.82% <100.00%> (+2.64%) ⬆️
...rface/test/mock_components/test_generic_system.cpp 99.75% <ø> (ø)
hardware_interface/test/test_components.hpp 100.00% <100.00%> (ø)
...ce/include/hardware_interface/sensor_interface.hpp 93.75% <93.54%> (-0.70%) ⬇️
... and 5 more

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

A review of implementation within the component interfaces

Comment on lines +131 to +158
/**
* Import the InterfaceDescription for the StateInterfaces from the HardwareInfo.
* Separate them into the possible types: Joint and store them.
*/
virtual void import_state_interface_descriptions(const HardwareInfo & hardware_info)
{
auto joint_state_interface_descriptions =
parse_state_interface_descriptions(hardware_info.joints);
for (const auto & description : joint_state_interface_descriptions)
{
joint_state_interfaces_.insert(std::make_pair(description.get_name(), description));
}
}

/**
* Import the InterfaceDescription for the CommandInterfaces from the HardwareInfo.
* Separate them into the possible types: Joint and store them.
*/
virtual void import_command_interface_descriptions(const HardwareInfo & hardware_info)
{
auto joint_command_interface_descriptions =
parse_command_interface_descriptions(hardware_info.joints);
for (const auto & description : joint_command_interface_descriptions)
{
joint_command_interfaces_.insert(std::make_pair(description.get_name(), description));
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should move these methods outside of the interfaces and have them in the helper headers instead of repeating the same in all the HW component interfaces.

Copy link
Member

Choose a reason for hiding this comment

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

they were declared virtual I guess in case people want to override them 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Well these methods are pretty much repeating in Sensor, System and Actuator, and if they want to override may be they can do it by overriding the on_init right?.

Anway, if repeating code is not an issue for you guys, fine for me :)

Copy link
Member Author

Choose a reason for hiding this comment

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

My initial intend was to provide default functionality by the framework but give the user possibility to override the functions for exporting the stateinterfaces in case they want to do some hardware specific stuff or order them differently. I am not sure which way we want to go, i would be interested in your opinions on this.

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would separate them into helper methods that would work for joints, gpois etc, and reuse them across all the hardware interfaces, and if the user wants to override it, then he can do it in the derived class on_init method

This way you would be able to automatically export all types of interfaces.

Anyway, this is just an implementation detail. What do you think?

Comment on lines 184 to 198
/**
* Override this method to export custom StateInterfaces which are not defined in the URDF file.
* Those interfaces will be added to the unlisted_state_interfaces_ map.
*
* Note method name is going to be changed to export_state_interfaces() as soon as the deprecated
* version is removed.
*
* \return vector of descriptions to the unlisted StateInterfaces
*/
virtual std::vector<hardware_interface::InterfaceDescription>
export_state_interface_descriptions()
{
// return empty vector by default.
return {};
}
Copy link
Member

@saikishor saikishor Aug 29, 2024

Choose a reason for hiding this comment

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

Do we need this part of the code?. I'm sure we would need functionality to expose more interfaces. In fact, Our usecase needs this kind of extensibility. I'm just not sure if this is the way to do it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we need for custom exported interfaces. If you have a better idea how to do it i would be interested.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, we can leave it like this, it's fine.

@mamueluth what do you think about renaming this method to have export_extra_state_interface_descriptions, so it is descriptive enough, so the users don't get the wrong idea and export everything again here?

If you think, it is fine to leave it like this, I'm fine with it

Copy link
Member Author

Choose a reason for hiding this comment

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

No i think you are right bence suggested the same to me and i fully agree. should be changed. Bence suggested: export_unlisted_state_interface_descriptions()

Copy link
Member

Choose a reason for hiding this comment

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

Sure, you can choose between these names :)

Comment on lines 207 to 237
virtual std::vector<std::shared_ptr<StateInterface>> on_export_state_interfaces()
{
// import the unlisted interfaces
std::vector<hardware_interface::InterfaceDescription> unlisted_interface_descriptions =
export_state_interface_descriptions();

std::vector<std::shared_ptr<StateInterface>> state_interfaces;
state_interfaces.reserve(
unlisted_interface_descriptions.size() + joint_state_interfaces_.size());

// add InterfaceDescriptions and create the StateInterfaces from the descriptions and add to
// maps.
for (const auto & description : unlisted_interface_descriptions)
{
auto name = description.get_name();
unlisted_state_interfaces_.insert(std::make_pair(name, description));
auto state_interface = std::make_shared<StateInterface>(description);
actuator_states_.insert(std::make_pair(name, state_interface));
unlisted_states_.push_back(state_interface);
state_interfaces.push_back(state_interface);
}

for (const auto & [name, descr] : joint_state_interfaces_)
{
auto state_interface = std::make_shared<StateInterface>(descr);
actuator_states_.insert(std::make_pair(name, state_interface));
joint_states_.push_back(state_interface);
state_interfaces.push_back(state_interface);
}
return state_interfaces;
}
Copy link
Member

@saikishor saikishor Aug 30, 2024

Choose a reason for hiding this comment

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

Maybe we can leave this to the user by providing some helper methods, so he can define it himself in the same structure or a different one. In this way, he will be open to doing it in an unordered_map, map, or a vector

Same comments for command interfaces as well

Copy link
Member

Choose a reason for hiding this comment

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

I'd opt for doing it all and cutting down on what we support later... With helpers we push
boilerplate code into user space which is troubling

Copy link
Member

Choose a reason for hiding this comment

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

Sure 👍🏾

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with Bence on this.

Copy link
Member

Choose a reason for hiding this comment

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

Sure! I'm fine with this

Comment on lines +408 to +427
void set_state(const std::string & interface_name, const double & value)
{
actuator_states_.at(interface_name)->set_value(value);
}

double get_state(const std::string & interface_name) const
{
return actuator_states_.at(interface_name)->get_value();
}

void set_command(const std::string & interface_name, const double & value)
{
actuator_commands_.at(interface_name)->set_value(value);
}

double get_command(const std::string & interface_name) const
{
return actuator_commands_.at(interface_name)->get_value();
}

Copy link
Member

@saikishor saikishor Aug 30, 2024

Choose a reason for hiding this comment

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

When we use the backward compatibility part of code, we can't use these methods (which is completely valid) and it is mentioned in the docs above.

I don't know if it is wiser to expose them if they don't work for the earlier approach

So, we can do 2 things here:

  • Remove support for the old way of exposing interfaces completely, so we can make use of these methods
  • Remove these methods along with some changes with the above comments I mentioned, in that way, the user will have the liberty to choose the way he accesses the interfaces via unordered_map, map, or vector

It's also because all the above new methods are pretty repetitive to all Component Interfaces (Actuator, Sensor, and System)

Copy link
Member

Choose a reason for hiding this comment

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

can we flag up warnings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah thats a valid point. How would you flag up warnings?

Copy link
Member

Choose a reason for hiding this comment

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

May be we can check if they are empty, and simply print a log message and return?
I can't think of a better idea

----------------------------------------------
In case the default implementation (``on_export_command_interfaces()`` or ``on_export_state_interfaces()`` ) for exporting the ``Command-/StateInterfaces`` is not enough you can override them. You should however consider the following things:

* If you want to have unlisted interfaces available you need to call the ``export_command_interfaces_2()`` or ``export_state_interfaces_2()`` and add them to the ``unlisted_command_interfaces_`` or ``unlisted_state_interfaces_``.
Copy link
Member

Choose a reason for hiding this comment

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

all the docs still have the _2 issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes sorry about this. I thought i changed it

Comment on lines +131 to +158
/**
* Import the InterfaceDescription for the StateInterfaces from the HardwareInfo.
* Separate them into the possible types: Joint and store them.
*/
virtual void import_state_interface_descriptions(const HardwareInfo & hardware_info)
{
auto joint_state_interface_descriptions =
parse_state_interface_descriptions(hardware_info.joints);
for (const auto & description : joint_state_interface_descriptions)
{
joint_state_interfaces_.insert(std::make_pair(description.get_name(), description));
}
}

/**
* Import the InterfaceDescription for the CommandInterfaces from the HardwareInfo.
* Separate them into the possible types: Joint and store them.
*/
virtual void import_command_interface_descriptions(const HardwareInfo & hardware_info)
{
auto joint_command_interface_descriptions =
parse_command_interface_descriptions(hardware_info.joints);
for (const auto & description : joint_command_interface_descriptions)
{
joint_command_interfaces_.insert(std::make_pair(description.get_name(), description));
}
}

Copy link
Member

Choose a reason for hiding this comment

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

they were declared virtual I guess in case people want to override them 🤷

Comment on lines 207 to 237
virtual std::vector<std::shared_ptr<StateInterface>> on_export_state_interfaces()
{
// import the unlisted interfaces
std::vector<hardware_interface::InterfaceDescription> unlisted_interface_descriptions =
export_state_interface_descriptions();

std::vector<std::shared_ptr<StateInterface>> state_interfaces;
state_interfaces.reserve(
unlisted_interface_descriptions.size() + joint_state_interfaces_.size());

// add InterfaceDescriptions and create the StateInterfaces from the descriptions and add to
// maps.
for (const auto & description : unlisted_interface_descriptions)
{
auto name = description.get_name();
unlisted_state_interfaces_.insert(std::make_pair(name, description));
auto state_interface = std::make_shared<StateInterface>(description);
actuator_states_.insert(std::make_pair(name, state_interface));
unlisted_states_.push_back(state_interface);
state_interfaces.push_back(state_interface);
}

for (const auto & [name, descr] : joint_state_interfaces_)
{
auto state_interface = std::make_shared<StateInterface>(descr);
actuator_states_.insert(std::make_pair(name, state_interface));
joint_states_.push_back(state_interface);
state_interfaces.push_back(state_interface);
}
return state_interfaces;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd opt for doing it all and cutting down on what we support later... With helpers we push
boilerplate code into user space which is troubling

Comment on lines +408 to +427
void set_state(const std::string & interface_name, const double & value)
{
actuator_states_.at(interface_name)->set_value(value);
}

double get_state(const std::string & interface_name) const
{
return actuator_states_.at(interface_name)->get_value();
}

void set_command(const std::string & interface_name, const double & value)
{
actuator_commands_.at(interface_name)->set_value(value);
}

double get_command(const std::string & interface_name) const
{
return actuator_commands_.at(interface_name)->get_value();
}

Copy link
Member

Choose a reason for hiding this comment

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

can we flag up warnings?

hardware_interface/src/actuator.cpp Outdated Show resolved Hide resolved
hardware_interface/src/actuator.cpp Outdated Show resolved Hide resolved
hardware_interface/src/sensor.cpp Outdated Show resolved Hide resolved
hardware_interface/src/system.cpp Outdated Show resolved Hide resolved
hardware_interface/src/system.cpp Outdated Show resolved Hide resolved
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Another minor review :)

@@ -71,10 +71,10 @@ class Actuator final
const rclcpp_lifecycle::State & error();

HARDWARE_INTERFACE_PUBLIC
std::vector<StateInterface> export_state_interfaces();
std::vector<std::shared_ptr<StateInterface>> export_state_interfaces();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::vector<std::shared_ptr<StateInterface>> export_state_interfaces();
std::vector<std::shared_ptr<const StateInterface>> export_state_interfaces();

The StateInterface better use the SharedConstPtr as they are not supposed to be changed outside the hardware interface. After defining the typedef, the following would be better

Suggested change
std::vector<std::shared_ptr<StateInterface>> export_state_interfaces();
std::vector<StateInterface::SharedConstPtr> export_state_interfaces();

Comment on lines 265 to 278
* Override this method to export custom CommandInterfaces which are not defined in the URDF file.
* Those interfaces will be added to the unlisted_command_interfaces_ map.
*
* Note method name is going to be changed to export_command_interfaces() as soon as the
* deprecated version is removed.
*
* \return vector of descriptions to the unlisted CommandInterfaces
*/
virtual std::vector<hardware_interface::InterfaceDescription>
export_command_interface_descriptions()
{
// return empty vector by default.
return {};
}
Copy link
Member

@saikishor saikishor Sep 9, 2024

Choose a reason for hiding this comment

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

Can we still have this when we fully move to the variants now?, as we will have the value instead of a pointer internally. If this is the case, maybe it's better to remove it completely or wrap the variant around a shared_ptr, so passing this might still work.

Copy link
Contributor

mergify bot commented Sep 11, 2024

This pull request is in conflict. Could you fix it @mamueluth?

Comment on lines -47 to 52
: prefix_name_(interface_description.prefix_name),
interface_name_(interface_description.interface_info.name),
handle_name_(prefix_name_ + "/" + interface_name_)
: prefix_name_(interface_description.get_prefix_name()),
interface_name_(interface_description.get_interface_name()),
handle_name_(interface_description.get_name())
{
// As soon as multiple datatypes are used in HANDLE_DATATYPE
// we need to initialize according the type passed in interface description
Copy link
Member

Choose a reason for hiding this comment

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

Are we missing the final changes of the variants here?. I don't find the changes in #1689 as well
The changes in the hardware component interfaces would kinda work only with the final version of the Handle changes right?

Copy link
Member

Choose a reason for hiding this comment

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

I saw that you are wrapping them with shared_ptrs from the existing objects, this should be fine and would work for backward compatibility

Copy link
Member Author

Choose a reason for hiding this comment

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

sry i did quite catch what you mean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants