-
Notifications
You must be signed in to change notification settings - Fork 571
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
Conversation
Codecov ReportAttention:
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
☔ View full report in Codecov by Sentry. |
@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 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: |
@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/src/logger.cpp
Outdated
// 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; |
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.
shouldn't we use a map to prevent creating redundant nodes? Not it should really matter as long as we are not spinning them
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.
This is a good suggestion, I'll do this.
Co-authored-by: Abishalini Sivaraman <[email protected]>
Co-authored-by: Henning Kayser <[email protected]>
Signed-off-by: Tyler Weaver <[email protected]>
Co-authored-by: Henning Kayser <[email protected]>
This reverts commit 6a7b557. Signed-off-by: Tyler Weaver <[email protected]>
This reverts commit 6a7b557. Signed-off-by: Tyler Weaver <[email protected]>
This reverts commit 6a7b557. Signed-off-by: Tyler Weaver <[email protected]>
This reverts commit 6a7b557. Signed-off-by: Tyler Weaver <[email protected]>
This reverts commit 6a7b557. Signed-off-by: Tyler Weaver <[email protected]>
Description
This creates a function in moveit utils for getting the logger and demonstrates using it by converting the whole warehouse package.