search

ros2 / rmw_iceoryx

rmw implementation for iceoryx
Apache License 2.0
144 stars 27 forks source link

Refactor the code with cpp api for iceoryx release Almond(1.0.1). #42

Closed ZhenshengLee closed 3 years ago

ZhenshengLee commented 3 years ago
  1. Refactor code with cpp api, alter the interface for iceoryx 1.0.1.
  2. Refactor iceoryx_ros2_bridge code left with a listener-adding work to do.
  3. Update the workflow description file according to the work in master branch.
  4. (done)Leaving waitset function to be done.
  5. (done )Leaving listener function issue#707 to be solved by iceoryx team.
  6. (commented the error msg)Remain waitset error msgs to debug.
  7. (commented the error msg)Remain some other todo tags to be done.

With more essential commits provided by @mossmaurice.

  1. Rework changes from ZhenshengLee
  2. Replace IceoryxGuardCondition with popo::UserTrigger
  3. Add SubscriberOptions to introspection subscriber
  4. Fix rmw_wait.
  5. Documentation, formating, and other debt payments.
budrus commented 3 years ago

@ZhenshengLee Thanks for starting the update. We are just finalizing the iceoryx 1.0.0 release. After that I'll do a review

clalancette commented 3 years ago

This looks like an API change, and we are past the Galactic API/feature freeze. Assuming that is the case, we're going to have to hold off on this one until after the Galactic release.

budrus commented 3 years ago

This looks like an API change, and we are past the Galactic API/feature freeze. Assuming that is the case, we're going to have to hold off on this one until after the Galactic release.

@clalancette This is not related to the iceoryx usage in Galactic we worked on in the last weeks. In Galactic, iceoryx is an optional shared memory transport for Cyclone and the rmw used is rmw_cyclone. rmw_iceoryx is an iceoryx stand-alone rmw that first needs to be updated to the new iceoryx API (this PR from @ZhenshengLee is a first step and also for the foxy branch). It is also not yet feature complete. It could become an IPC focused rmw in future but this is more a topic for the H release.

I'm not sure if rmw_iceoryx is even integrated into rolling and the build farm. I guess @Karsten1987 could give answers here

clalancette commented 3 years ago

@clalancette This is not related to the iceoryx usage in Galactic we worked on in the last weeks. In Galactic, iceoryx is an optional shared memory transport for Cyclone and the rmw used is rmw_cyclone.

Ah! You are totally right. Sorry, I got confused. In that case, no worries, carry on here :).

ZhenshengLee commented 3 years ago

Thanks for reviewing! I will push more commis in days.

ZhenshengLee commented 3 years ago

Hey, I just rebase the code with @mossmaurice branch and renew this PR. @budrus May this PR be accepted, although only simple a pub&sub function is working, it's a good start and more issues will be created.

mossmaurice commented 3 years ago

Thanks @ZhenshengLee! Let me try adding the WaitSet binding in the next few days. Then I think we would be good to go for a merge.

mossmaurice commented 3 years ago

@ZhenshengLee I added the WaitSet, run ament_uncrustify over all files among some other minor changes. Can you rebase all the additional commits from my branch update-rmw-to-iceoryx-almond?

I've also checked that things work with the latest release v1.0.1. Maybe you can adapt the PR title.

Thanks!

Next steps:

@Karsten1987 Could you create a rmw_iceoryx v1.0.1 release and re-enable all CI jobs once this PR is merged?

@elfenpiff FYI

ZhenshengLee commented 3 years ago

@mossmaurice Hey, I just cannot reproduce ros2cli behavior like RMW_IMPLEMENTATION=rmw_iceoryx_cpp ros2 topic list

image

I wonder how to debug ros2cli program, to treat them as python scripts?

my env:

mossmaurice commented 3 years ago

@mossmaurice Hey, I just cannot reproduce ros2cli behavior like RMW_IMPLEMENTATION=rmw_iceoryx_cpp ros2 topic list

Weird, it works for me :thinking: Can you try the following, while the pub/sub/RouDi example is running:

ros2 daemon stop
RMW_IMPLEMENTATION=rmw_iceoryx_cpp ros2 topic list # This will read in the correct env var and restart the ros2 daemon
ZhenshengLee commented 3 years ago

@mossmaurice Hey, I just cannot reproduce ros2cli behavior like RMW_IMPLEMENTATION=rmw_iceoryx_cpp ros2 topic list

Weird, it works for me Can you try the following, while the pub/sub/RouDi example is running:

ros2 daemon stop
RMW_IMPLEMENTATION=rmw_iceoryx_cpp ros2 topic list # This will read in the correct env var and restart the ros2 daemon

After ros2 daemon stop, ros2cli mentioned in readme.md works fine.

ZhenshengLee commented 3 years ago

Hey, I added support to rqt tools with implementation of get_publishers_info_by_topic based on iceoryx introspection topic, please have a review ; ) @mossmaurice

Thanks.

ZhenshengLee commented 3 years ago

@Karsten1987 Could you add me as the member of this repo so that I can create branch galactic and maintain ci config？

mossmaurice commented 3 years ago

@Karsten1987 Could you add me as the member of this repo so that I can create branch galactic and maintain ci config？

@budrus @Karsten1987 I would also volunteer to maintain rmw_iceoryx

ZhenshengLee commented 3 years ago

I tested with lgsvl, and found a segmentfault . @mossmaurice 

zs@zs-3630:~$ ros2 topic echo /tf
2021-07-06 10:39:44.892 [ Debug ]: Application registered management segment 0x7fe178bad000 with size 60126248 to id 1
2021-07-06 10:39:44.893 [ Debug ]: Application registered payload data segment 0x7fe16fcd0000 with size 149264720 to id 2
Segmentation fault (core dumped)
ZhenshengLee commented 3 years ago

Bug found. @mossmaurice

but it is OK with ros2 run rqt_image_view rqt_image_view

ros2 run v4l2_camera v4l2_camera_node

rviz2

# add camera or image topic to subscriber
>>> [rcutils|error_handling.c:108] rcutils_set_error_state()
This error state is being overwritten:

  'Handle's typesupport identifier (rosidl_typesupport_c) is not supported by this library
, at /tmp/binarydeb/ros-foxy-rosidl-typesupport-c-1.0.2/src/type_support_dispatch.hpp:116'

with this new error message:

  'failed to attach guard condition, at /home/zs/zs_ws/ga_ros2/ga_deps/rmw_iceoryx/rmw_iceoryx_cpp/src/rmw_wait.cpp:82'

Thanks.

ZhenshengLee commented 3 years ago

Bug report, ros2 bag record -a causes segfault while ros2 bag record /rslidar_points specified topics is fine.

zs@zs-3630:~$ ros2 bag record -a
Log level set to: [Warning]
[INFO] [1625574853.226275516] [rosbag2_storage]: Opened database 'rosbag2_2021_07_06-20_34_13/rosbag2_2021_07_06-20_34_13_0.db3' for READ_WRITE.
2021-07-06 20:34:13.228 [Warning]: Requested queue capacity 1000 exceeds the maximum possible one for this subscriber, limiting from 1000 to 256
[INFO] [1625574853.228191146] [rosbag2_transport]: Listening for topics...
[WARN] [1625574853.228413563] [rosbag2_transport]: Hidden topics are not recorded. Enable them with --include-hidden-topics
[INFO] [1625574853.229241649] [rosbag2_transport]: Subscribed to topic '/rslidar_points'
[INFO] [1625574853.229600856] [rosbag2_transport]: Subscribed to topic '/rosout'
[INFO] [1625574853.229893345] [rosbag2_transport]: Subscribed to topic '/parameter_events'
Segmentation fault (core dumped)

zs@zs-3630:~$ ros2 bag record /rslidar_points
Log level set to: [Warning]
[INFO] [1625574924.931204293] [rosbag2_storage]: Opened database 'rosbag2_2021_07_06-20_35_24/rosbag2_2021_07_06-20_35_24_0.db3' for READ_WRITE.
2021-07-06 20:35:24.933 [Warning]: Requested queue capacity 1000 exceeds the maximum possible one for this subscriber, limiting from 1000 to 256
[INFO] [1625574924.933614973] [rosbag2_transport]: Listening for topics...
[INFO] [1625574924.934774024] [rosbag2_transport]: Subscribed to topic '/rslidar_points'
[INFO] [1625574924.934875025] [rosbag2_transport]: All requested topics are subscribed. Stopping discovery...
ZhenshengLee commented 3 years ago

LGTM, despite the known bugs I'd suggest to merge this PR as it is getting way too big.

@ZhenshengLee Can you create separate issues on GitHub for all the bugs? Then we can fix them one by one. Thanks

OK, I will create issues when this pr get merged.

Thanks.

mossmaurice commented 3 years ago

Thanks for your work and your patience @ZhenshengLee! I created #45. I think once this is fixed we can create a foxy+v1.0.1 release followed by #48 and a galactic+v1.0.1 release.