-
Notifications
You must be signed in to change notification settings - Fork 508
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
Conversation
|
||
moveit::ConstructException::ConstructException(const std::string& what_arg) : std::runtime_error(what_arg) | ||
{ | ||
ROS_ERROR_NAMED("exceptions", "Error during construction of object: %s\nException thrown.", what_arg.c_str()); | ||
// ROS_ERROR_NAMED("exceptions", "Error during construction of object: %s\nException thrown.", what_arg.c_str()); |
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.
remove dead 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.
👎 Is there a _NAMED
replacement in ROS2 logging at the moment?
removing _NAMED
from the code-base is no option in my opinion.
@davetcoleman and others pushed for a long time to get it in there in the first place.
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.
+1 to keeping NAMED
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.
You should use RCLCPP_ERROR(some->way->to_get->node->get_logger(), "...");
, but if access to the node is impossible here, then something like RCLCPP_ERROR(rclcpp::get_logger("moveit"), "...");
.
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.
Note, if you're going for the latter you can store the logger object somewhere to avoid creating a std::string
each time you get the logger.
|
||
moveit::ConstructException::ConstructException(const std::string& what_arg) : std::runtime_error(what_arg) | ||
{ | ||
ROS_ERROR_NAMED("exceptions", "Error during construction of object: %s\nException thrown.", what_arg.c_str()); | ||
// ROS_ERROR_NAMED("exceptions", "Error during construction of object: %s\nException thrown.", what_arg.c_str()); |
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.
👎 Is there a _NAMED
replacement in ROS2 logging at the moment?
removing _NAMED
from the code-base is no option in my opinion.
@davetcoleman and others pushed for a long time to get it in there in the first place.
#include <rcutils/logging_macros.h> | ||
#include "rclcpp/rclcpp.hpp" | ||
|
||
#define ROS_ERROR RCUTILS_LOG_ERROR |
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 like RCUTILS_LOG_ERROR
(i.e. as opposed to RCUTILS_LOG_ERROR_NAMED
), e.g.:
In general though, you guys should be using rclcpp
's macros rather than the ones in rcutils
, since you're depending on rclcpp
. They are implemented in rcutils
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 having RCLCPP_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's ROS_ERROR
. The correct equivalent in ROS 2 is RCLCPP_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 the moveit_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 assume there's a core data structure from which everything else is created and which owns all of those things, like
parent
in Qt.
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:
I assume there's a core data structure from which everything else is created and which owns all of those things, like
Node
in ROS 2, which is used to create things likePublisher
's, so the publisher has access to the node that created it.
Or maybe like Qt, where you pass the parent in to each "sub-entity" but it does not own it.
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.
install(DIRECTORY include/ DESTINATION ${CATKIN_GLOBAL_INCLUDE_DESTINATION}) | ||
LIBRARY DESTINATION lib | ||
ARCHIVE DESTINATION lib) | ||
install(DIRECTORY include/ DESTINATION include) |
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
|
||
moveit::ConstructException::ConstructException(const std::string& what_arg) : std::runtime_error(what_arg) | ||
{ | ||
ROS_ERROR_NAMED("exceptions", "Error during construction of object: %s\nException thrown.", what_arg.c_str()); | ||
ROS_ERROR("Error during construction of object: %s\nException thrown.", what_arg.c_str()); |
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
Port Kinematic_constraints to ROS2
I believe we have sufficiently debated logging and should be good to proceed with this PR
@henningkayser Can you give this a 2nd review? |
} | ||
|
||
moveit::Exception::Exception(const std::string& what_arg) : std::runtime_error(what_arg) | ||
{ | ||
ROS_ERROR_NAMED("exceptions", "%s\nException thrown.", what_arg.c_str()); | ||
RCLCPP_ERROR(logger_exceptions, "Exception thrown.", what_arg.c_str()); |
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.
Why is "%s\n" removed from the error message? This way the exception is not printed to the log
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.
That's right, thanks for the catch @henningkayser b329df8
#include "rclcpp/rclcpp.hpp" | ||
|
||
// Logger | ||
rclcpp::Logger logger_exceptions = rclcpp::get_logger("exceptions"); |
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.
rclcpp::Logger logger_exceptions = rclcpp::get_logger("exceptions"); | |
static const rclcpp::Logger LOGGER = rclcpp::get_logger("exceptions"); |
(see #33)
This is optional since it's still being discussed.
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.
I'm ok with this change but it'll require further changes in the file (those places where logger_exceptions is used will also need to be renamed).
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.
Maybe, shall we leave this change aside for now and make a global change if we decide to use LOGGER
?
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.
I'm fine with this!
Follows from moveit#7 (review)
Follows from moveit#7 (review)
Except for #7 (comment), are we good to go here? |
… AddRuckigTrajectorySmoothing (moveit#7)
… AddRuckigTrajectorySmoothing (moveit#7)
… AddRuckigTrajectorySmoothing (moveit#7)
…anning-scene Support for planning in a given planning scene
No description provided.