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

Port exceptions submodule of moveit_core #7

Merged
merged 7 commits into from
Mar 14, 2019
Merged
Show file tree
Hide file tree
Changes from 4 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
9 changes: 4 additions & 5 deletions moveit_core/exceptions/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
set(MOVEIT_LIB_NAME moveit_exceptions)

add_library(${MOVEIT_LIB_NAME} src/exceptions.cpp)
set_target_properties(${MOVEIT_LIB_NAME} PROPERTIES VERSION "${${PROJECT_NAME}_VERSION}")
vmayoral marked this conversation as resolved.
Show resolved Hide resolved

target_link_libraries(${MOVEIT_LIB_NAME} ${catkin_LIBRARIES} ${urdfdom_LIBRARIES} ${urdfdom_headers_LIBRARIES} ${Boost_LIBRARIES})
target_link_libraries(${MOVEIT_LIB_NAME} ${rclcpp_LIBRARIES} ${rmw_implementation_LIBRARIES} ${urdfdom_LIBRARIES} ${urdfdom_headers_LIBRARIES} ${Boost_LIBRARIES})

install(TARGETS ${MOVEIT_LIB_NAME}
LIBRARY DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION}
ARCHIVE DESTINATION ${CATKIN_PACKAGE_LIB_DESTINATION})
install(DIRECTORY include/ DESTINATION ${CATKIN_GLOBAL_INCLUDE_DESTINATION})
LIBRARY DESTINATION lib
ARCHIVE DESTINATION lib)
install(DIRECTORY include/ DESTINATION include)
Copy link
Member

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

9 changes: 6 additions & 3 deletions moveit_core/exceptions/src/exceptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,17 @@
/* Author: Acorn Pooley, Ioan Sucan */

#include <moveit/exceptions/exceptions.h>
#include <ros/console.h>
#include <rcutils/logging_macros.h>
#include "rclcpp/rclcpp.hpp"

#define ROS_ERROR RCUTILS_LOG_ERROR
Copy link
Contributor

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.

Copy link
Member

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!

Copy link
Contributor

@wjwwood wjwwood Feb 20, 2019

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.:

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 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:

Copy link
Contributor

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");.

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor

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 like Publisher'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.

Copy link
Member

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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


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());
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logging changed 7138f30

}

moveit::Exception::Exception(const std::string& what_arg) : std::runtime_error(what_arg)
{
ROS_ERROR_NAMED("exceptions", "%s\nException thrown.", what_arg.c_str());
ROS_ERROR("Exception thrown.", what_arg.c_str());
}