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.
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
Port exceptions submodule of moveit_core #7
Port exceptions submodule of moveit_core #7
Changes from 4 commits
8039e73
dcfdc76
9f9e4cf
eaa7cf2
06c07e0
7138f30
b329df8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this kind of changes would be ideal to do across the code base at once
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.
Defining these macros in specific moveit headers does not make any sense. 👎
I fail to understand why they are not defined in
rcutils/logging_macros.h
(if they really do not exist there).Apparently it works to redefine them here...
If OSRF disagrees with having them in there, we should at least define all of them in a single standard header for all of MoveIt, but I would strongly push to have upstream ROS2 provide them.
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.
this has already been created for moveit, just this week!
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.
There is a "named" version in
RCLCPP_ERROR
, see:http://docs.ros2.org/crystal/api/rclcpp/logging_8hpp.html#ad8f3687eda7312561768319e99cd4ca6
It requires that you provide a "Logger", but that's just a struct wrapping a string, so it's essentially a name. You can get the node's logger (which is just a logger named after the node and node namespace) which is recommended:
http://docs.ros2.org/crystal/api/rclcpp/classrclcpp_1_1Node.html#a1d6fda144e3748d52d3e10ea81e9558f
Once you have it, you can "extend" the logger name with
get_child(string)
:http://docs.ros2.org/crystal/api/rclcpp/classrclcpp_1_1Logger.html#a68ab28459973c1072d3ad7a593ac994e
Or you can create a logger from scratch using your own invented name (less good, but ok if you don't have a node in the context, e.g. pre node construction) with
rclcpp::get_logger(string)
:http://docs.ros2.org/crystal/api/rclcpp/namespacerclcpp.html#ae7295751947c08312aa69f45fd673171
Or you can use just an empty string when calling
rclcpp::get_logger("")
, which is the least good alternative because it doesn't give any context to the log consumer (user). This is essentially what unnamed macros in rcutils do likeRCUTILS_LOG_ERROR
(i.e. as opposed toRCUTILS_LOG_ERROR_NAMED
), e.g.:https://github.com/ros2/rcutils/blob/a7581a333444c2804f2b7c27dd0da1b2946bebff/resource/logging_macros.h.em#L183
In general though, you guys should be using
rclcpp
's macros rather than the ones inrcutils
, since you're depending onrclcpp
. They are implemented inrcutils
so we can reuse them in our C api and in python, as well as in c++.See also:
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.
Also, it's worth saying, that we don't provide an "unnamed" macro in rclcpp because unlike ROS 1's
ROS_ERROR
, in ROS 2 there's no singleton node who's name can be used with it. So havingRCLCPP_ERROR
with no logger/name as an argument would simply have no name or node attributed to it, unlike ROS 1. So it would be misleading/incorrect to use that as the alternative to ROS 1'sROS_ERROR
. The correct equivalent in ROS 2 isRCLCPP_ERROR(node->get_logger(), "my message");
.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.
moveit_core
isn't suppose to have the notion of a ROS node, that all lives in themoveit_ros
set of packages. Are you saying we'd need to track the node object throughout the moveit_core code? is there a logger that doesn't require this? or would, say,rclcpp::get_logger("moveit")
suffice for a non-ROS part of the code?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.
My wording was kind of bad in that quote, what I meant was more like:
I think the answer is still that this assumption was wrong, but I just wanted to be clear.
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.
Which reading it one more time is also inaccurate... oh boy. One more time:
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.
MoveGroup is the only thing i can think of that reminds me of what you maybe saying...
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.
See #21 (comment)
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.
7138f30
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.
this is a regression from moveit1 - we want to keep the namespaces
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.
Logging changed 7138f30