Use only a single ros node inside of rviz #197

anhosi · 2018-02-05T15:40:46Z

Fixes #174

Use the RosNodeAbstraction instead of the rclcpp::Node api
Creating a subscription via the RosNodeAbstraction is not yet possible (that is the reason for the acces to the rclcpp::Node). This will be done as a followup.

anhosi · 2018-02-05T15:54:23Z

CI:

Linux
Linux-aarch64
macOS
Windows

wjwwood

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".

wjwwood · 2018-02-05T23:03:37Z

rviz_common/include/rviz_common/ros_integration/ros_node_abstraction.hpp

-  RosNodeAbstraction(
-    const std::string & node_name,
-    std::shared_ptr<RosNodeStorageIface> ros_node_storage);
+  void add_to_executor(rclcpp::executor::Executor & executor) override;


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.

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.

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.

wjwwood · 2018-02-05T23:12:09Z

rviz_common/include/rviz_common/ros_integration/ros_node_abstraction.hpp


 private:
-  std::string node_name_;
-  std::shared_ptr<RosNodeStorageIface> ros_node_storage_;
+  rclcpp::Node::SharedPtr raw_node_;


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.

wjwwood · 2018-02-05T23:13:25Z

rviz_common/include/rviz_common/ros_integration/ros_node_abstraction_iface.hpp

-  get_topic_names_and_types() = 0;
+  get_topic_names_and_types() const = 0;
+
+  // TODO(anhosi): remove once the RosNodeAbstraction is extended to handle subscriptions


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.

anhosi · 2018-02-06T13:53:53Z

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:

goal 1a: complete abstraction of the rclcpp interface in a ROS1/ROS2 agnostic way
goal 2a: improve testability by providing interfaces that can be mocked in tests
next steps:
- extend abstraction to also cover subscriptions and executors (so far we have only a abstraction for node)
- the abstraction shall be usable without having to reference rclcpp in the rest of the rviz code
- keep the external abstraction interface independent of ROS2 specifics
  Did i understand that correctly @wjwwood?

Regarding the executor. It is currrently used only in two places:

add the rviz ros node to it (more or less at rviz start)
call spin_once within VisualizationManager::onUpdate
I don't think that this is enough to warrant a separate entity ExecutorAbstraction and tend to include spin_once function to the RosClientAbstraction which could be called from the onUpdate. The adding of the rviz ros node could then happen during RosClientAbstraction::init.

Do you prefer one big pr with all of the abstractions or several prs for probably "node abstraction finished", "subscription abstraction", "execution abstraction"?

anhosi · 2018-05-29T16:39:45Z

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 RosNodeAbstraction::add_to_executor has been removed as it is not necessary. The get_raw_node accessor is still present and necessary until a full ros abstraction has been acomplished.

anhosi · 2018-05-29T16:45:47Z

CI:

Linux
Linux-aarch64
macOS
Windows

anhosi · 2018-05-30T05:25:10Z

new CI as I used a wrong ros2.repos file in the last attempt:

Linux
Linux-aarch64
macOS
Windows
(edit: restartet cancelled MacOS and Windows builds)

anhosi · 2018-05-30T14:17:39Z

new Ci after fixing Mac build:

Linux
Linux-aarch64
macOS
Windows

Karsten1987 · 2018-06-07T21:32:40Z

I can confirm that all separate nodes for displays within RViz are gone with this patch.
After starting RViz with this patch, the only nodes visible are:

 ➭ ros2 node list
rviz
transform_listener_impl

Given that the tf2 listener is started as an external dependencies and the node name transform_listener_impl is hardcoded within, I believe there is not much which can be done here.
https://github.com/ros2/geometry2/blob/ros2/tf2_ros/src/transform_listener.cpp#L46-L47

tfoote · 2018-06-07T21:42:08Z

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

anhosi · 2018-06-12T11:43:40Z

New CI after rebase:

Linux
Linux-aarch64
macOS
Windows

wjwwood

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.

wjwwood requested changes Feb 5, 2018

View reviewed changes

wjwwood added enhancement New feature or request in progress Actively being worked on (Kanban column) labels Feb 5, 2018

wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 2, 2018

wjwwood force-pushed the feature/ros_interface_abstraction_followup branch from ba6b7fc to 15db78a Compare April 3, 2018 00:56

wjwwood added in review Waiting for review (Kanban column) and removed in review Waiting for review (Kanban column) labels May 1, 2018

wjwwood added this to the bouncy milestone May 1, 2018

wjwwood added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels May 16, 2018

anhosi force-pushed the feature/ros_interface_abstraction_followup branch from 15db78a to b8fab16 Compare May 29, 2018 16:35

anhosi force-pushed the feature/ros_interface_abstraction_followup branch from b8fab16 to 08b6511 Compare May 30, 2018 14:15

wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 6, 2018

wjwwood added the 🔥 label Jun 12, 2018

anhosi added 6 commits June 12, 2018 08:26

Cleanup: use existing typedefs for node shared pointers

1f520ac

Provide access to the rviz main node via the display context

a808675

Make RosNodeAbstraction interface const correct

d2929bc

Store the rclcpp::Node inside the RosNodeAbstraction

648f0af

- 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.

Add the main rviz node to the executor

3822108

anhosi added 5 commits June 12, 2018 09:22

Hand the central ros node to the TopicDisplayWidget

547e13e

Use only the single ros node of rviz

4f2de89

- 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.

Cleanup

c1c7248

Remove add_to_executor from node interface as it is not needed

b3f401b

- The main rviz node is added to the main executor at application start - Further interaction with the executor should not be necessary

Cleanup: consistent naming

49e9107

anhosi force-pushed the feature/ros_interface_abstraction_followup branch from 08b6511 to 49e9107 Compare June 12, 2018 10:56

anhosi mentioned this pull request Jun 12, 2018

Make tf use the rviz ros node #290

Merged

wjwwood approved these changes Jun 12, 2018

View reviewed changes

wjwwood merged commit 1def9b3 into ros2:ros2 Jun 12, 2018

rohbotics removed the in review Waiting for review (Kanban column) label Jun 12, 2018

anhosi deleted the feature/ros_interface_abstraction_followup branch June 13, 2018 07:16

ddengster mentioned this pull request May 27, 2020

ROS abstraction needs a (multithreaded) executor to add nodes to #548

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use only a single ros node inside of rviz #197

Use only a single ros node inside of rviz #197

anhosi commented Feb 5, 2018

anhosi commented Feb 5, 2018

wjwwood left a comment

wjwwood Feb 5, 2018

wjwwood Feb 5, 2018

anhosi Feb 6, 2018

wjwwood Feb 5, 2018

wjwwood Feb 5, 2018

anhosi commented Feb 6, 2018

anhosi commented May 29, 2018

anhosi commented May 29, 2018

anhosi commented May 30, 2018 •

edited

Loading

anhosi commented May 30, 2018

Karsten1987 commented Jun 7, 2018

tfoote commented Jun 7, 2018

anhosi commented Jun 12, 2018

wjwwood left a comment

Use only a single ros node inside of rviz #197

Use only a single ros node inside of rviz #197

Conversation

anhosi commented Feb 5, 2018

anhosi commented Feb 5, 2018

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood Feb 5, 2018

Choose a reason for hiding this comment

wjwwood Feb 5, 2018

Choose a reason for hiding this comment

anhosi Feb 6, 2018

Choose a reason for hiding this comment

wjwwood Feb 5, 2018

Choose a reason for hiding this comment

wjwwood Feb 5, 2018

Choose a reason for hiding this comment

anhosi commented Feb 6, 2018

anhosi commented May 29, 2018

anhosi commented May 29, 2018

anhosi commented May 30, 2018 • edited Loading

anhosi commented May 30, 2018

Karsten1987 commented Jun 7, 2018

tfoote commented Jun 7, 2018

anhosi commented Jun 12, 2018

wjwwood left a comment

Choose a reason for hiding this comment

anhosi commented May 30, 2018 •

edited

Loading