Filters as ROS2 components #758
Conversation
| return o; | ||
| } | ||
|
|
||
| EkfFilter::EkfFilter(const rclcpp::NodeOptions & options) |
There was a problem hiding this comment.
Why not just register the EKF / UKF files? https://github.com/cra-ros-pkg/robot_localization/blob/humble-devel/src/ukf.cpp If the alpha/beta/kappa params are stuck into the UKF file itself, there shouldn't be a need for these separate objects for componentizing
There was a problem hiding this comment.
Especially if we don't need a shared pointer that you changed to this (assuming that works)
There was a problem hiding this comment.
Hi, happy to share my findings... Of course I tried in a first iteration not to define the classes you saw, but just exposing existing ones.
One problem there is that the RosFilter<> extracts the node name in the constructor (thus, I needed to inject it manually, since component instantiation is a standardized procedure).
One other problem is that the usage of shared_from_this() lead to bad_weak_ptr exceptions due to the fact that component instantiation seems not to create the node as a shared pointer.
Third, I wanted to mimic the behaviour of the Ekf and Ukf nodes initialization.
Let me know if you need further demonstration of the first two points, I can make some branches demonstrating the various approaches I tried.
There was a problem hiding this comment.
extracts the node name in the constructor
@ayrton04 are you against just having the names default to filter_node or something so that this isn't a problem?
Yeah, I understand the shared pointer issue well, those changes would be needed either way
There was a problem hiding this comment.
Yeah, changing the way node name is set in the mainline code would be a simplification.
One last thing: to expose RosFilter as a component, the risk is to have a component named "robot_localization::RosFilter<robot_localization::Ekf>" since I tried in a lot of ways (failing) to use the typedef "RosEkf". I think that typedefs are not visible at linking/loading scope, as a symbol. I digged with "nm" tool in the shared library to confirm my theory.
There was a problem hiding this comment.
Believe that would be true
There was a problem hiding this comment.
So, are there requested changes at the end?
There was a problem hiding this comment.
Well, from the discussion I though we ruled out some initial comments from being implemented
There was a problem hiding this comment.
You're still needing a shared pointer in the constructor, even if you call it in another function. The scope of the constructor is not completed yet so there's no reference counter on the shared pointer. Thus, this still has the same issues as before. If you're finding that changing to this works, then that would work too for the componentization of the EKF/UKF themselves without wrappers.
If we change the name to some logical default, that deals with that issue.
robot_localization::RosFilter<robot_localization::Ekf> is not super clean, I will admit, but I think if you register the type def instead of the object, that should work. Either way, that would still be fine semantically
This patch make Ekf and Ukf loadable as ROS2 components.
This should allow reduction in transport latency from sensor nodes that are loadable as components too: composing the measurement sources with the filter can shorten the pub/sub overhead, by using intra-process communication.