-
Notifications
You must be signed in to change notification settings - Fork 212
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
Use only a single ros node inside of rviz #197
Use only a single ros node inside of rviz #197
Conversation
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.
Since you ran CI, I guess your intention is to have this reviewed for merging, but I don't feel that I can take it as-is.
I've left some notes about what I think is wrong with the approach and what we might do instead, but I can give more feedback as requested if that's not enough.
For now, I'm declining to merge and putting it "in progress" rather than "review".
RosNodeAbstraction( | ||
const std::string & node_name, | ||
std::shared_ptr<RosNodeStorageIface> ros_node_storage); | ||
void add_to_executor(rclcpp::executor::Executor & executor) override; |
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 doesn't work, since it breaks the ROS 1/ROS 2 abstraction (the whole point of this class). Instead we'll need to abstract the idea of using an executor. Also the function below that returns the "raw node pointer" also breaks this abstraction.
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.
Even if we were to put the raw node pointer function in for now, this one doesn't make sense, because you can just get the node and add it to the executor. There's really no reason that I can see to have this function on the node abstraction class.
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.
So far I thought more about refactoring towards easier testability but I guess the goals are very similar.
The accessor the the "raw node" is intended to be removed as soon as the abstraction for subscriptions is ready (which we already started working on). The add_to_executor
would then be the only way to achieve that. It was also my way of replacing DisplayContext::addNodeToMainExecutor()
.
Maybe we no longer need this function if we add the rviz ros node to the executor when we initialize the ros client abstraction. Currently the VisualizationManager
holds the executor.
|
||
private: | ||
std::string node_name_; | ||
std::shared_ptr<RosNodeStorageIface> ros_node_storage_; | ||
rclcpp::Node::SharedPtr raw_node_; |
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.
In my opinion, this is the wrong direction to be moving in for this class. The abstraction class really needs to keep ROS 2 stuff out of it (hence the private node storage class).
I'd rather see a ROS 2 specific subclass of this abstraction interface which can get the rclcpp (ROS 2) node out of the abstraction. That way this class remains abstract but you can introduce another (non ROS 1/ROS 2 agnostic) set of classes to do specific things in the meantime.
get_topic_names_and_types() = 0; | ||
get_topic_names_and_types() const = 0; | ||
|
||
// TODO(anhosi): remove once the RosNodeAbstraction is extended to handle subscriptions |
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.
Maybe this is redundant information with what I've written above, but this todo isn't sufficient. You'd also need to remove the reference to the executor above.
Maybe I should have mentioned that I added the CI jobs mainly to have more confidence in this change as I also changed how the ros nodes are stored internally. I think we are going to have a couple of iterations on this one 😃. I try to summarize how I understand your comments:
Regarding the executor. It is currrently used only in two places:
Do you prefer one big pr with all of the abstractions or several prs for probably "node abstraction finished", "subscription abstraction", "execution abstraction"? |
ba6b7fc
to
15db78a
Compare
15db78a
to
b8fab16
Compare
This PR now only contains the change to use a single ros node inside of rviz. Further extension of a ROS abstraction layer within rviz is postponed as not all necessary design issues have been decided. The |
b8fab16
to
08b6511
Compare
I can confirm that all separate nodes for displays within RViz are gone with this patch.
Given that the tf2 listener is started as an external dependencies and the node name |
As it stands yes though the tf2_ros API should be extended to support passing in the node instance so we can continue to reduce node counts. |
- No longer uses the node_name as indirect handle to the node. - The interface of node abstraction is not yet extended, also for now still only the node name is handed to the VisualizationFrame. - The ros client abstraction internally holds a shared pointer to the "one" rviz ros node (in form of a RosNodeAbstraction). Users only receive a weak pointer to the RosNodeAbstraction so that the RosClientAbstraction can perform a proper cleanup upon shutdown. - Make documentation comments describe reality.
- Intended usage: rviz should have only a single instance of RosNodeAbstraction to manage all node related ros interaction. - So there is no need to handle many node. Thus RosNodeStorage can be removed.
- All of rviz now uses only a single ros node. - As the abstraction for subscriptions is not yet present the wrapped raw rclcpp::Node has to be access in order to subscribe. This will be addressed in the future.
- The main rviz node is added to the main executor at application start - Further interaction with the executor should not be necessary
08b6511
to
49e9107
Compare
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.
lgtm
For external observers, we decided that I would reevaluate the abstraction layer in the future when I actually have time to integrate with ROS 1 again.
Fixes #174