Add cout bt logs #4449
Add cout bt logs #4449
Conversation
Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
|
I don't have the time currently to do what it takes to get this merged in but I wanted to open a draft PR with the idea otherwise my branch would just slowly disappear |
|
@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues. |
| @@ -128,7 +128,8 @@ class NavigatorBase | |||
| const std::vector<std::string> & plugin_lib_names, | |||
| const FeedbackUtils & feedback_utils, | |||
| nav2_core::NavigatorMuxer * plugin_muxer, | |||
| std::shared_ptr<nav2_util::OdomSmoother> odom_smoother) = 0; | |||
| std::shared_ptr<nav2_util::OdomSmoother> odom_smoother, | |||
| std::shared_ptr<BT::StdCoutLogger> logger_cout_) = 0; | |||
There was a problem hiding this comment.
Why not the abstract logger so folks could pass in any logger they want (or have option in the higher level node to select the logger type they prefer)?
| @@ -125,7 +125,7 @@ BtNavigator::on_configure(const rclcpp_lifecycle::State & /*state*/) | |||
| navigators_.push_back(class_loader_.createUniqueInstance(navigator_type)); | |||
| if (!navigators_.back()->on_configure( | |||
| node, plugin_lib_names, feedback_utils, | |||
| &plugin_muxer_, odom_smoother_)) | |||
| &plugin_muxer_, odom_smoother_, logger_cout_)) | |||
There was a problem hiding this comment.
Where is logger_cout_ made? Shouldn't this be created in the BT Navigator to be passed onto the navigator base to share? I'm not sure why we're passing in a nullptr and its unused in the BT Navigator level unless (1) we're creating it here for all N instances to share or (2) we're keeping the logger object here to use somewhere.
(1) would mean we should be instantiating it above and then passing it in to share. Move the parameter logic here too.
(2) would mean we can remove modifying the on_configure API / storing the logger object at this high level, because its unused and breaks ABI (since its unused here anyway). That would make this fully a feature of the plugins and not of the BT Navigator instance itself.
The mixing is weird unless it would be used at a higher level
There was a problem hiding this comment.
yeah I know its weird to pass a nullptr but I got to this point to statisfy:
- There can only be one instance of StdCoutLogger so it has to be shared across all navigator plugins
- the logger can only be created with a
tree_which is only availableBehaviorTreeNavigatorand not inBTNavigator
It's possible that I missed another obvious way of implementing though
There was a problem hiding this comment.
@facontidavide this is the second time that the singleton nature of the loggers has caused us problems in Nav2. We actually removed live Groot monitoring because of it.
Would it be possible to allow multiple loggers in BT.CPP to allow for this? If we’re running into this every couple of years, I bet others are too.
There was a problem hiding this comment.
First, it is ok to remove the singleton constraint. Let me work on it.
There was a problem hiding this comment.
There was a problem hiding this comment.
Note that creating your own logger is trivial and boring. StdCout is only 30 lines of code. Are you sure that you don't want to create your own, maybe one that use RCLCPP_INFO instead of std::cout ?
There was a problem hiding this comment.
I leave that to @tonynajjar , but I agree that doing an RCLCPP log would be better so it enters the .ros/logs entries. That would probably need to live here I guess since ROS shouldn't bleed into BT.CPP.
There was a problem hiding this comment.
That would probably need to live here I guess since ROS shouldn't bleed into BT.CPP.
exactly!
|
What I need is already there 🤦♂️ Closing this PR. Let me know if you see more potential to it |
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Future work that may be required in bullet points
For Maintainers: