ros-controls / ros2_control

Generic and simple controls framework for ROS 2
https://control.ros.org
Apache License 2.0
479 stars 293 forks source link

Fix params_file typo in spawner and update release notes for use_global_arguments #1701

Closed saikishor closed 1 month ago

saikishor commented 1 month ago

The typo was introduced in https://github.com/ros-controls/ros2_control/pull/1661

https://github.com/ros-controls/ros2_control/blob/4498d25ff7cfdc40780a34b36cf4d1fea40f7521/controller_manager/src/controller_manager.cpp#L484-L495

Fixes: https://github.com/ros-controls/ros2_control_ci/issues/118 Fixes: https://github.com/ros-controls/ros2_control_ci/issues/119 Fixes: https://github.com/ros-controls/ros2_control_ci/issues/120 Fixes: https://github.com/ros-controls/ros2_control_ci/issues/121 Fixes: https://github.com/ros-controls/ros2_control_demos/issues/569 Fixes: https://github.com/ros-controls/ros2_control_demos/issues/570 Fixes: https://github.com/ros-controls/ros2_control_demos/issues/571 Fixes: https://github.com/ros-controls/ros2_control_demos/issues/572

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.73%. Comparing base (4498d25) to head (b470a63). Report is 2 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1701 +/- ## ========================================== + Coverage 86.64% 86.73% +0.08% ========================================== Files 115 115 Lines 10527 10528 +1 Branches 970 971 +1 ========================================== + Hits 9121 9131 +10 + Misses 1056 1047 -9 Partials 350 350 ``` | [Flag](https://app.codecov.io/gh/ros-controls/ros2_control/pull/1701/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/ros-controls/ros2_control/pull/1701/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | `86.73% <100.00%> (+0.08%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/ros-controls/ros2_control/pull/1701?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls) | Coverage Δ | | |---|---|---| | [...controller\_interface/controller\_interface\_base.hpp](https://app.codecov.io/gh/ros-controls/ros2_control/pull/1701?src=pr&el=tree&filepath=controller_interface%2Finclude%2Fcontroller_interface%2Fcontroller_interface_base.hpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#diff-Y29udHJvbGxlcl9pbnRlcmZhY2UvaW5jbHVkZS9jb250cm9sbGVyX2ludGVyZmFjZS9jb250cm9sbGVyX2ludGVyZmFjZV9iYXNlLmhwcA==) | `87.50% <100.00%> (ø)` | | | [.../controller\_manager/controller\_manager\_services.py](https://app.codecov.io/gh/ros-controls/ros2_control/pull/1701?src=pr&el=tree&filepath=controller_manager%2Fcontroller_manager%2Fcontroller_manager_services.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#diff-Y29udHJvbGxlcl9tYW5hZ2VyL2NvbnRyb2xsZXJfbWFuYWdlci9jb250cm9sbGVyX21hbmFnZXJfc2VydmljZXMucHk=) | `70.00% <ø> (ø)` | | | [controller\_manager/src/controller\_manager.cpp](https://app.codecov.io/gh/ros-controls/ros2_control/pull/1701?src=pr&el=tree&filepath=controller_manager%2Fsrc%2Fcontroller_manager.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ros-controls#diff-Y29udHJvbGxlcl9tYW5hZ2VyL3NyYy9jb250cm9sbGVyX21hbmFnZXIuY3Bw) | `78.07% <100.00%> (+0.83%)` | :arrow_up: |
saikishor commented 1 month ago

We will still have issues with tests like this https://github.com/ros-controls/ros2_controllers/blob/master/ackermann_steering_controller/test/test_load_ackermann_steering_controller.cpp because this https://github.com/ros-controls/ros2_controllers/blob/master/ackermann_steering_controller/CMakeLists.txt#L61-L64 sets the params to the global context and with the new change introduced in #1661 we can't do it this way!

saikishor commented 1 month ago

We will still have issues with tests like this https://github.com/ros-controls/ros2_controllers/blob/master/ackermann_steering_controller/test/test_load_ackermann_steering_controller.cpp because this https://github.com/ros-controls/ros2_controllers/blob/master/ackermann_steering_controller/CMakeLists.txt#L61-L64 sets the params to the global context and with the new change introduced in #1661 we can't do it this way!

Addressed in https://github.com/ros-controls/ros2_controllers/pull/1256

saikishor commented 1 month ago

Tested with ros2_control_demos, thanks for the fix!

Thanks to you for taking time and testing it.

saikishor commented 1 month ago

do we want to reference global arguments rclcpp somewhere in the migration docs?

Well I've added some info in the migration docs regarding it. It just doesn't use the global context for the parameters and creates its own.

Let me know what you think

saikishor commented 1 month ago

do we want to reference global arguments rclcpp somewhere in the migration docs?

@bmagyar do you mean link of the docs like this: https://docs.ros.org/en/rolling/p/rclcpp/generated/classrclcpp_1_1NodeOptions.html#_CPPv4N6rclcpp11NodeOptions20use_global_argumentsEb

bmagyar commented 1 month ago

Yes I meant that one and if there are any longer writeups of when one would rely on them. Can go into a new PR though, let's merge this!