Skip to content
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

Migration notes #1

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Migration notes #1

wants to merge 11 commits into from

Conversation

philippewarren
Copy link
Collaborator

No description provided.

README.md Outdated
version: 7.6.0
EOF
```
This combines a `rosinstall_generator` call to get all repos from the `desktop_full` pack,and additionnal dependencies. It also adds the `cv_camera` package, which was ported to ROS2 in a fork that is not available via `rosinstall_generator`, and the `xtl`, `xtensor` and `xsimd` C++ libraries using the versions they have on Ubuntu 22.04. for maximum compatibility.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pack, and

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with "bundle, and", with a space in between

Then run these commands to clone all the repos and install their dependencies:
```bash
vcs import src < ros2.humble.opentera_webrtc_ros.rosinstall
rosdep install --from-paths src --ignore-src -y --skip-keys "fastcdr rti-connext-dds-6.0.1 urdfdom_headers xsimd xtensor test_pluginlib" --rosdistro humble
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe explain why we need the --skip-keys option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explained for xsimd and xtensor, but for the others, I just linked the doc, which has them.

rosdep install --from-paths src --ignore-src -y --skip-keys "fastcdr rti-connext-dds-6.0.1 urdfdom_headers xsimd xtensor test_pluginlib" --rosdistro humble
```

You will need to apply a few patches as shown [here](https://github.com/introlab/t-top/blob/ros2-migration/tools/setup_scripts/ros2_humble_install.sh#L241-L242). The patch files are [here (raw libg2o)](https://raw.githubusercontent.com/introlab/t-top/ros2-migration/tools/setup_scripts/patch/libg2o.patch) and [here (raw octomap_msgs)](https://raw.githubusercontent.com/introlab/t-top/ros2-migration/tools/setup_scripts/patch/octomap_msgs.patch).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what the patches fix ?


You will need to apply a few patches as shown [here](https://github.com/introlab/t-top/blob/ros2-migration/tools/setup_scripts/ros2_humble_install.sh#L241-L242). The patch files are [here (raw libg2o)](https://raw.githubusercontent.com/introlab/t-top/ros2-migration/tools/setup_scripts/patch/libg2o.patch) and [here (raw octomap_msgs)](https://raw.githubusercontent.com/introlab/t-top/ros2-migration/tools/setup_scripts/patch/octomap_msgs.patch).

Create a file named `colcon_defaults.yaml` in the root of the workspace with the following content:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add optional optimisation flags for ARM ? Can we add optional optimisation flags to build the realsense driver with CUDA support ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could just link to the install script for T-Top and explain that there are optimization flags there for the people interested?

colcon build
```

This will take a while.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add time estimation for Jetson Orin / Intel Desktop ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you or @mamaheux remember a time estimate for Jetson Orin? I can rebuild on my laptop and measure it to check for mainstream Intel computer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More than 4h

README.md Outdated
3. For every message, service and action, add the `::<interface_type>` subnamespace. For instance, `std_msgs::String` becomes `std_msgs::msg::String`.
4. Every `ros::Publisher`, `ros::Subscriber` and stuff like that is now templated on the message type. You will to hunt down which thing is connected to which callback of which message type, and bring back this information where you declare the thing, probably in the header file. Also, `Subscriber` is now `Subscription`. Also, store shared ptr. For instance, if you had a `ros::Subscriber` that received `std_msgs/String` messages, you will now have a `rclcpp::Subscription<std_msgs::msg::String>::SharedPtr`.
5. For every `advertiseService`, `advertise` and `subscribe`, you will need to use the node instance as `node->create_{service/publisher/subscription}`. Also, you can't use the `(<name>, &Class::callback, this)` form anymore: use a lambda to wrap the call in a self-contained callback. You can use [a helper like this](https://github.com/introlab/opentera-webrtc-ros/blob/ros2/opentera_webrtc_ros/include/opentera_webrtc_ros/utils.h#L11-L57) if you want to help you wrap everything. Same goes for services. If you are using `image_transport`, the API has not changed and you can still use the `(<name>, &Class::callback, this)` form with `image_transport`.
6. Timers are also created from the node (`node->create_timer`), and time needs to be obtained from it as well (`node->now()/node->get_clock()->now()`). As a bonus, `Time` now lacks useful methods to convert to and from an integer number of milliseconds, so you will need to do this manually.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a bonus, ... avoid sarcasm. Please note that...

README.md Outdated
1. Move the selection/choice inside the constructor of the one single node class. This breaks the single-responsibility principle and will make the code harder to reason about, probably.
2. Create a dummy temporary node, get the parameter, and destroy the dummy node. Then, use the parameter, and create the real node based on it. There is [a C++ example of this here](https://github.com/introlab/opentera-webrtc-ros/blob/ros2/face_cropping/src/face_cropping_node.cpp#L94-L103), and [a Python example here](https://github.com/introlab/audio_utils/blob/ros2/audio_utils/scripts/resampling_node.py#L304-L319).
3. Use composition and not inheritance. Create a node instance, get the parameters you need, then pass the node instance to the constructor of the main class, which will store it and use it as its node.
7. `rclcpp::Node` inherits from `std::enable_shared_from_this`, which means that instances of `Node` are always meant to be stored inside a `shared_ptr`, and you can get a new `shared_ptr` to it even if you don't have a `shared_ptr`, but a direct reference to the node object. Most of the ROS2 API takes the node by `shared_ptr`, too. If you use composition, you will probably want to pass a reference to the `node` object to parts of your main node class: use a reference, you are guaranteed that it will live long enough because of composition, references can't be null, and they can call `node->shared_from_this()` if they ever need a shared ptr to pass to a ROS2 API function. But some things, like `image_transport`, need a `shared_ptr` to the node in their constructor. If you have composition of this inside your main node class which inherits from `Node`, you won't be able to initialize it in the constructor as `this->shared_from_this()` will not work in the constructor (you will get some sort of segmentation fault, probably). In this case, it might be easier to forgo inheritance altogether, and to use composition for the node: store a `shared_ptr<Node>` in your main node class, and use it instead of yourself when you need a node. It will act similar to a `NodeHandle` in ROS1. Place it befor the `image_transport` thing in your class, and it will be fully initialized when you need it to initialize `image_transport`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

befor -> before

README.md Outdated
- If you used to pass a launch parameter to change the `output=` of nodes, you can't anymore: it needs an hardcoded string that is either "log", "screen" or "both". A tip: use "log" (or nothing as "log" is the default), and pass the `-a` flag to the `ros2-launch` command when you need to debug: this will redirect everything to the console. You can also use the `OVERRIDE_LAUNCH_PROCESS_OUTPUT` environment variable (this is what `-a` does).
- In ROS1, `eval` tags were way easier to use than in ROS2. Now, you need a pair of quotes englobing the whole thing that you want to evaluate, which means that you'll need a bunch of escaping of strings. Also, you used to have access to substitutions using `arg('name')` inside the evaluated expression: you can't do that anymore, you need to use launch file substitutions, which are textual. This is painful, and it also means that using `eval` for doing an `OR` on two conditions, for instance, will have weird results, because it will operate on strings and not booleans. You can compare to the "true" or "false" strings explicitly, or you can use the new operators substitutions like shown [here](https://github.com/introlab/odas_ros/blob/ros2/odas_ros/launch/odas.launch.xml#L32) with `$(or ...)`.
- If you used `rosparam` tags to pass YAML structured parameters, this does not work anymore. Use `param`. You can use nested `param` tags to reproduce a nested/mapping structure.
- If you need to pass an empty array to a parameter, you will suffer. Passing `"[]"` will be rejected as the type of the array cannot be deduced. For a string array, you can use `"['']"` and filter for empty strings in your code, if you don't need empty strings usually. For numeric arrays, use a special value that you will filter that is out of the range you use, or combine the array with a boolean that chooses wether the array should be ignored/considered empty or wether should be used.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you will suffer -> you have to be careful.

README.md Outdated
### Gazebo, simulation, URDF and navigation migration
1. Need to pass `use_sim_time:=true` to every node in launch. Use special operations to set a parameter in every node (`set_parameter` tag at global scope in XML).
2. Use [this page](https://github.com/ros-simulation/gazebo_ros_pkgs/wiki) to check how to migrate a given gazebo-ros plugin.
3. For you models to appear in Gazebo, you might need a line [like this one in your `package.xml`](https://github.com/introlab/opentera-webrtc-ros/blob/ros2/turtlebot3_beam_description/package.xml#L26).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you->your

The easiest way is probably to re-create the config file from scratch by re-adding and re-configuring the components you need.
Checking a diff between your old config file and a new one using similar components can also work, migrating the changes parts that seem important, but not touching window sizes and stuff like that.

## Other tips
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a tip about multi robot configuration and node/topic/service discovery with https://docs.ros.org/en/foxy/Concepts/About-Domain-ID.html ROS_DOMAIN_ID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants