-
Notifications
You must be signed in to change notification settings - Fork 317
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
Use logging #190
Use logging #190
Conversation
rclcpp/minimal_service/main.cpp
Outdated
@@ -25,7 +25,9 @@ void handle_service( | |||
const std::shared_ptr<AddTwoInts::Response> response) | |||
{ | |||
(void)request_header; | |||
printf("request: %" PRId64 " + %" PRId64 "\n", request->a, request->b); | |||
RCLCPP_INFO( | |||
rclcpp::get_logger("minimal_service"), |
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.
for free-function callbacks we've been capturing the logger in a callback and passing it to the handler: ros2/demos@52dbdac
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 thought this was unnecessary complexity here for just passing a string around so I didnt do it on purpose.
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 string so you mean? If it's the logger name then the purpose is to have it reflect remapping on the node name (which isn't possible today but they'd be set up for when it's possible)
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 meant the logger. I think that as it stands it doesnt deserve doubling the size of the main to propagate it. I integrated @wjwwood suggestion (#191) instead.
If it's the logger name then the purpose is to have it reflect remapping on the node name
Yeah indeed that's important to leverage when this becomes available. I was just discussing this with wjwwood and we agreed that hopefully by then we will provide an easy way for user to pass additional argument to callbacks as logging from callback is something very common.
* possible simple solution to free function logging * typo
d3d9fbb
to
b38ceb6
Compare
@mikaelarguedas to avoid conflicts with #192 I added 71d215e |
|
||
void handle_service( | ||
const std::shared_ptr<rmw_request_id_t> request_header, | ||
const std::shared_ptr<AddTwoInts::Request> request, | ||
const std::shared_ptr<AddTwoInts::Response> response) | ||
{ | ||
(void)request_header; | ||
printf("request: %" PRId64 " + %" PRId64 "\n", request->a, request->b); | ||
RCLCPP_INFO( | ||
g_node->get_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.
Segfaults since the global variable is never set.
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.
Actually this example works fine. I think you're referring to the not composable subscriber. See #195 for a fix
I also changed the message printing from
Publishing: [%s]
toPublishing: '%s'
as[INFO] [publisher_node]: Publisher: [Hello, world! 0]
was getting a bit full of[
and harder to scope the user printed message.In Python it prints
Publishing: "%s"
but I can change it if we want the messages to be identical in both languages