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

[PR-1689] Follow-up PR of the controller interface variants integration #1779

Merged
merged 6 commits into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,10 @@ class ChainableControllerInterface : public ControllerInterfaceBase
// BEGIN (Handle export change): for backward compatibility
std::vector<double> reference_interfaces_;
// END
std::vector<hardware_interface::CommandInterface::SharedPtr> ordered_reference_interfaces_;
std::vector<hardware_interface::CommandInterface::SharedPtr>
ordered_exported_reference_interfaces_;
std::unordered_map<std::string, hardware_interface::CommandInterface::SharedPtr>
reference_interfaces_ptrs_;
exported_reference_interfaces_;

private:
/// A flag marking if a chainable controller is currently preceded by another controller.
Expand Down
10 changes: 10 additions & 0 deletions controller_interface/include/controller_interface/helpers.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@ inline bool interface_list_contains_interface_type(
interface_type_list.end();
}

template <typename T>
void add_element_to_list(std::vector<T> & list, const T & element)
{
if (std::find(list.begin(), list.end(), element) == list.end())
{
// Only add to the list if it doesn't exist
list.push_back(element);
Comment on lines +86 to +87
Copy link
Member

Choose a reason for hiding this comment

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

when are we adding things multiple times?

Copy link
Member Author

Choose a reason for hiding this comment

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

std::vector<hardware_interface::StateInterface>
ChainableControllerInterface::on_export_state_interfaces()
{
state_interfaces_values_.resize(exported_state_interface_names_.size(), 0.0);
std::vector<hardware_interface::StateInterface> state_interfaces;
for (size_t i = 0; i < exported_state_interface_names_.size(); ++i)
{
state_interfaces.emplace_back(
get_node()->get_name(), exported_state_interface_names_[i], &state_interfaces_values_[i]);
}
return state_interfaces;
}
std::vector<hardware_interface::CommandInterface>
ChainableControllerInterface::on_export_reference_interfaces()
{
reference_interfaces_.resize(exported_reference_interface_names_.size(), 0.0);
std::vector<hardware_interface::CommandInterface> reference_interfaces;
for (size_t i = 0; i < exported_reference_interface_names_.size(); ++i)
{
reference_interfaces.emplace_back(hardware_interface::CommandInterface(
get_node()->get_name(), exported_reference_interface_names_[i], &reference_interfaces_[i]));
}
return reference_interfaces;
}

here we are creating the interfaces from the exported_state_interface_names_ and exported_reference_interface_names_ and later we are readding again to the same vector in the method. I think it is better to check and insert to not to have inconsistent sizes.

This will only happen if we use these variables to create and export the interfaces

Copy link
Member

Choose a reason for hiding this comment

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

sounds good, thank you

}
}

} // namespace controller_interface

#endif // CONTROLLER_INTERFACE__HELPERS_HPP_
37 changes: 26 additions & 11 deletions controller_interface/src/chainable_controller_interface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include <vector>

#include "controller_interface/helpers.hpp"
#include "hardware_interface/types/lifecycle_state_names.hpp"
#include "lifecycle_msgs/msg/state.hpp"

Expand Down Expand Up @@ -67,7 +68,7 @@ ChainableControllerInterface::export_state_interfaces()
throw std::runtime_error(error_msg);
}
auto state_interface = std::make_shared<hardware_interface::StateInterface>(interface);
const auto interface_name = state_interface->get_name();
const auto interface_name = state_interface->get_interface_name();
auto [it, succ] = exported_state_interfaces_.insert({interface_name, state_interface});
// either we have name duplicate which we want to avoid under all circumstances since interfaces
// need to be uniquely identify able or something else really went wrong. In any case abort and
Expand All @@ -87,11 +88,24 @@ ChainableControllerInterface::export_state_interfaces()
throw std::runtime_error(error_msg);
}
ordered_exported_state_interfaces_.push_back(state_interface);
exported_state_interface_names_.push_back(interface_name);
add_element_to_list(exported_state_interface_names_, interface_name);
state_interfaces_ptrs_vec.push_back(
std::const_pointer_cast<const hardware_interface::StateInterface>(state_interface));
}

if (exported_state_interfaces_.size() != state_interfaces.size())
{
std::string error_msg =
"The internal storage for state interface ptrs 'exported_state_interfaces_' variable has "
"size '" +
std::to_string(exported_state_interfaces_.size()) +
"', but it is expected to have the size '" + std::to_string(state_interfaces.size()) +
"' equal to the number of exported reference interfaces. Please correct and recompile the "
"controller with name '" +
get_node()->get_name() + "' and try again.";
throw std::runtime_error(error_msg);
}

bmagyar marked this conversation as resolved.
Show resolved Hide resolved
return state_interfaces_ptrs_vec;
}

Expand All @@ -102,7 +116,7 @@ ChainableControllerInterface::export_reference_interfaces()
std::vector<hardware_interface::CommandInterface::SharedPtr> reference_interfaces_ptrs_vec;
reference_interfaces_ptrs_vec.reserve(reference_interfaces.size());
exported_reference_interface_names_.reserve(reference_interfaces.size());
ordered_reference_interfaces_.reserve(reference_interfaces.size());
ordered_exported_reference_interfaces_.reserve(reference_interfaces.size());

// BEGIN (Handle export change): for backward compatibility
// check if the "reference_interfaces_" variable is resized to number of interfaces
Expand Down Expand Up @@ -135,16 +149,16 @@ ChainableControllerInterface::export_reference_interfaces()

hardware_interface::CommandInterface::SharedPtr reference_interface =
std::make_shared<hardware_interface::CommandInterface>(std::move(interface));
const auto inteface_name = reference_interface->get_name();
const auto interface_name = reference_interface->get_interface_name();
// check the exported interface name is unique
auto [it, succ] = reference_interfaces_ptrs_.insert({inteface_name, reference_interface});
auto [it, succ] = exported_reference_interfaces_.insert({interface_name, reference_interface});
// either we have name duplicate which we want to avoid under all circumstances since interfaces
// need to be uniquely identify able or something else really went wrong. In any case abort and
// inform cm by throwing exception
if (!succ)
{
std::string error_msg =
"Could not insert Reference interface<" + inteface_name +
"Could not insert Reference interface<" + interface_name +
"> into reference_interfaces_ map. Check if you export duplicates. The "
"map returned iterator with interface_name<" +
it->second->get_name() +
Expand All @@ -155,16 +169,17 @@ ChainableControllerInterface::export_reference_interfaces()
reference_interfaces_ptrs_vec.clear();
throw std::runtime_error(error_msg);
}
ordered_reference_interfaces_.push_back(reference_interface);
exported_reference_interface_names_.push_back(inteface_name);
ordered_exported_reference_interfaces_.push_back(reference_interface);
add_element_to_list(exported_reference_interface_names_, interface_name);
reference_interfaces_ptrs_vec.push_back(reference_interface);
}

if (reference_interfaces_ptrs_.size() != ref_interface_size)
if (exported_reference_interfaces_.size() != ref_interface_size)
{
std::string error_msg =
"The internal storage for reference ptrs 'reference_interfaces_ptrs_' variable has size '" +
std::to_string(reference_interfaces_ptrs_.size()) +
"The internal storage for exported reference ptrs 'exported_reference_interfaces_' variable "
"has size '" +
std::to_string(exported_reference_interfaces_.size()) +
"', but it is expected to have the size '" + std::to_string(ref_interface_size) +
"' equal to the number of exported reference interfaces. Please correct and recompile the "
"controller with name '" +
Expand Down
8 changes: 4 additions & 4 deletions controller_manager/src/controller_manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -796,16 +796,16 @@ controller_interface::return_type ControllerManager::configure_controller(
// TODO(destogl): Add test for this!
RCLCPP_ERROR(
get_logger(),
"Controller '%s' is chainable, but does not export any reference interfaces. Did you "
"override the on_export_method() correctly?",
"Controller '%s' is chainable, but does not export any state or reference interfaces. "
"Did you override the on_export_method() correctly?",
controller_name.c_str());
return controller_interface::return_type::ERROR;
}
}
catch (const std::runtime_error & e)
catch (const std::exception & e)
{
RCLCPP_FATAL(
get_logger(), "Creation of the reference interfaces failed with following error: %s",
get_logger(), "Export of the state or reference interfaces failed with following error: %s",
e.what());
return controller_interface::return_type::ERROR;
}
Expand Down
Loading