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 script with latest zenoh api #149

Conversation

tomkimsour
Copy link

@tomkimsour tomkimsour commented Oct 15, 2024

New feature implementation

Implemented feature

Migrate zenoh to zenoh 1.0.0.b4

@tomkimsour tomkimsour force-pushed the easy-full-control-zenoh-1.0.0.b4 branch 6 times, most recently from 9fddeac to dee69c4 Compare October 15, 2024 15:41
@tomkimsour
Copy link
Author

I saw that migrating to zenoh 1.0.0 was on your todo list and I needed it so I migrated it. I also was able to run it on humble with rmf_ros2 2.7.1 tag.

Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Thanks for the huge help with the migration! Yes I was definitely planning to get to 1.0.0 soon, but was hesitant with it still being in beta, and free_fleet's lack of static and integration tests (it's WIP though!)

I've just done a first pass and left a few comments, I'll need to run some tests manually too. The merge conflict is most definitely from #146, where some minor tests and testing infra have been added

edit: just curious, have you been testing with hardware? Or with the simulation examples so far?

free_fleet_adapter/setup.py Outdated Show resolved Hide resolved
@tomkimsour tomkimsour force-pushed the easy-full-control-zenoh-1.0.0.b4 branch from dee69c4 to 0c8311d Compare October 16, 2024 08:29
Signed-off-by: thomasung <thomas.ung@pal-robotics.com>
Signed-off-by: thomasung <thomas.ung@pal-robotics.com>
Signed-off-by: thomasung <thomas.ung@pal-robotics.com>

clean dangling code
@tomkimsour tomkimsour force-pushed the easy-full-control-zenoh-1.0.0.b4 branch from 0c8311d to 9625a6f Compare October 16, 2024 08:40
Signed-off-by: thomasung <thomas.ung@pal-robotics.com>
@tomkimsour tomkimsour force-pushed the easy-full-control-zenoh-1.0.0.b4 branch from 6b3590d to eb48b1f Compare October 16, 2024 15:24
@tomkimsour
Copy link
Author

NB: I've been testing with zenoh 1.0.0 and the upgrade does not seem to break anything

@davecrawley
Copy link

davecrawley commented Oct 30, 2024 via email

Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! I've a few last comments, and after linting is fixed, we should be good to merge.

edit: adding the job logs, https://github.com/open-rmf/free_fleet/actions/runs/11368807346/job/32189477205

@@ -43,7 +43,7 @@ sudo apt install python3-pip ros-jazzy-rmw-cyclonedds-cpp
The dependencies `eclipse-zenoh` and `pycdr2` are available through `pip`. Users can choose to set up a virtual environment, or `--break-system-packages` by performing the installation directly.

```bash
pip3 install eclipse-zenoh==0.11.0 pycdr2 --break-system-packages
pip3 install pip install eclipse-zenoh==1.0.0b4 pycdr2 --break-system-packages
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pip3 install pip install eclipse-zenoh==1.0.0b4 pycdr2 --break-system-packages
pip install eclipse-zenoh==1.0.0 pycdr2 --break-system-packages

@@ -23,7 +23,7 @@ Supports
* [ROS 2 Jazzy](https://docs.ros.org/en/jazzy/index.html)
* [rmw-cyclonedds-cpp](https://github.com/ros2/rmw_cyclonedds)
* [Open-RMF on main](https://github.com/open-rmf/rmf)
* [zenoh-bridge-ros2dds v0.11](https://github.com/eclipse-zenoh/zenoh-plugin-ros2dds/releases/tag/0.11.0)
* [zenoh-bridge-ros2dds v1.0.0-beta.4](https://github.com/eclipse-zenoh/zenoh-plugin-ros2dds/releases/tag/1.0.0-beta.4)
Copy link
Member

Choose a reason for hiding this comment

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

We can update this to v1.0.0 now that it has been released. I just ran the same tests and it's looking good

Copy link
Author

Choose a reason for hiding this comment

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

yes, sorry I didn't see that comment, I'm using 1.0.0 now and it is indeed working the same way

Copy link
Author

Choose a reason for hiding this comment

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

should it be reverted and fixed ?

@aaronchongth aaronchongth merged commit 8cdc298 into open-rmf:easy-full-control Nov 7, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants