ros2 / demos

Apache License 2.0
493 stars 329 forks source link

Add matched event demo for rclcpp and rclpy #607

Closed Barry-Xu-2018 closed 1 year ago

Barry-Xu-2018 commented 1 year ago

Address ros2/rmw#330

Barry-Xu-2018 commented 1 year ago

README.md need to be updated.
I will update it.

Barry-Xu-2018 commented 1 year ago

a new test is required in https://github.com/ros2/demos/tree/9fbd3e840113e183b7ccc935e17fa275fdea6540/demo_nodes_cpp/test

matched_event_detect like 'service'. What output depend on 'client' behavior.
This PR doesn't provide 'client'. I want to use 'ros2 topic echo' and 'ros2 topic pub' command as 'client'.
Not sure whether current test framework can support it or not.

iuhilnehc-ynos commented 1 year ago

matched_event_detect like 'service'. What output depend on 'client' behavior. This PR doesn't provide 'client'. I want to use 'ros2 topic echo' and 'ros2 topic pub' command as 'client'. Not sure whether current test framework can support it or not.

I see. Let's hear others' opinions.

Barry-Xu-2018 commented 1 year ago

matched_event_detect like 'service'. What output depend on 'client' behavior. This PR doesn't provide 'client'. I want to use 'ros2 topic echo' and 'ros2 topic pub' command as 'client'. Not sure whether current test framework can support it or not.

I see. Let's hear others' opinions.

I change demo codes to remove dependency of roscli. So now I can add test. @iuhilnehc-ynos

fujitatomoya commented 1 year ago

lgtm, @iuhilnehc-ynos can you do another review on this? if it is good to go, please start the CI.

Barry-Xu-2018 commented 1 year ago

I updated your comments @fujitatomoya @iuhilnehc-ynos
Please check again

iuhilnehc-ynos commented 1 year ago

CI:

fujitatomoya commented 1 year ago

@Barry-Xu-2018 test with connextdds is failing, could you check the failure?

Barry-Xu-2018 commented 1 year ago

I cannot reproduce this problem in my local environment.
I guess waiting time is too short for test.
So I extend waiting time.
Please help to run CI again @iuhilnehc-ynos

iuhilnehc-ynos commented 1 year ago

CI:

Barry-Xu-2018 commented 1 year ago

@iuhilnehc-ynos Help to run CI again.

iuhilnehc-ynos commented 1 year ago

CI:

Barry-Xu-2018 commented 1 year ago

Update to codes to ensure the program does spin until the matched event occurs.

Barry-Xu-2018 commented 1 year ago

@iuhilnehc-ynos Please help to run CI again

iuhilnehc-ynos commented 1 year ago

@iuhilnehc-ynos Please help to run CI again

CI:

Barry-Xu-2018 commented 1 year ago

CI failure is related to a bug in Fastdds. https://github.com/eProsima/Fast-DDS/issues/3395 The fix was submitted. https://github.com/eProsima/Fast-DDS/pull/3396
We have to wait for this patch merged.

fujitatomoya commented 1 year ago

CI failure is related to a bug in Fastdds. eProsima/Fast-DDS#3395 The fix was submitted. eProsima/Fast-DDS#3396 We have to wait for this patch merged.

that makes sense, to avoid double triggered, update_publication_matched_status does only updating the cached publication matched event only.

iuhilnehc-ynos commented 1 year ago

CI:

Barry-Xu-2018 commented 1 year ago

CI for Linux didn't run. Please help to run Linux again @iuhilnehc-ynos

iuhilnehc-ynos commented 1 year ago

CI for Linux didn't run. Please help to run Linux again @iuhilnehc-ynos

CI is busy building some nighty jobs. Please be patient.

fujitatomoya commented 1 year ago

@clalancette @audrow can you do us a favor? we do not have merge permission to this repo.

after either of you merged this PR, i will merge the following accordingly. (or if you can do that, that would be nice!!!)

iuhilnehc-ynos commented 1 year ago

Based on https://github.com/eProsima/Fast-DDS/issues/3395#issuecomment-1491700658, we found Fast-DDS is updated again, just in case, I need to re-trigger CI:

fujitatomoya commented 1 year ago

it looks like https://github.com/eProsima/Fast-DDS/pull/3418 brings CI failure once again?

Barry-Xu-2018 commented 1 year ago

it looks like https://github.com/eProsima/Fast-DDS/pull/3418 brings CI failure once again? Yes. @MiguelCompany explain this change in https://github.com/eProsima/Fast-DDS/issues/3395#issuecomment-1491700658.

I will consider how to resolve it.

Barry-Xu-2018 commented 1 year ago

Now depend on the fix issue ros2/rmw_fastrtps#681

fujitatomoya commented 1 year ago

CI:

iuhilnehc-ynos commented 1 year ago

As ros2 repos changed a lot, I need to re-run CI with a new custom repos file.

CI:

fujitatomoya commented 1 year ago

@clalancette @mjcarroll can you merge this to rolling?