[HW interface] Resource manager publish all Command-/StateInterface values #1244
Conversation
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
github-actions
bot
requested review from
aprotyas,
bmagyar,
DasRoteSkelett,
destogl,
livanov93,
peterdavidfagan and
saikishor
aprotyas
reviewed
Dec 22, 2023
Comment on lines
1335
to
1349
| try | ||
| { | ||
| const auto command_interface_value = | ||
| resource_storage_->command_interface_map_.at(command_interface_name).get_value(); | ||
| command_interface_values.interface_names.push_back(command_interface_name); | ||
| command_interface_values.values.push_back(command_interface_value); | ||
| } | ||
| catch (const std::out_of_range & e) | ||
| { | ||
| RCUTILS_LOG_WARN_NAMED( | ||
| "resource_manager", | ||
| "command interface '%s' is in available list, but could not get the interface " | ||
| "command_interface_map_ (std::out_of_range exception thrown).", | ||
| command_interface_name.c_str()); | ||
| } |
There was a problem hiding this comment.
Alternatively you could avoid the cost of throwing-consuming an exception and just emit your log behind a key-exists-in-the-map check?
Suggested change
| try | |
| { | |
| const auto command_interface_value = | |
| resource_storage_->command_interface_map_.at(command_interface_name).get_value(); | |
| command_interface_values.interface_names.push_back(command_interface_name); | |
| command_interface_values.values.push_back(command_interface_value); | |
| } | |
| catch (const std::out_of_range & e) | |
| { | |
| RCUTILS_LOG_WARN_NAMED( | |
| "resource_manager", | |
| "command interface '%s' is in available list, but could not get the interface " | |
| "command_interface_map_ (std::out_of_range exception thrown).", | |
| command_interface_name.c_str()); | |
| } | |
| if (const auto iter = resource_storage_->command_interface_map_.find(command_interface_name); iter != resource_storage_->command_interface_map_.cend()) | |
| { | |
| const auto command_interface_value = | |
| *iter; | |
| command_interface_values.interface_names.push_back(command_interface_name); | |
| command_interface_values.values.push_back(command_interface_value); | |
| } | |
| else | |
| { | |
| RCUTILS_LOG_WARN_NAMED( | |
| "resource_manager", | |
| "command interface '%s' is in available list, but could not get the interface " | |
| "command_interface_map_ (std::out_of_range exception thrown).", | |
| command_interface_name.c_str()); | |
| } |
There was a problem hiding this comment.
Good one. This also makes sense.
I would only change a bit the printed log a bit.
There was a problem hiding this comment.
Done.
How would you change it?
saikishor
requested changes
Jan 5, 2024
hardware_interface/include/hardware_interface/resource_manager.hpp
Outdated
Show resolved
Hide resolved
hardware_interface/include/hardware_interface/resource_manager.hpp
Outdated
Show resolved
Hide resolved
| } | ||
| catch (const std::out_of_range & e) | ||
| { | ||
| RCUTILS_LOG_WARN_NAMED( |
There was a problem hiding this comment.
Out of curiosity, do you think this should be throttled? or do you think it is fine to have like this?
There was a problem hiding this comment.
i am not sure about the performance penalty, but since this case should not happen under normal circumstances, i think its fine like this. But any thoughts/corrections are welcome.
Comment on lines
1335
to
1349
| try | ||
| { | ||
| const auto command_interface_value = | ||
| resource_storage_->command_interface_map_.at(command_interface_name).get_value(); | ||
| command_interface_values.interface_names.push_back(command_interface_name); | ||
| command_interface_values.values.push_back(command_interface_value); | ||
| } | ||
| catch (const std::out_of_range & e) | ||
| { | ||
| RCUTILS_LOG_WARN_NAMED( | ||
| "resource_manager", | ||
| "command interface '%s' is in available list, but could not get the interface " | ||
| "command_interface_map_ (std::out_of_range exception thrown).", | ||
| command_interface_name.c_str()); | ||
| } |
There was a problem hiding this comment.
Good one. This also makes sense.
I would only change a bit the printed log a bit.
livanov93
reviewed
Jun 5, 2024
mamueluth
commented
Jun 5, 2024
mamueluth
commented
Jun 5, 2024
|
The messages were merged and released to Iron, Jazzy and Rolling ;) |
|
This pull request is in conflict. Could you fix it @mamueluth? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This adds publishing of all the values of the command-/stateInterfaces by the resource manager. Provides a kind of diagnostic interface. The values are published to
/resource_manager_publisher_node/interface_values.This PR depends on this PR from the control_msgs repos.