Skip to content

Fixing the unit test for the logging interface change in rcl.#582

Closed
nburek wants to merge 3 commits intoros2:masterfrom
nburek:logging
Closed

Fixing the unit test for the logging interface change in rcl.#582
nburek wants to merge 3 commits intoros2:masterfrom
nburek:logging

Conversation

@nburek
Copy link
Copy Markdown

@nburek nburek commented Nov 13, 2018 •
edited by tfoote
Loading

Connects to ros2/rcl#327

Summary

This change is part of a set of changes for new logging features. These changes include the following features:

rcutils

  • Slight refactor to the buffer used in logging to make it more generic and reusable
  • A new output handler that allows multiple output handlers to be set (default is still only the console output function). This includes the following three output handlers: 1) The console output handler 2) The external logging lib output handler 3) The rosout output handler
  • Change to the output format to accept only the log string instead of the format and vargs. The log will be formatted before being sent to the output so that there will be a consistent format across all the outputs
  • Added an interface for an external logging library, an output handler function for sending logs to this library, and cmake changes so that the external logging lib can be specified (default is rc_logging_log4cxx)

rcl

  • Added new command line parameters for disabling various logging output handlers
  • Added new command line parameter for passing in a config file to the external logging library
  • Called the new configuration function in the rcutils logging to configure the logging during the ROS init process

rclcpp

  • Fixing a broken unit test

rc_logging_log4cxx

  • An external logging implementation that uses log4cxx under the hood
  • Sets up a default FileAppender logger to forward logs to files
  • Uses the process Id as the filename by default

Remaining Feature Work

  • The rosout handlers is stubbed out in rcutils right now, but more work will need to be done to actually support sending logs to a rosout topic on a node. This will likely include moving the rosout handler up to rcl so that it can use the node interfaces to publish.

Links

https://discourse.ros.org/t/ros2-logging/6469
https://github.com/nburek/rc_logging_log4cxx

Pull requests for change

rclcpp: #582
rcl: ros2/rcl#327
rcutils: ros2/rcutils#127

@tfoote tfoote added the in review Waiting for review (Kanban column) label Nov 13, 2018
This was referenced Nov 13, 2018
Merged
Merged
@tfoote tfoote mentioned this pull request Nov 14, 2018
Closed
@tfoote
Copy link
Copy Markdown
Contributor

tfoote commented Nov 15, 2018

@nburek fyi if you put an entry like:
Connects to ros2/rcl#327 in the description it will get grouped in https://waffle.io/ros2/ros2 where we triage and review tickets. Just pick a main issue/ticket and connect the others to it.

@nburek
Copy link
Copy Markdown
Author

nburek commented Nov 16, 2018

@tfoote Cool, thanks for adding that to this set of reviews. I'll make sure to mark them with Connect to * in the future.

@tfoote tfoote requested a review from wjwwood November 20, 2018 23:45
tfoote
tfoote approved these changes Nov 21, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

This looks fine pending approval of the connected change.

@nburek nburek force-pushed the logging branch 2 times, most recently from ee354e0 to 14efb7b Compare December 4, 2018 05:00
@nburek
Copy link
Copy Markdown
Author

nburek commented Dec 4, 2018

Rebased on master.

wjwwood
wjwwood reviewed Dec 6, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

0e619a8 looks ok to me, though if we go through with reverting the signature change for the output handler as I suggested in the rcutils pr, then this pr also will go away.

I don't understand why the style changes are required. Maybe this just needs to be rebased?

wjwwood
wjwwood reviewed Dec 6, 2018
View reviewed changes
rclcpp/test/test_logging.cpp
EXPECT_EQ(RCUTILS_LOG_SEVERITY_DEBUG, g_last_log_event.level);
EXPECT_EQ("name", g_last_log_event.name);
EXPECT_EQ("message 3", g_last_log_event.message);
EXPECT_EQ("[DEBUG] [name]: message 3", g_last_log_event.message);
Copy link
Copy Markdown
Member

@wjwwood wjwwood Dec 6, 2018

Choose a reason for hiding this comment

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

Changes like this will probably go away after changes I suggested in rcutils.

@nburek
Copy link
Copy Markdown
Author

nburek commented Dec 6, 2018

This entire PR will probably be discarded after the change. I'll run tests to verify though and then close it out if it's no longer needed.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Dec 6, 2018

This entire PR will probably be discarded after the change. I'll run tests to verify though and then close it out if it's no longer needed.

That's ok, I just wanted to make sure it was ready if needed.

@nburek nburek closed this Dec 6, 2018
@tfoote tfoote removed the in review Waiting for review (Kanban column) label Dec 6, 2018
nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022
@dirk-thomas
make rosout publisher transient local with a depth of 1000 (ros2#582)
74a1e7f 
Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood approved these changes

@tfoote tfoote tfoote approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nburek @tfoote @wjwwood