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

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

Merged
merged 4 commits into from
Aug 21, 2024

Conversation

saikishor
Copy link
Member

@saikishor saikishor commented Aug 20, 2024

The typo was introduced in #1661

const std::string param_name = controller_name + ".params_file";
std::string parameters_file;
// Check if parameter has been declared
if (!has_parameter(param_name))
{
declare_parameter(param_name, rclcpp::ParameterType::PARAMETER_STRING);
}
if (get_parameter(param_name, parameters_file) && !parameters_file.empty())
{
controller_spec.info.parameters_file = parameters_file;
}

Fixes: ros-controls/ros2_control_ci#118
Fixes: ros-controls/ros2_control_ci#119
Fixes: ros-controls/ros2_control_ci#120
Fixes: ros-controls/ros2_control_ci#121
Fixes: ros-controls/ros2_control_demos#569
Fixes: ros-controls/ros2_control_demos#570
Fixes: ros-controls/ros2_control_demos#571
Fixes: ros-controls/ros2_control_demos#572

@saikishor saikishor changed the title Fix params_file type in spawner and update release notes for use_global_arguments Fix params_file typo in spawner and update release notes for use_global_arguments Aug 20, 2024
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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 Coverage Δ
unittests 86.73% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...controller_interface/controller_interface_base.hpp 87.50% <100.00%> (ø)
.../controller_manager/controller_manager_services.py 70.00% <ø> (ø)
controller_manager/src/controller_manager.cpp 78.07% <100.00%> (+0.83%) ⬆️

@saikishor
Copy link
Member Author

saikishor commented Aug 20, 2024

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
Copy link
Member Author

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Tested with ros2_control_demos, thanks for the fix!

@saikishor
Copy link
Member Author

Tested with ros2_control_demos, thanks for the fix!

Thanks to you for taking time and testing it.

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

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

@saikishor
Copy link
Member Author

saikishor commented Aug 20, 2024

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
Copy link
Member Author

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
Copy link
Member

bmagyar commented Aug 21, 2024

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!

@bmagyar bmagyar merged commit fb1d254 into ros-controls:master Aug 21, 2024
21 checks passed
@bmagyar bmagyar deleted the fix/params_file/typo branch August 21, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants