Update documentation about recorder and player creation via composition #1510
Conversation
roncapat
requested review from
MichaelOrlov and
hidmic
and removed request for
a team
There was a problem hiding this comment.
@roncapat Thanks for updating documentation and sorry for my late response. It was a new year holidays.
Overall looks good to me. I have only a few minor comments and suggestions.
MichaelOrlov
changed the title
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-for-2024-01-18/35779/1 |
| Play and record are fundamental tasks of `rosbag2`. However, playing or recording data at high rates may have limitations (e.g. spurious packet drops) due to one of the following: | ||
| - low network bandwith | ||
| - high CPU load | ||
| - slow mass memory | ||
| - ROS 2 middleware serialization/deserialization delays & overhead | ||
|
|
||
| ROS 2 C++ nodes can benefit from intra-process communication to partially or completely bypass network transport of messages between two nodes. | ||
|
|
||
| Multiple _components_ can be _composed_, either [statically](https://docs.ros.org/en/rolling/Tutorials/Intermediate/Composition.html#compile-time-composition-using-ros-services) or [dynamically](https://docs.ros.org/en/rolling/Tutorials/Intermediate/Composition.html#run-time-composition-using-ros-services-with-a-publisher-and-subscriber): all the composed component will share the same address space because they will be loaded in a single process. | ||
|
|
||
| A prerequirement is for each C++ node to be [_composable_](https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Composition.html?highlight=composition) and to follow the [guidelines](https://docs.ros.org/en/rolling/Tutorials/Demos/Intra-Process-Communication.html?highlight=intra) for efficient publishing & subscription. | ||
|
|
||
| With the above requirements met, the user can: | ||
| - compose multiple nodes together | ||
| - explicitly enable intra-process communication | ||
|
|
||
| Whenever a publisher and a subscriber on the same topic belong to the same _composed_ process, and intra-process is enabled for both, `rclcpp` completely bypasses RMW layer and below transport layer (i.e. DDS). Instead, messages are shared via process memory and *potentially* never copied. Some exception hold, so please have a look to the [IPC guidelines](https://docs.ros.org/en/rolling/Tutorials/Demos/Intra-Process-Communication.html?highlight=intra). |
There was a problem hiding this comment.
I am not sure if we want to re-explain the benefits from Composition in general, which is described in https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Composition.html#ros-2-unified-api good.
I would refer to this link, and keep this doc to how to use player and recorder as components only in rosbag2 package.
There was a problem hiding this comment.
btw, i just tried with the following. this is not expected result, i may be mistaken?
root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 component load /ComponentManager composition rosbag2_transport::Player
Failed to load component: Failed to find class with the requested plugin name.
root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 component load /ComponentManager composition rosbag2_transport::Recorder
Failed to load component: Failed to find class with the requested plugin name.There was a problem hiding this comment.
@fujitatomoya It works with
ros2 component load /ComponentManager rosbag2_transport rosbag2_transport::Player
need to use rosbag2_transport instead of composition.
It seems to be able to use it as you expected with ros2 component load /ComponentManager composition rosbag2_transport::Player will need to use composition instead of the ${PROJECT_NAME} here
rosbag2/rosbag2_transport/CMakeLists.txt
Lines 73 to 77 in bbbb217
There was a problem hiding this comment.
As regards to the
I am not sure if we want to re-explain the benefits from Composition in general, which is described in https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Composition.html#ros-2-unified-api good.
I would refer to this link, and keep this doc to how to use player and recorder as components only in rosbag2 package.
I have an opposite opinion and think it would be very useful to have a brief introduction with an explanation of why it is needed.
There was a problem hiding this comment.
As regards composition name. Me and @fujitatomoya think it would be better to use rosbag2 name instead of rosbag2_transport or composition.
i.e. to be
ros2 component load /ComponentManager rosbag2 rosbag2_transport::Player
There was a problem hiding this comment.
Unfortunately not easy, if you remember we already discussed during implementation. Current CMake registration macro doesn't allow as far as I understand to change package name when registering components in ament.
There was a problem hiding this comment.
There was a problem hiding this comment.
The most trivial way to achieve so would be to register the components in rosbag2 package, adding something like (draft code here):
#include "rosbag2_transport/player.hpp"
namespace rosbag2 {
class Player : rosbag2_transport::Player {
using rosbag2_transport::Player::Player;
};
}
#include "rclcpp_components/register_node_macro.hpp"
// Register the component with class_loader.
// This acts as a sort of entry point, allowing the component to be
// discoverable when its library is being loaded into a running process.
RCLCPP_COMPONENTS_REGISTER_NODE(rosbag2::Player)Then build and register it inside rosbag2 package CMakeLists.txt. Still unclear then if some component load tests would need to be moved out from rosbag2_transport too then.
There was a problem hiding this comment.
@roncapat Thanks for the proposal with a workaround in the `rosbag2 package.
I would appreciate it if you could try to make prototype changes for that.
I am sorry, that I don't have the capacity for such an experiment by myself for a while.
There was a problem hiding this comment.
Possible approach here #1603
|
@MichaelOrlov may I ask to briefly review again? Is there something else I can do? |
There was a problem hiding this comment.
@roncapat Thanks for the updates.
I would shorten example yaml files to a bare minimum with essential parameters and put links on the full yaml config files in our tests.
See my comments:
| Play and record are fundamental tasks of `rosbag2`. However, playing or recording data at high rates may have limitations (e.g. spurious packet drops) due to one of the following: | ||
| - low network bandwith | ||
| - high CPU load | ||
| - slow mass memory | ||
| - ROS 2 middleware serialization/deserialization delays & overhead | ||
|
|
||
| ROS 2 C++ nodes can benefit from intra-process communication to partially or completely bypass network transport of messages between two nodes. | ||
|
|
||
| Multiple _components_ can be _composed_, either [statically](https://docs.ros.org/en/rolling/Tutorials/Intermediate/Composition.html#compile-time-composition-using-ros-services) or [dynamically](https://docs.ros.org/en/rolling/Tutorials/Intermediate/Composition.html#run-time-composition-using-ros-services-with-a-publisher-and-subscriber): all the composed component will share the same address space because they will be loaded in a single process. | ||
|
|
||
| A prerequirement is for each C++ node to be [_composable_](https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Composition.html?highlight=composition) and to follow the [guidelines](https://docs.ros.org/en/rolling/Tutorials/Demos/Intra-Process-Communication.html?highlight=intra) for efficient publishing & subscription. | ||
|
|
||
| With the above requirements met, the user can: | ||
| - compose multiple nodes together | ||
| - explicitly enable intra-process communication | ||
|
|
||
| Whenever a publisher and a subscriber on the same topic belong to the same _composed_ process, and intra-process is enabled for both, `rclcpp` completely bypasses RMW layer and below transport layer (i.e. DDS). Instead, messages are shared via process memory and *potentially* never copied. Some exception hold, so please have a look to the [IPC guidelines](https://docs.ros.org/en/rolling/Tutorials/Demos/Intra-Process-Communication.html?highlight=intra). |
There was a problem hiding this comment.
@roncapat Thanks for the proposal with a workaround in the `rosbag2 package.
I would appreciate it if you could try to make prototype changes for that.
I am sorry, that I don't have the capacity for such an experiment by myself for a while.
There was a problem hiding this comment.
@roncapat Thanks. LGTM.
Only need to fix DCO https://github.com/ros2/rosbag2/pull/1510/checks?check_run_id=23600300567
Signed-off-by: Patrick Roncagliolo <[email protected]>
Signed-off-by: Patrick Roncagliolo <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: Patrick Roncagliolo <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: Patrick Roncagliolo <[email protected]>
Co-authored-by: Michael Orlov <[email protected]> Signed-off-by: Patrick Roncagliolo <[email protected]>
Signed-off-by: Patrick Roncagliolo <[email protected]>
Co-authored-by: Michael Orlov <[email protected]> Signed-off-by: Patrick Roncagliolo <[email protected]>
Co-authored-by: Michael Orlov <[email protected]> Signed-off-by: Patrick Roncagliolo <[email protected]>
Co-authored-by: Michael Orlov <[email protected]> Signed-off-by: Patrick Roncagliolo <[email protected]>
Signed-off-by: Patrick Roncagliolo <[email protected]>
|
Going to merge without CI since changes only in the |
Closes #1509.
Feedbacks welcomed :)