Add rmw zenoh to nightly ci builds#5516
Conversation
|
Two of the tests are still failing, this PR is not ready for review |
There was a problem hiding this comment.
Please leave the default as is before merging, but I understand it’s useful to test in CI for now.
Also, why did the docking test need to be updated? Did you find root cause or is that a workaround for zenoh to work? If a workaround, that might indicate a problem in the RMW if there’s not a logical error in the test.
Will do after all tests pass
I will explain the changes I made to the test here:
I can actually reproduce this as well in cyclonedds when running 1000 times repeatedly
We can actually decrease |
|
Ok sounds good to me. I might prefer spinning for those 10ms rather than having a background spin & 10ms sleeps, but I generally don’t nitpick over tests. Are you working on the last failure in CI? The rest LGTM |
Yes, it looks like that we need to overwrite the zenoh config at least for the CI ros2/rmw_zenoh#783 (comment), what do you think about that? |
|
It seems to me like something that should be addressed in the RMW / Zenoh layers. If this is a quirk of ROS 2, then either that should change, RMW Zenoh should adjust, or the default configurations should handle it. |
I share the same opinion. Admittedly, I am not a zenoh expert, so I think I might need to leave this PR open until we can find a solution together with the zenoh maintainers |
|
OK - did you open a ticket on this with the RMW? Might be good and then link this PR |
Good idea, I have open a new issue in the rmw repo |
|
This pull request is in conflict. Could you fix it @mini-1235? |
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
f60a807 to
723ac34
Compare
Signed-off-by: Maurice <mauricepurnawan@gmail.com>
723ac34 to
044b6ec
Compare
|
@SteveMacenski, I have just rebased this, now waiting for next rmw zenoh's release to pass all tests |
|
OK! Ping me when we can take a look again! |
I tried using
New test failing, and this is exactly the same error reported in #5470, I will try to reproduce it locally |
|
I cannot reproduce this locally, but I think I understand the cause. When AMCL is on configure stage, it calls navigation2/nav2_amcl/src/amcl_node.cpp Line 75 in 47327a5 In the function, it uses the variable navigation2/nav2_amcl/src/amcl_node.cpp Lines 1453 to 1460 in 47327a5 However, this two variables are initialized later when calling navigation2/nav2_amcl/src/amcl_node.cpp Line 80 in 47327a5 That's probably why we are seeing NaNs here #5470 (comment) I think we need to move |
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 16 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
|
I did some modifications:
|
I think then we either need to have something that checks for Zenoh set on compilation so we do this for Zenoh automatically or we update the Development Guide / Migration Guide to mention the fact that if you use Zenoh this is required to run the Router for tests to pass |
|
Otherwise LGTM! |
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
I don't think this is necessary. From my experience, I usually start the Zenoh router in a separate terminal and build without isolated tests. If we run This aligns with normal Zenoh usage and is not specific to tests. However, if you prefer, I can also add a note in the migration guide mentioning that building with isolated options is also possible. |
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
|
Switching back to cyclonedds now :) |
Please - I think things like that can save some poor new developer a solid few hours if we leave a breadcrumb for them |
* Add rmw zenoh cpp to ci Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Linting Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Sourcing ros Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Use spin all to avoid flaky tests Signed-off-by: Maurice <mauricepurnawan@gmail.com> * Fix test Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Fix order in AMCL Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Implement nav2_add_test functions Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Use nav2_add_test Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Lint Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Update key Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Debug circleci Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Revert key change, try adding cmake args Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Edit config Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Update CMakeLists.txt Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Update config Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Update config Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Update config Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Try updating key Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Default isolated Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Try updating key Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Fix collision monitor test Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Add timeout and directory back Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Switch back to cyclonedds Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> --------- Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> Signed-off-by: Maurice <mauricepurnawan@gmail.com>
* Add rmw zenoh cpp to ci Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Linting Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Sourcing ros Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Use spin all to avoid flaky tests Signed-off-by: Maurice <mauricepurnawan@gmail.com> * Fix test Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Fix order in AMCL Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Implement nav2_add_test functions Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Use nav2_add_test Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Lint Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Update key Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Debug circleci Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Revert key change, try adding cmake args Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Edit config Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Update CMakeLists.txt Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Update config Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Update config Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Update config Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Try updating key Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Default isolated Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Try updating key Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Fix collision monitor test Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Add timeout and directory back Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> * Switch back to cyclonedds Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> --------- Signed-off-by: mini-1235 <mauricepurnawan@gmail.com> Signed-off-by: Maurice <mauricepurnawan@gmail.com> Signed-off-by: Decwest <fumiyaonishi1016@gmail.com>
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Description of how this change was tested
Future work that may be required in bullet points
For Maintainers:
backport-*.