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

Adding Nav2 / Open Navigation Docking #4406

Merged
merged 14 commits into from
Jun 11, 2024
Merged

Adding Nav2 / Open Navigation Docking #4406

merged 14 commits into from
Jun 11, 2024

Conversation

SteveMacenski
Copy link
Member

@SteveMacenski SteveMacenski commented Jun 8, 2024

Thanks to NVIDIA for their support to make this possible!

Original location: https://github.com/open-navigation/opennav_docking
Tutorials: https://docs.nav2.org/tutorials/docs/using_docking.html
Configuration: https://docs.nav2.org/configuration/packages/configuring-docking-server.html
Commander API: https://docs.nav2.org/commander_api/index.html

A few known enhancements that would be great to have over time: #4405 #4404 #4403

@ros-navigation ros-navigation deleted a comment from mergify bot Jun 10, 2024
@ros-navigation ros-navigation deleted a comment from mergify bot Jun 10, 2024
@SteveMacenski
Copy link
Member Author

There's a weird issue where the docking smoke system test is failing by timing out with no logging in CI but I cannot reproduce locally. Working on it... sorry for testing in CI

@mikeferguson
Copy link
Contributor

How do you keep your ros2 launch-based tests from conflicting with each other? (like, this test publishes and subscribes stuff - is it maybe conflicting with some other test that is running? Is there some magic in ros2 launch file tests that magically assigns different ROS_DOMAIN_IDs? I haven't built a huge set of ros2 launch tests yet to see what issues arise...

@SteveMacenski
Copy link
Member Author

SteveMacenski commented Jun 11, 2024

How do you keep your ros2 launch-based tests from conflicting with each other? (like, this test publishes and subscribes stuff - is it maybe conflicting with some other test that is running? Is there some magic in ros2 launch file tests that magically assigns different ROS_DOMAIN_IDs? I haven't built a huge set of ros2 launch tests yet to see what issues arise...

The tests are run sequentially within a given package, but could be run in parallel w.r.t. other packages. In our CI settings, we have parallel-workers set to 1 (https://github.com/ros-navigation/navigation2/blob/main/.circleci/defaults.yaml#L11-L14) so that only one package is tested at a time making sure they don't conflict.

By the way, I found at least the first cause of the issue, the while loops are never exiting because the action_results aren't being populated for some reason, which implies that the result_future.add_done_callback(self.action_result_callback) or future.add_done_callback(self.action_goal_callback) are never called. Its really hard to tell which since CI isn't giving me back any logs so I'm kind of stabbing in the dark narrowing in on it:

        while len(self.action_result) == 0:
            rclpy.spin_once(self.node, timeout_sec=0.1)
            time.sleep(0.1)

I still cannot reproduce this locally but is apparently happening in Rolling on 24.04 or something peculiar in CI. I assumed at the start that may have been the issue (since it timed out rather than crashed or something), but I made sure it wasn't a regression in launch testing first - which are some of the fumbling commits you see above. I'm pretty sure now its either something we're doing or rclpy's spinning / action lib is busted (which I wouldn't put behind it since we did find some issues with Jazzy already with spinning repeating needed to make work the first time and historically we haven't relied on the result/accepted callbacks because they weren't reliable)

@SteveMacenski
Copy link
Member Author

SteveMacenski commented Jun 11, 2024

@mikeferguson in case you're curious about the outcome, basically I just swapped out all the done callbacks for the goal / result with spinning until complete and that resolved it. There seems to be some problem in spinning in Jazzy/Rolling I think, this isn't the first time I've seen something like this since working on the 24.04 migration. Putting all the action work inline and blocking seems to resolve the issue instead of using callback functions. Ex.

        future = self.undock_action_client.send_goal_async(goal)
        rclpy.spin_until_future_complete(self.node, future)
        self.goal_handle = future.result()
        assert self.goal_handle.accepted
        result_future = self.goal_handle.get_result_async()
        rclpy.spin_until_future_complete(self.node, result_future)
        self.action_result.append(result_future.result())

Now it all works as expected 95c547b

@SteveMacenski SteveMacenski merged commit 7737a2f into main Jun 11, 2024
9 of 10 checks passed
@SteveMacenski SteveMacenski deleted the docking_tmp branch June 11, 2024 03:49
Marc-Morcos pushed a commit to Marc-Morcos/navigation2 that referenced this pull request Jul 4, 2024
* adding docking temp build test

* add missing exmport

* fix test

* adding server to bringup

* add docking server to Nav2 package; migrate to msgs package

* migrate msgs

* in line versions with rest of stack

* removing docking msgs from main server cmake

* fix bug

* linting and logging

* bump cache

* fix tests with rclpy API action spinning

* add full metapackage repos
savalena pushed a commit to savalena/navigation2 that referenced this pull request Jul 5, 2024
* adding docking temp build test

* add missing exmport

* fix test

* adding server to bringup

* add docking server to Nav2 package; migrate to msgs package

* migrate msgs

* in line versions with rest of stack

* removing docking msgs from main server cmake

* fix bug

* linting and logging

* bump cache

* fix tests with rclpy API action spinning

* add full metapackage repos
Manos-G pushed a commit to Manos-G/navigation2 that referenced this pull request Aug 1, 2024
* adding docking temp build test

* add missing exmport

* fix test

* adding server to bringup

* add docking server to Nav2 package; migrate to msgs package

* migrate msgs

* in line versions with rest of stack

* removing docking msgs from main server cmake

* fix bug

* linting and logging

* bump cache

* fix tests with rclpy API action spinning

* add full metapackage repos
masf7g pushed a commit to quasi-robotics/navigation2 that referenced this pull request Oct 23, 2024
* adding docking temp build test

* add missing exmport

* fix test

* adding server to bringup

* add docking server to Nav2 package; migrate to msgs package

* migrate msgs

* in line versions with rest of stack

* removing docking msgs from main server cmake

* fix bug

* linting and logging

* bump cache

* fix tests with rclpy API action spinning

* add full metapackage repos
@botman93 botman93 mentioned this pull request Oct 27, 2024
7 tasks
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.

2 participants