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

Node logger through singleton (warehouse) #2445

Merged
merged 9 commits into from
Oct 24, 2023
Merged

Conversation

tylerjw
Copy link
Member

@tylerjw tylerjw commented Oct 12, 2023

Description

This creates a function in moveit utils for getting the logger and demonstrates using it by converting the whole warehouse package.

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (faa4795) 50.35% compared to head (047dbd6) 50.81%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2445      +/-   ##
==========================================
+ Coverage   50.35%   50.81%   +0.47%     
==========================================
  Files         390      392       +2     
  Lines       31954    32140     +186     
==========================================
+ Hits        16087    16329     +242     
+ Misses      15867    15811      -56     
Files Coverage Δ
...oveit/online_signal_smoothing/butterworth_filter.h 0.00% <ø> (ø)
...include/moveit/robot_trajectory/robot_trajectory.h 98.67% <ø> (ø)
...eit_core/robot_trajectory/src/robot_trajectory.cpp 66.86% <100.00%> (ø)
moveit_core/utils/src/logger.cpp 100.00% <100.00%> (ø)
...de/moveit/ompl_interface/detail/ompl_constraints.h 100.00% <100.00%> (ø)
...z_industrial_motion_planner/trajectory_generator.h 100.00% <100.00%> (ø)
...ustrial_motion_planner/trajectory_generator_circ.h 100.00% <ø> (ø)
...dustrial_motion_planner/trajectory_generator_lin.h 100.00% <ø> (ø)
...dustrial_motion_planner/trajectory_generator_ptp.h 100.00% <ø> (ø)
...strial_motion_planner/src/trajectory_functions.cpp 100.00% <ø> (ø)
... and 12 more

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tylerjw
Copy link
Member Author

tylerjw commented Oct 24, 2023

@Abishalini and @henningkayser, I think this is ready for review again.

I implemented child logging with testing for it, and I got it to work on Humble, Rolling, and Iron.

There is one annoying limitation to how I did this that I'm not sure matters, but I'll call it out here so it is easier to understand this PR. You'll notice that the child logger names only have underscores in them.

That is because of the complexity of how node names work and how these child node namespaces interact. The logger names are separated by dots ., and the node is inside namespaces separated by slashes /. On humble to get child loggers to log to /rosout. I need to just create a new node object with the name: /<root node>/<child node>. The namespace is just copied from the root node name. If you want multiple levels of this, I must do some string parsing and create nested namespaces.

I don't think this is necessary, though, because one thing that is different about ros2 for this whole logging namespacing is it is tied to node objects, which means that every child logger is going to change the full namespace name based on what node is running them. So, in ros1 one class in moveit_core would always have the logging namespace: moveit.robot_state it will now be something like move_group.moveit_robot_state because that first part is always the name of the node.

@tylerjw
Copy link
Member Author

tylerjw commented Oct 24, 2023

@clalancette here is my current attempt to get rosout logging working in moveit without changing API everywhere passing around node or logger objects.

Note that this is just changing one library but if we decide to go this way I'm going to apply this to the whole repo.

If you have a chance to look at this I'd really appreciate it. I know you said there might be some way to hook up rosout logging without nodes? Do you have an example somewhere I could read?

moveit_core/utils/include/moveit/utils/logger.hpp Outdated Show resolved Hide resolved
// Request for backport (rclpy issue) - https://github.com/ros2/rclpy/issues/1131
// MoveIt PR that added this - https://github.com/ros-planning/moveit2/pull/2445
#if !RCLCPP_VERSION_GTE(21, 0, 3)
static std::vector<std::shared_ptr<rclcpp::Node>> child_nodes;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we use a map to prevent creating redundant nodes? Not it should really matter as long as we are not spinning them

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good suggestion, I'll do this.

@tylerjw tylerjw removed the request for review from Abishalini October 24, 2023 14:26
Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
Co-authored-by: Henning Kayser <henningkayser@picknik.ai>
@tylerjw tylerjw merged commit 6a7b557 into moveit:main Oct 24, 2023
10 of 11 checks passed
@tylerjw tylerjw deleted the logger_util branch October 24, 2023 17:18
tylerjw added a commit that referenced this pull request Nov 9, 2023
tylerjw added a commit that referenced this pull request Nov 9, 2023
This reverts commit 6a7b557.

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
@tylerjw tylerjw mentioned this pull request Nov 9, 2023
tylerjw added a commit that referenced this pull request Nov 9, 2023
This reverts commit 6a7b557.

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
tylerjw added a commit that referenced this pull request Nov 9, 2023
This reverts commit 6a7b557.

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
tylerjw added a commit that referenced this pull request Nov 9, 2023
This reverts commit 6a7b557.

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
tylerjw added a commit that referenced this pull request Nov 9, 2023
This reverts commit 6a7b557.

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
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.

None yet

3 participants