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

Use logging #190

Merged
merged 8 commits into from
Dec 5, 2017
Merged

Use logging #190

merged 8 commits into from
Dec 5, 2017

Conversation

mikaelarguedas
Copy link
Member

I also changed the message printing from Publishing: [%s] to Publishing: '%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

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Dec 5, 2017
@mikaelarguedas
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Though CI wont show much as examples are not tested purposefully

@@ -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"),
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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)

Copy link
Member Author

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.

@mikaelarguedas
Copy link
Member Author

mikaelarguedas commented Dec 5, 2017

ready for review

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@dhood
Copy link
Member

dhood commented Dec 5, 2017

@mikaelarguedas to avoid conflicts with #192 I added 71d215e

@mikaelarguedas mikaelarguedas merged commit e2ab494 into master Dec 5, 2017
@mikaelarguedas mikaelarguedas deleted the use_logging branch December 5, 2017 20:10
@mikaelarguedas mikaelarguedas removed the in progress Actively being worked on (Kanban column) label Dec 5, 2017

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

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.

Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants