[HW Interface] Change creation, exportation and storing of Command-/StateInterface + Support for variant #1240
+2,659
−268
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,53 +44,91 @@ return_type ChainableControllerInterface::update( | |
| return ret; | ||
| } | ||
|
|
||
| std::vector<std::shared_ptr<hardware_interface::StateInterface>> | ||
| ChainableControllerInterface::export_state_interfaces() | ||
| { | ||
| auto state_interfaces = on_export_state_interfaces(); | ||
| std::vector<std::shared_ptr<hardware_interface::StateInterface>> state_interfaces_ptrs_vec; | ||
| state_interfaces_ptrs_vec.reserve(state_interfaces.size()); | ||
|
|
||
| // check if the names of the controller state interfaces begin with the controller's name | ||
| for (const auto & interface : state_interfaces) | ||
| { | ||
| if (interface.get_prefix_name() != get_node()->get_name()) | ||
| { | ||
| std::string error_msg = | ||
| "The prefix of the interface '" + interface.get_prefix_name() + | ||
| "' does not equal the controller's name '" + get_node()->get_name() + | ||
| "'. This is mandatory for state interfaces. No state interface will be exported. Please " | ||
| "correct and recompile the controller with name '" + | ||
| get_node()->get_name() + "' and try again."; | ||
| throw std::runtime_error(error_msg); | ||
| } | ||
|
|
||
| state_interfaces_ptrs_vec.push_back( | ||
| std::make_shared<hardware_interface::StateInterface>(interface)); | ||
| } | ||
|
|
||
| return state_interfaces_ptrs_vec; | ||
| } | ||
|
|
||
| std::vector<std::shared_ptr<hardware_interface::CommandInterface>> | ||
| ChainableControllerInterface::export_reference_interfaces() | ||
| { | ||
| auto reference_interfaces = on_export_reference_interfaces(); | ||
| std::vector<std::shared_ptr<hardware_interface::CommandInterface>> reference_interfaces_ptrs_vec; | ||
| reference_interfaces_ptrs_vec.reserve(reference_interfaces.size()); | ||
|
|
||
| // BEGIN (Handle export change): for backward compatibility | ||
| // check if the "reference_interfaces_" variable is resized to number of interfaces | ||
| if (reference_interfaces_.size() != reference_interfaces.size()) | ||
| { | ||
| // TODO(destogl): Should here be "FATAL"? It is fatal in terms of controller but not for the | ||
| // framework | ||
|
Comment on lines
+86
to
+87
There was a problem hiding this comment. i have to check this will comment later i would prefer fixing everything and providing as well. |
||
| std::string error_msg = | ||
| "The internal storage for reference values 'reference_interfaces_' variable has size '" + | ||
| std::to_string(reference_interfaces_.size()) + "', but it is expected to have the size '" + | ||
| std::to_string(reference_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); | ||
| } | ||
| // END | ||
|
|
||
| // check if the names of the reference interfaces begin with the controller's name | ||
| const auto ref_interface_size = reference_interfaces.size(); | ||
| for (auto & interface : reference_interfaces) | ||
| { | ||
| if (interface.get_prefix_name() != get_node()->get_name()) | ||
| { | ||
| std::string error_msg = "The name of the interface " + interface.get_name() + | ||
| " does not begin with the controller's name. This is mandatory for " | ||
| "reference interfaces. Please " | ||
| "correct and recompile the controller with name " + | ||
| get_node()->get_name() + " and try again."; | ||
| throw std::runtime_error(error_msg); | ||
| } | ||
|
|
||
| std::shared_ptr<hardware_interface::CommandInterface> interface_ptr = | ||
| std::make_shared<hardware_interface::CommandInterface>(std::move(interface)); | ||
| reference_interfaces_ptrs_vec.push_back(interface_ptr); | ||
| reference_interfaces_ptrs_.insert(std::make_pair(interface_ptr->get_name(), interface_ptr)); | ||
| } | ||
|
|
||
| if (reference_interfaces_ptrs_.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()) + | ||
| "', 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 '" + | ||
| get_node()->get_name() + "' and try again."; | ||
| throw std::runtime_error(error_msg); | ||
| } | ||
|
|
||
| return reference_interfaces_ptrs_vec; | ||
| } | ||
|
|
||
| bool ChainableControllerInterface::set_chained_mode(bool chained_mode) | ||
|
|
@@ -130,8 +168,8 @@ ChainableControllerInterface::on_export_state_interfaces() | |
| 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; | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -22,12 +22,14 @@ ControllerInterface::ControllerInterface() : ControllerInterfaceBase() {} | |||||
|
|
||||||
| bool ControllerInterface::is_chainable() const { return false; } | ||||||
|
|
||||||
| std::vector<std::shared_ptr<hardware_interface::StateInterface>> | ||||||
| ControllerInterface::export_state_interfaces() | ||||||
| { | ||||||
| return {}; | ||||||
| } | ||||||
|
|
||||||
| std::vector<std::shared_ptr<hardware_interface::CommandInterface>> | ||||||
|
There was a problem hiding this comment.
Suggested change
Was wondering if we could do this instead? Just to shorten things... There was a problem hiding this comment. no because then they would not override the base functions (which have to have this return type because controllers need access to the exported handles to set/get values from them). There was a problem hiding this comment. I think @bmagyar means here that we define a new type, something like: This should go to the header where On the other ahdn I am not fan of it, as it makes things shorter, but make them also less clear and direct. |
||||||
| ControllerInterface::export_reference_interfaces() | ||||||
| { | ||||||
| return {}; | ||||||
| } | ||||||
|
|
||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do these needs to be shared pointers? I'm just wondering about how best we can keep track of the lifecycle of things moving around...
would a unique_ptr be unusable? I also hate seeing
std::movein active codebase but it is a fair use-case for this IMOThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess no because we want to export the reference interfaces but the controller still needs to have access to them (since value is now stored directly). Its same with the hardware. We could avoid creating shared_ptrs by creating special loans for hw or chainable controllers. But i think should be fine in this case since handles have no lifecycle bit their lifecycle is indirectly connected to the components lifecycle itself. this is managed by resource manager which should be the only part having the pointers besides the components themselves.