Skip to content

Migrate to Lyrical (rolling)#95

Merged
SteveMacenski merged 17 commits intoopen-navigation:mainfrom
adivardi:lyrical
Sep 3, 2025
Merged

Migrate to Lyrical (rolling)#95
SteveMacenski merged 17 commits intoopen-navigation:mainfrom
adivardi:lyrical

Conversation

@adivardi
Copy link
Contributor

@adivardi adivardi commented Aug 4, 2025 •
edited
Loading

Migrate the code to rolling:

  • Use Nav2 ROS Common & Nav2 Lifecycle Node
  • Use declare_or_get_parameter
  • remove action_server_result_timeout

(Includes changes from #94, since current main is for Jazzy)

adivardi
adivardi commented Aug 4, 2025
View reviewed changes
Copy link
Contributor Author

@adivardi adivardi left a comment

Choose a reason for hiding this comment

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

self review

@adivardi adivardi marked this pull request as ready for review August 4, 2025 16:07
SteveMacenski
SteveMacenski reviewed Aug 4, 2025
View reviewed changes
@SteveMacenski
Copy link
Member

SteveMacenski commented Aug 4, 2025 •
edited
Loading

We need to (unfortunately) build Nav2 or use the nav2_docker Rolling Nightly containers for the testing job in order to have Nav2 installed & have a workspace you can source with its packages.

adivardi and others added 9 commits August 7, 2025 07:45
@adivardi
Migrate to Kilted (open-navigation#94)
d6e41a2 
* update msg for Kilted

* [opennav_coverage_bt] migrate BT xml files

* migrate coverage_navigator

* rm error_msg from ComputeCoveragePath

* use newer rostooling docker images, run test for Kilted

* run linter on Kilted

* use base image
@adivardi
Migrate to Lyrical
c653723
@adivardi
fix all compile errors
a70d42f 
due to wrong includes and to methods that moved from nav2_util to nav2 namespace
@adivardi
remove leftover declaration
548d8af
@adivardi
fix missing assignment to class members
7684231
@adivardi
make Blackboard variable ID prive to the navigator
0403416 
to avoid clashes between navigators
@adivardi
Break long lines
60e3c91
@adivardi
only run CI on rolling
b14cb2c
@adivardi
keep BB ID params outside navigator namespace
1e7ab7c
@SteveMacenski
Copy link
Member

SteveMacenski commented Aug 7, 2025 •
edited
Loading

The two options would be to vcs-repo-file-url: "${{ github.workspace }}/.github/deps.repos" add Nav2 to this so it builds Nav2 ... which would be alot but possible (will take ~1.5 hours per PR). Ideally we add in building only up to a certain package we require.

Or, we change the base container to use Nav2 Rolling's Nightly, create a new workspace that sources from that, and build (much faster, but more github actions coding)

This was referenced Aug 18, 2025
Closed
Merged
@adivardi adivardi force-pushed the lyrical branch 2 times, most recently from 2388748 to 6db223b Compare August 21, 2025 13:16
@adivardi adivardi force-pushed the lyrical branch 2 times, most recently from 686f20c to d5756d6 Compare August 21, 2025 13:34
@SteveMacenski
Copy link
Member

SteveMacenski commented Aug 21, 2025 •
edited
Loading 

  C++ exception with description "[Any::convert]: no known safe conversion between [std::shared_ptr<rclcpp::Node>] and [std::shared_ptr<nav2::LifecycleNode>]" thrown in the test body.

This is just a migration error for Rolling t hat something needs to be changed properly to match

C++ exception with description "package 'nav2_bt_navigator' not found, searching: [/__w/opennav_coverage/opennav_coverage/ros_ws/install/opennav_coverage_navigator, /__w/opennav_coverage/opennav_coverage/ros_ws/install/opennav_coverage_bt, /__w/opennav_coverage/opennav_coverage/ros_ws/install/nav2_core, /__w/opennav_coverage/opennav_coverage/ros_ws/install/opennav_coverage, /__w/opennav_coverage/opennav_coverage/ros_ws/install/nav2_costmap_2d, /__w/opennav_coverage/opennav_coverage/ros_ws/install/opennav_coverage_msgs, /__w/opennav_coverage/opennav_coverage/ros_ws/install/nav2_map_server, /__w/opennav_coverage/opennav_coverage/ros_ws/install/nav2_lifecycle_manager, /__w/opennav_coverage/opennav_coverage/ros_ws/install/nav2_behavior_tree, /__w/opennav_coverage/opennav_coverage/ros_ws/install/nav2_util, /__w/opennav_coverage/opennav_coverage/ros_ws/install/nav2_ros_common, /__w/opennav_coverage/opennav_coverage/ros_ws/install/nav2_voxel_grid, /__w/opennav_coverage/opennav_coverage/ros_ws/install/nav2_msgs, /__w/opennav_coverage/opennav_coverage/ros_ws/install/nav2_common, /opt/ros/rolling]" thrown in the test body.

@adivardi
Copy link
Contributor Author

adivardi commented Aug 22, 2025

Missed migrating the node in the tests to nav2::LifecycleNode

@adivardi adivardi mentioned this pull request Aug 22, 2025
Merged
@adivardi
Copy link
Contributor Author

adivardi commented Aug 22, 2025

Now I don't get it. Locally the opennav_coverage_navigator test passes, though there are warnings/error prints

[2025-08-22 09:25:10.223] [test_node] error_code_name_prefixes parameters were not set. Using default values of: assisted_teleop backup compute_path dock_robot drive_on_heading follow_path nav_thru_poses nav_to_pose spin undock_robot wait
7: Make sure these match your BT and there are not other sources of error codes youreported to your application
7: 
[2025-08-22 09:25:10.224] [test_node_navigate_complete_coverage_rclcpp_node] 
7: 	test_node_navigate_complete_coverage_rclcpp_node lifecycle node launched. 
7: 	Waiting on external lifecycle transitions to activate
7: 	See https://design.ros2.org/articles/node_lifecycle.html for more information.
7: 
[2025-08-22 09:25:11.226] [test_node_navigate_complete_coverage_rclcpp_node] "compute_coverage_path" action server not available after waiting for 1.00s
7: 
[2025-08-22 09:25:11.226] [test_node] Setting internal error error_code:801, error_msg:Exception when loading BT: Action server compute_coverage_path not available
7: 
[2025-08-22 09:25:11.226] [test_node] Error loading XML file: /opt/enway/ros2_ws/install/opennav_coverage_bt/share/opennav_coverage_bt/behavior_trees/navigate_w_basic_complete_coverage.xml
7: 
[2025-08-22 09:25:11.228] [test_node] Action server is inactive. Rejecting the goal.
7: 
[2025-08-22 09:25:12.228] [test_node] Action server is inactive. Rejecting the goal.
7: 
[2025-08-22 09:25:13.229] [test_node_navigate_complete_coverage_rclcpp_node] Destroying

On the CI, the test seems to fail because the node doesn't shutdown

      [ RUN      ] CoverageNavigatorTests.TestBasicFunctionality
      [INFO] [1755851015.724542836] [test_node]: 
      	test_node lifecycle node launched. 
      	Waiting on external lifecycle transitions to activate
      	See https://design.ros2.org/articles/node_lifecycle.html for more information.
      [INFO] [1755851015.728374626] [test_node]: Destroying
      [WARN] [1755851015.728405254] [rclcpp_lifecycle]: LifecycleNode is not shut down: Node still in state(1) in destructor
      unknown file: Failure

I am not sure about the error
C++ exception with description "package 'nav2_bt_navigator' not found" thrown in the test body.
This package is not built, but it should not be needed

@SteveMacenski
Copy link
Member

SteveMacenski commented Aug 26, 2025

This package is not built, but it should not be needed

I think that's the issue, its trying to find it as a dependency and failing so it quits (I think). I think that exception being thrown is why you see the other error about lifecycle because it is no longer in a recoverable state (either crashed or in the fail state of the lifecycle FSM). Can you try building that instead? It might be that locally you've either built it or its in your path of install from /opt/ros/XYZ to be found

@adivardi
Copy link
Contributor Author

adivardi commented Aug 27, 2025

It looks like it is building (I see nav2_bt_navigator in the colcon build output).
Let's try to add it as a dependency

@SteveMacenski
Copy link
Member

SteveMacenski commented Aug 27, 2025 •
edited
Loading 

  C++ exception with description "Could not load library: libnav2_path_expiring_timer_condition.so: cannot open shared object file: No such file or directory" thrown in the test body.

Add Nav2 behavior tree -- I think you're just missing the library of BT nodes perhaps. This is a good step forward though

@adivardi
Copy link
Contributor Author

adivardi commented Sep 1, 2025

finally passed 🎉

SteveMacenski
SteveMacenski approved these changes Sep 3, 2025
View reviewed changes
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Thanks so much for your diligence, I know this was a pain! Merging 🥳

@SteveMacenski SteveMacenski merged commit 396fffa into open-navigation:main Sep 3, 2025
7 checks passed
@adivardi adivardi deleted the lyrical branch September 4, 2025 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveMacenski SteveMacenski SteveMacenski approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@adivardi @SteveMacenski