fuse -> ROS 2 : Port Logging #279
Conversation
methylDragon
force-pushed
the
rolling-logging
branch
3 times, most recently
from
df05a41
to
3581bd7
Compare
methylDragon
force-pushed
the
rolling-logging
branch
2 times, most recently
from
a75269b
to
e65f256
Compare
methylDragon
changed the base branch from
rolling
to
rolling-fuse_core-common
| @@ -135,6 +136,7 @@ class AsyncPublisher : public Publisher | |||
| protected: | |||
| ros::CallbackQueue callback_queue_; //!< The local callback queue used for all subscriptions | |||
| std::string name_; //!< The unique name for this publisher instance | |||
| // rclcpp::Node::SharedPtr node_; //!< The node for this publisher (TODO(CH3): Uncomment when it's time) | |||
There was a problem hiding this comment.
This might end up being a shared pointer to the NodeTopicsInterface , so that lifecycle nodes can be used too
There was a problem hiding this comment.
Comment updated 771e4f4
Pending merge of ros2/rclcpp#2041 , but if we expect that to take too long I can duplicate an implementation in fuse_core...
There was a problem hiding this comment.
Reverting the comment update since it turns out we do have to store a SharedPtr to a node after all (for now at least)
e9a0a40
| @@ -190,7 +190,7 @@ CERES_OPTION_STRING_DEFINITIONS(VisibilityClusteringType) | |||
| * @param[in] default_value - A default value to use if the provided parameter name does not exist | |||
| * @return The loaded (or default) value | |||
| */ | |||
| template <class T> | |||
| template <class T> // TODO(CH3): Replace nh with a node | |||
There was a problem hiding this comment.
I'd recommend it takes a NodeLoggingInterface and NodeParamsInterface with another convenience method that gets those off of "Node like" objects. That will allow it to work with Lifecycle nodes too.
There was a problem hiding this comment.
Comment updated 771e4f4
Pending merge of ros2/rclcpp#2041 , but if we expect that to take too long I can duplicate an implementation in fuse_core...
There was a problem hiding this comment.
Reverting the comment update since it turns out we do have to store a SharedPtr to a node after all (for now at least)
e9a0a40
| @@ -70,7 +70,7 @@ struct Odometry2DPublisherParams : public ParameterBase | |||
| * | |||
| * @param[in] nh - The ROS node handle with which to load parameters | |||
| */ | |||
| void loadFromROS(const ros::NodeHandle& nh) final | |||
| void loadFromROS(const ros::NodeHandle& nh) final // TODO(CH3): Replace with a NodePtr | |||
There was a problem hiding this comment.
NodeParamsInterface probably
There was a problem hiding this comment.
Comment updated 771e4f4
Pending merge of ros2/rclcpp#2041 , but if we expect that to take too long I can duplicate an implementation in fuse_core...
There was a problem hiding this comment.
Reverting the comment update since it turns out we do have to store a SharedPtr to a node after all (for now at least)
e9a0a40
| @@ -84,12 +85,14 @@ bool findPose( | |||
| } | |||
| catch (const std::exception& e) | |||
| { | |||
| ROS_WARN_STREAM_THROTTLE(10.0, "Failed to find a pose at time " << stamp << ". Error" << e.what()); | |||
| RCLCPP_WARN_STREAM_THROTTLE(rclcpp::get_logger("pose_2d_publisher"), rclcpp::Clock(), 10.0 * 1000, | |||
There was a problem hiding this comment.
Any places where we don't have a logger, I'd recommend using the package name as the logger name. I think that will make it easier for users to know where messages are coming from.
methylDragon
force-pushed
the
rolling-fuse_core-common
branch
4 times, most recently
from
e5c0244
to
095dfa1
Compare
methylDragon
force-pushed
the
rolling-logging
branch
from
e65f256
to
30b82ed
Compare
Signed-off-by: methylDragon <methylDragon@gmail.com>
Signed-off-by: methylDragon <methylDragon@gmail.com>
methylDragon
force-pushed
the
rolling-logging
branch
from
30b82ed
to
c4f48a8
Compare
Signed-off-by: methylDragon <methylDragon@gmail.com>
methylDragon
changed the base branch from
rolling-fuse_core-common
to
rolling
Signed-off-by: methylDragon <methylDragon@gmail.com>
This reverts commit 771e4f4.
See: #276
This PR ports logging calls to RCLCPP across the entire stack.
Note that it won't compile since only logging's been ported (and that requires ROS 2 constructs like node pointers, which have yet to be integrated.)
Additionally, the delayed throttle filter was ported to use std::chrono to separate its functionality from ROS. (Also because ROS doesn't support negative time (: )
It's recommended to take a look at how the logging calls have been ported: 3581bd7
I also left behind comments for future edits (mostly porting node handles -> nodes)
PS: Would a std::chrono::steady_clock be preferable for the console throttler? It's a bit of a roundabout to be using it if yes, so if it's not critical I'd continue using system_clock.
Also pinging @svwilliams for visibility.