Use logging macros [logger name instead of object] #191
Conversation
| @@ -32,13 +34,14 @@ void print_usage() | |||
|
|
|||
| void chatterCallback(const std_msgs::msg::String::SharedPtr msg) | |||
| { | |||
| std::cout << "I heard: [" << msg->data << "]" << std::endl; | |||
| RCLCPP_INFO(g_logger_name, "I heard: [%s]", msg->data.c_str()); | |||
There was a problem hiding this comment.
I'm not really a fan of this but it's the most direct approach for demos with one node. Other options here are to change the callback signature and use std::bind (it will be a bit clunky because of ros2/rclcpp#273, so I avoided it for our "minimalist" demo) or add a standard callback signature to rclcpp that includes the logger, but if everyone is using composition then that signature "shouldn't" be needed.
Speaking of which, it's probably time to convert these demos to at least inherit from node, which will remove this less-than-ideal logging practice. I'll do that.
dhood
added
in progress
This reverts commit 32140d9.
dhood
added
in review
dhood
force-pushed
the
logging_macros
branch
from
bd071b5
to
52dbdac
Compare
|
To make reviewing easier, rather than change all of the files at once, I've done a sample of files, and then I'll propagate the conventions to the rest of them. Please see the individual commits for 4 different "types" of conversion. |
|
Another point to take note of: in the past we've been against semicolons on macros. Since we will likely require semicolons at the end of macros in the future (e.g. to limit the scope of locally defined variables as is done in other rcutils macros and in rosconsole logging macros), I have not removed them from these logging macro calls. |
|
Never mind, I don't want to hold this up bringing in more discussion points than necessary (plus, limiting the scope does not always imply requiring a semicolon). I've removed the semicolons in fb69a37 so the logging macro calls are semi-colon-less as in other parts of the code base. Please review, @ros2/team |
There was a problem hiding this comment.
LGTM, some calls look to be more debug than info but maybe everything should be info as it is a demo... not sure.
I guess we want this to be merged before the alternative approach suggested in #192 is merged?
It's the other way around (for the demos that need to be swapped to inheritance; the demos in this PR don't need to be changed so there is no 'blocker'). Thanks for the reviews on the subset of files: after CI with #189 I will merge this PR so we don't have to re-review these parts, but there will be a followup PR including the changes to the other demos. |
|
CI including #189:
|
This currently has a few files converted: 4 commits for 4 different "types" of conversion.
Please give feedback on this and then I'll do the rest of them matching the conventions.
Remember that the macros are taking a name instead of a logger object at the moment:
node->get_name()will one day be replaced bynode->get_logger().