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

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

Closed
wants to merge 3 commits into from

Conversation

nburek
Copy link

@nburek nburek commented Nov 13, 2018

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
Copy link
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
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
Copy link
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
Author

nburek commented Dec 4, 2018

Rebased on master.

Copy link
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?

@@ -89,27 +89,27 @@ TEST_F(TestLoggingMacros, test_logging_named) {
}
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
Member

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
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
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants