Port exceptions submodule of moveit_core #7

vmayoral · 2019-02-19T09:32:00Z

No description provided.

moveit_core/exceptions/CMakeLists.txt

moveit_core/exceptions/src/exceptions.cpp

mlautman · 2019-02-19T21:16:54Z

moveit_core/exceptions/src/exceptions.cpp


 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());


remove dead code

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

+1 to keeping NAMED

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

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_core/exceptions/src/exceptions.cpp

moveit_core/exceptions/CMakeLists.txt

v4hn · 2019-02-20T14:34:02Z

moveit_core/exceptions/src/exceptions.cpp


 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());


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

v4hn · 2019-02-20T14:36:54Z

moveit_core/exceptions/src/exceptions.cpp

+#include <rcutils/logging_macros.h>
+#include "rclcpp/rclcpp.hpp"
+
+#define ROS_ERROR RCUTILS_LOG_ERROR


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.

this has already been created for moveit, just this week!

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:

https://index.ros.org/doc/ros2/Concepts/Logging/

https://github.com/ros2/demos/blob/master/logging_demo/src/logger_usage_component.cpp

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

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?

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.

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.

MoveGroup is the only thing i can think of that reminds me of what you maybe saying...

See #21 (comment)

moveit_core/exceptions/src/exceptions.cpp

vmayoral · 2019-02-24T11:19:44Z

Addressed some of the concerns expressed above regarding dead code. I'd suggest we follow the recommendations of @wjwwood in another PR since it'll require changes in several of the submodules of moveit_core.

Connected to #21.

moveit_core/exceptions/CMakeLists.txt

davetcoleman · 2019-02-24T21:17:47Z

moveit_core/exceptions/CMakeLists.txt

-install(DIRECTORY include/ DESTINATION ${CATKIN_GLOBAL_INCLUDE_DESTINATION})
+        LIBRARY DESTINATION lib
+        ARCHIVE DESTINATION lib)
+install(DIRECTORY include/ DESTINATION include)


this kind of changes would be ideal to do across the code base at once

davetcoleman · 2019-02-24T21:18:14Z

moveit_core/exceptions/src/exceptions.cpp


 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());


this is a regression from moveit1 - we want to keep the namespaces

Logging changed 7138f30

As required by https://github.com/ros-planning/moveit2/pull/7/files/eaa7cf2273745737d3c27b011a240d40b78c74ca#r259640729

Follows from moveit#21

Port Kinematic_constraints to ROS2

I believe we have sufficiently debated logging and should be good to proceed with this PR

mlautman · 2019-03-11T22:07:37Z

@henningkayser Can you give this a 2nd review?

henningkayser · 2019-03-13T17:44:27Z

moveit_core/exceptions/src/exceptions.cpp

 }

 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());


Why is "%s\n" removed from the error message? This way the exception is not printed to the log

That's right, thanks for the catch @henningkayser b329df8

henningkayser · 2019-03-13T17:49:49Z

moveit_core/exceptions/src/exceptions.cpp

+#include "rclcpp/rclcpp.hpp"
+
+// Logger
+rclcpp::Logger logger_exceptions = rclcpp::get_logger("exceptions");


Suggested change

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.

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

Maybe, shall we leave this change aside for now and make a global change if we decide to use LOGGER?

I'm fine with this!

Follows from moveit#7 (review)

vmayoral · 2019-03-14T11:48:01Z

Except for #7 (comment), are we good to go here?

More Windows Fixes

… AddRuckigTrajectorySmoothing (moveit#7)

* ikfast tutorial

…anning-scene Support for planning in a given planning scene

Port exceptions submodule of moveit_core

8039e73

mlautman reviewed Feb 19, 2019

View reviewed changes

v4hn previously requested changes Feb 20, 2019

View reviewed changes

This was referenced Feb 20, 2019

Port kinematics_base submodule moveit_core #8

Merged

Port robot_model submodule of moveit_core to ROS 2 #10

Merged

anasarrak mentioned this pull request Feb 21, 2019

Collision detection fcl AcutronicRobotics/moveit2#5

Merged

davetcoleman mentioned this pull request Feb 22, 2019

Convert to ROS2 Logging #21

Closed

vmayoral added 3 commits February 24, 2019 11:53

Remove dead code

dcfdc76

remove dead code 2

9f9e4cf

Remove dead code 3

eaa7cf2

davetcoleman reviewed Feb 24, 2019

View reviewed changes

vmayoral added 2 commits February 26, 2019 11:08

Reverting change

06c07e0

As required by https://github.com/ros-planning/moveit2/pull/7/files/eaa7cf2273745737d3c27b011a240d40b78c74ca#r259640729

Exceptions submodule, fix logging

7138f30

Follows from moveit#21

davetcoleman mentioned this pull request Mar 6, 2019

Example reference logger approach #33

Closed

mlautman pushed a commit that referenced this pull request Mar 8, 2019

Merge pull request #7 from AcutronicRobotics/kinematic_constraints

ae43c39

Port Kinematic_constraints to ROS2

mlautman approved these changes Mar 11, 2019

View reviewed changes

henningkayser requested changes Mar 13, 2019

View reviewed changes

Address error log concerns

b329df8

Follows from moveit#7 (review)

vmayoral added a commit to AcutronicRobotics/moveit2 that referenced this pull request Mar 14, 2019

Address error log concerns

2bc9e7c

Follows from moveit#7 (review)

henningkayser approved these changes Mar 14, 2019

View reviewed changes

henningkayser merged commit 8a08637 into moveit:master Mar 14, 2019

lilustga referenced this pull request in lilustga/moveit2 Jul 24, 2020

Merge pull request ms-iot#7 from ooeygui/master

e1c46cc

More Windows Fixes

AndyZe pushed a commit to AndyZe/moveit2 that referenced this pull request Aug 14, 2021

Register ruckig smoothing as a plugin via xml. Fix bad constructor in…

e70d7cd

… AddRuckigTrajectorySmoothing (moveit#7)

AndyZe pushed a commit to AndyZe/moveit2 that referenced this pull request Aug 21, 2021

Register ruckig smoothing as a plugin via xml. Fix bad constructor in…

c750db8

… AddRuckigTrajectorySmoothing (moveit#7)

AndyZe pushed a commit to AndyZe/moveit2 that referenced this pull request Sep 15, 2021

Register ruckig smoothing as a plugin via xml. Fix bad constructor in…

e7e3b53

… AddRuckigTrajectorySmoothing (moveit#7)

JeisonUR mentioned this pull request Mar 17, 2022

occupancy map server crash when is executed #1126

Closed

MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022

Ikfast (moveit#7)

7bf4471

* ikfast tutorial

galou pushed a commit to galou/moveit2 that referenced this pull request Sep 7, 2022

Merge pull request moveit#7 from dolejjir7/planning-in-alternative-pl…

c74d2e8

…anning-scene Support for planning in a given planning scene

SmarkSauer mentioned this pull request Nov 24, 2022

Segmentation fault in kdl_kinematics_plugin.cpp #1762

Closed

robertokcanale mentioned this pull request Apr 26, 2023

Moveit Servo Crashes #2141

Closed

This was referenced Jul 3, 2023

moveit2 ros2 humble bug in custom robot, #2258

Closed

moveit2 ros2 humble error after goal execution #2260

Closed

patrickKXMD mentioned this pull request Oct 19, 2023

Random crash with STOMP with path constraints #2462

Closed

madgrizzle mentioned this pull request Nov 12, 2023

Uncaught 'std::out_of_range' upon instantiating MoveItPy node #2525

Open

This was referenced Dec 23, 2023

moveit2__tutorials: Failed to load any planning pipelines. moveit/moveit2_tutorials#862

Closed

moveit2__tutorials: Failed to load any planning pipelines moveit/moveit2_tutorials#851

Closed

shrutic12 mentioned this pull request Apr 10, 2024

executable 'servo_node_main' not found #2383

Closed

xuanmumy mentioned this pull request May 26, 2024

'joint_state_broadcaster' [move_group-4] terminate called after throwing an instance of 'rclcpp::ParameterTypeException' #2854

Closed

AnishNavalgund mentioned this pull request Jun 4, 2024

Robot Model not Loaded: MoveIt w/ RVIZ #2738

Open

Leopold-Klotz mentioned this pull request Jun 27, 2024

move_group Process Dying in MoveIt2 Tutorials #2887

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port exceptions submodule of moveit_core #7

Port exceptions submodule of moveit_core #7

vmayoral commented Feb 19, 2019

mlautman Feb 19, 2019

v4hn Feb 20, 2019

davetcoleman Feb 20, 2019

wjwwood Feb 20, 2019

wjwwood Feb 20, 2019 •

edited

Loading

v4hn Feb 20, 2019

v4hn Feb 20, 2019

davetcoleman Feb 20, 2019

wjwwood Feb 20, 2019 •

edited

Loading

wjwwood Feb 20, 2019

davetcoleman Feb 21, 2019

wjwwood Feb 23, 2019

wjwwood Feb 23, 2019

davetcoleman Feb 23, 2019

vmayoral Feb 24, 2019

vmayoral Mar 5, 2019

vmayoral commented Feb 24, 2019

davetcoleman Feb 24, 2019

davetcoleman Feb 24, 2019

vmayoral Mar 5, 2019

mlautman commented Mar 11, 2019

henningkayser Mar 13, 2019

vmayoral Mar 14, 2019

henningkayser Mar 13, 2019 •

edited

Loading

vmayoral Mar 14, 2019

vmayoral Mar 14, 2019

henningkayser Mar 14, 2019

vmayoral commented Mar 14, 2019

	rclcpp::Logger logger_exceptions = rclcpp::get_logger("exceptions");
	static const rclcpp::Logger LOGGER = rclcpp::get_logger("exceptions");

Port exceptions submodule of moveit_core #7

Port exceptions submodule of moveit_core #7

Conversation

vmayoral commented Feb 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmayoral commented Feb 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlautman commented Mar 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henningkayser Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmayoral commented Mar 14, 2019

wjwwood Feb 20, 2019 •

edited

Loading

wjwwood Feb 20, 2019 •

edited

Loading

henningkayser Mar 13, 2019 •

edited

Loading