-
Notifications
You must be signed in to change notification settings - Fork 337
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
Port the nodelet base class to a ROS2 component node #306
Port the nodelet base class to a ROS2 component node #306
Conversation
Most of work comes from PR ros-perception#255/ros-perception#229 by sloretz/klintan
cloudQoS() const | ||
{ | ||
rclcpp::QoS qos(max_queue_size_); | ||
return qos; | ||
} | ||
|
||
public: | ||
EIGEN_MAKE_ALIGNED_OPERATOR_NEW |
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.
It's not clear to me if this is still needed.
This doesn't add any actual functionality, there's no unit test, and the class is subject to change as other modules come online. I've decided to split it out into its own PR so that work can proceed in parallel on the various sub-packages which derive from the base class |
tf_listener_(tf_buffer_) | ||
{ | ||
{ | ||
rcl_interfaces::msg::ParameterDescriptor desc; |
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 really unusual way to declare parameters, can you explain why this is necessary? Why read only?
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.
To be honest, the reason is because I just took the implementation from pr #255 as a straight copy and didn't think much about it. I'll have to read up on how to declare parameters in ROS2 to do this right.
typedef ModelCoefficients::ConstSharedPtr ModelCoefficientsConstPtr; | ||
|
||
/** \brief Constructor. */ | ||
PCLNode(std::string node_name, const rclcpp::NodeOptions & options) |
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.
I think to be a component node, that the constructor can only take in options
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.
same comment as above
too half baked. Siddarth is taking over the porting of |
I was trying this out along with enabling filter.cpp. The compilation is going through with a few changes on foxy release. If I just want to test if a some basic filter works what would be the easiest test case? Will I need to write it from scratch? No tests seem to be enabled as of now. |
Most of work comes from PR #255/#229 by sloretz/klintan