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

Update Zenoh-pico from 0.7.0 to 1.0.0 #23462

Merged
merged 4 commits into from
Aug 2, 2024
Merged

Update Zenoh-pico from 0.7.0 to 1.0.0 #23462

merged 4 commits into from
Aug 2, 2024

Conversation

AlexisTM
Copy link
Contributor

@AlexisTM AlexisTM commented Jul 29, 2024

Solved Problem

Zenoh 1.0.0 is soon to be released. This upgrades Zenoh-pico version from 0.7.0 to 1.0.0.

The API of Zenoh won't be changing anymore, making the upgrades trivial from now on. This also means from this version onwards, we should feel comfortable to add features without having to re-implement the Zenoh interface from scratch.

Followup of #22017

Solution

Changelog Entry

For release notes:

Feature Zenoh 1.0
Updated the Zenoh version to 1.0.
Enables /dev/urandom for fmu-v6x

Usage

On your machine, tested in ROS 2 Jazzy

# The dds plugin uses cyclonedds serialization
sudo apt install ros-jazzy-rmw-cyclonedds-cpp
# Netplan config of your ethernet https://docs.px4.io/main/en/advanced_config/ethernet_setup.html
# I used 10.41.10.1/24 on a USB Ethernet adapter

# Install the zenoh ros2dds plugin to forward the topics
mkdir -p ~/ros2/src
cd ~/ros2/src
git clone https://github.com/eclipse-zenoh/zenoh-plugin-ros2dds/ -b dev/1.0.0
git clone https://github.com/PX4/px4_msgs
cd ~/ros2
colcon build

# Start the Zenoh router with ros2dds
source ~/ros2/install/setup.bash
export RMW_IMPLEMENTATION=rmw_cyclonedds_cpp
RUST_LOG=debug zenoh_bridge_ros2dds -l tcp/10.41.10.1:7447

PX4 setup

make px4-fmuv6x_zenoh upload
# Netman config https://docs.px4.io/main/en/advanced_config/ethernet_setup.html

# in NSH
zenoh config net client tcp/10.41.10.1:7447
zenoh config addpublisher vehicle_status vehicle_status
zenoh config addpublisher sensor_combined sensor_combined
zenoh start

Receive the data

source ~/ros2/install/setup.bash
export RMW_IMPLEMENTATION=rmw_cyclonedds_cpp
ros2 topic echo sensor_combined px4_msgs/SensorCombined
ros2 topic echo vehicle_attitude px4_msgs/VehicleAttitude

Context

This is still to be considered as highly experimental.

NOTE: Zenoh requires /dev/urandom to be available which means you should use make [platform] menuconfig and enable it under device drivers on the platform you want to use it on.

Limitations

  • PX4 crashes if the link goes down (at least in client mode)
  • I failed to use the peer mode (multicast mode)
  • The ROS2 DDS plugin requires the message type to be available to use ros2 topic echo
  • No timesync
  • No rate limiting
  • Naive CSV file for configuration, having a common configuration file between microXRCE and Zenoh would be great
  • FMU-to-FMU communication no distinction between own msgs and from incoming from Zenoh
  • No documentation yet
  • No message version checking - Different versions of PX4 may produce corrupted data (px4_msgs & px4 firmware sync)

.gitmodules Outdated Show resolved Hide resolved
@PetervdPerk-NXP
Copy link
Member

PetervdPerk-NXP commented Jul 29, 2024

Great work!
Could you run make format to fix the style issues.
Also did you test the multicast option as well?
One nice thing about zenoh is that 2 PX4 instances can communicate with each-other.

Other topics are properly declared but no data is sent (sensor_combined).

You said earlier that sensor_combined doesn't work let's try to fix that before merging this, because it used to be working before.

@AlexisTM
Copy link
Contributor Author

I changed the module source to the PX4 fork & formatted the project.

It is indeed needed to make it work with ROS 2 & check multicast before merging.

My main motivation is to put this work out is to avoid duplication of efforts.

@PetervdPerk-NXP
Copy link
Member

Btw it seems that for some odd reason zenoh-bridge-dds isn't compatible by design with rmw_zenoh

ros2/rmw_zenoh#201

The intended use case of this rmw is not to communicate with other native Zenoh applications

Which is quite unfortunate but I think it's fixable, the whole is goal of using Zenoh on PX4 is that we can work without a bridge and we can do p2p multicast. communication.

Looking at this PR ros2/rmw_zenoh#171 we need to change the keyexpr to be compatible with rmw_zenoh
<ros_domain_id>/<topic_name>/<topic_type>/<topic_hash>

@AlexisTM
Copy link
Contributor Author

AlexisTM commented Jul 29, 2024

I looked further. The sensor_combined topic actually works now, I built it all from scratch and probably had some options clashing last week.

I didn't dig rmw_zenoh as it is not on the correct version and there are over 300 z_ functions to review to go for 1.0.0. Good catch for the keyexpr! [but that can wait I suppose]

I tried using ros2dds as well as the dds compatibility layer with DDS without success as they both report:

(venv) ➜  PX4-Zenoh git:(pr-zenoh-1.0) ✗ ros2 topic echo /rc/vehicle_status px4_msgs/VehicleStatus
The passed message type is invalid

This was due to the wrong rmw being used. Switching to rmw_cyclonedds_cpp makes this work.

On the other hand, I successfully decode them manually using zenoh-python and idlc.

@PetervdPerk-NXP
Copy link
Member

You could try if the official zenoh pico dds example also has this problem.
https://github.com/eclipse-zenoh/zenoh-demos/blob/main/zenoh-pico-dds/helloworld/z_pub_cdr.c
Then it would mean that something is wrong with the ros2dds zenoh plugin.

@AlexisTM
Copy link
Contributor Author

Thank you for the hint! I will try that tomorrow :)

If you want to test the Python interface through idlc, I intentionally pushed the px4 idl file & generated library.

https://github.com/AlexisTM/zenoh-python/blob/df8029845409a3b304ba103dd4d67c00ee4819ba/examples/z_sub_px4.py#L100-L110

image

@AlexisTM
Copy link
Contributor Author

AlexisTM commented Jul 30, 2024

@PetervdPerk-NXP Actually, the ROS 2 dds part works. It was my bad as I installed ros-jazzy-desktop which I supposed (wrongly) was including rmw_cyclonedds as it is the default rmw.

I will now update the description and test the multicast.

No success for the multicast :/

@PetervdPerk-NXP
Copy link
Member

PetervdPerk-NXP commented Jul 30, 2024

Indeed good point the Zenoh DDS bridge relies on CycloneDDS.

ros2 topic echo /rt/sensor_combined px4_msgs/SensorCombined

Do you have to add rt/ to be able to subscribe on ROS2?
I would expect the following to work
ros2 topic echo sensor_combined px4_msgs/SensorCombined
ROS2 should take care of it.

@AlexisTM
Copy link
Contributor Author

I just tried it and confirm I need to use the namespace rt.

@PetervdPerk-NXP
Copy link
Member

My guess is I have been using zenoh-bridge-ros2dds but before you were using zenoh-bridge-dds. As the rt/ is a quirk of ROS 2, ros2dds adds the prefix automagically. We are publishing (with ros2dds) to rt/rt/xxx

Indeed ros2dds didn't exist yet when I was implementing it.
I'm okay with removing the rt/ on our side.

// Indicates ROS2 Topic namespace
bool _rostopic;
const char *_rt_prefix = "rt/";
const size_t _rt_prefix_offset = 3; // "rt/" are 3 chars

if (_rostopic) {
strncpy(this->_topic, (char *)_rt_prefix, _rt_prefix_offset);
if (keyexpr[0] == '/') {
strncpy(this->_topic + _rt_prefix_offset, keyexpr + 1, sizeof(this->_topic) - _rt_prefix_offset);
} else {
strncpy(this->_topic + _rt_prefix_offset, keyexpr, sizeof(this->_topic) - _rt_prefix_offset);
}
} else {

// Indicates ROS2 Topic namespace
bool _rostopic;
const char *_rt_prefix = "rt/";
const size_t _rt_prefix_offset = 3; // "rt/" are 3 chars

if (_rostopic) {
strncpy(this->_topic, (char *)_rt_prefix, _rt_prefix_offset);
if (keyexpr[0] == '/') {
strncpy(this->_topic + _rt_prefix_offset, keyexpr + 1, sizeof(this->_topic) - _rt_prefix_offset);
} else {
strncpy(this->_topic + _rt_prefix_offset, keyexpr, sizeof(this->_topic) - _rt_prefix_offset);
}
} else {

Unlike zenoh-bridge-dds we were using, zenoh-bridge-ros2dds is now
adding the rt/ prefix automagically.
@AlexisTM
Copy link
Contributor Author

AlexisTM commented Aug 2, 2024

I removed the rt/ prefix (as well as the rostopic now unused switch) and we can now subscribe to the topic without the rt/ prefix on ros side.

@PetervdPerk-NXP
Copy link
Member

I'm okay with merging this as is, hopefully rmw_zenoh will move soon to 1.0.0 as well.
Could update the instruction in the PR description without the rt/ prefix.

@dagar dagar merged commit abc629c into PX4:main Aug 2, 2024
87 of 91 checks passed
@MaEtUgR
Copy link
Member

MaEtUgR commented Aug 5, 2024

image
on main probably because
image
I guess the branch dev/1.0.0-px4 (https://github.com/PX4/zenoh-pico/commits/dev/1.0.0-px4/) was updated but the submodule commit stayed on the previous commit not containing eclipse-zenoh/zenoh-pico#560
image
and especially not getting advertised by GitHub because it's a commit not contained in any branch.

@MaEtUgR
Copy link
Member

MaEtUgR commented Aug 5, 2024

Is this what you wanted? #23492

vertiq-jordan pushed a commit to iq-motion-control/PX4-Autopilot that referenced this pull request Aug 21, 2024
* Update Zenoh-pico from 0.7.0 to 1.0.0
* Update the zenoh-pico version to use PX4/dev/1.0.0-px4
* Remove the rostopic and rt/ prefix
 * Unlike zenoh-bridge-dds we were using, zenoh-bridge-ros2dds is now adding the rt/ prefix automagically.
@bake1912
Copy link

bake1912 commented Nov 21, 2024

Hello PX4 Team,
I am encountering an issue while running the following setup:
Command: make px4_sitl_zenoh gz_x500
PX4 Commit Hash: 058fe54
I added the following Zenoh publishers:

zenoh config addpublisher fmu/out/vehicle_status vehicle_status
zenoh config addpublisher fmu/out/vehicle_global_position vehicle_global_position
zenoh config addpublisher fmu/out/vehicle_odometry vehicle_local_position
zenoh config addpublisher fmu/out/sensor_combined sensor_combined

Everything works fine when I keep the number of Zenoh publishers and subscribers under 9. However, when I try to add more publishers and subscribers, I encounter a segmentation fault like

Error CDR buffer is too small
Segmentation fault.

Could you please guide me on how to debug or resolve this issue? Is there a known limitation with the number of Zenoh publishers, or could it be related to memory allocation?

Looking forward to your advice!
Thank you

@AlexisTM AlexisTM deleted the pr-zenoh-1.0 branch November 22, 2024 11:16
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.

5 participants