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

Set declare_params functionality to parameter declaration only #156

Closed

Conversation

saikishor
Copy link

Hello!

We have observed some issues recently in the ros2_controller that have issues with the parameters when loading the controller.

We believe we found the reason all behind that IMUSensorBroadcaster PR. I was also able to reproduce things with ros2_control_demos examples

Apparantly, if you parse the robot_controller parameters to the ros2_control_nodehttps://github.com/ros-controls/ros2_control_demos/blob/master/example_1/bringup/launch/rrbot.launch.py#L67-L72 everything will work as usual. If this is not the case, then it becomes tricky, if you either use the spawner command line or the launch_utils.py in the controller_manager, then we have this issue with the validators etc using the generate_parameter_library.

However, moving the object creation and the retrieval of the parameters of GPL to the configure helps only in the Humble especially because of the allow_undeclared_parameter being true , this is why we are able to set the parameters without issue after loading the controller and before the configuring the controller (that's the reason moving them to the configure it works for both having parameters at ros2_control_node level or by loading them after initialization).

Unfortunately, this is not the case anymore for rolling as this allow_undeclared_parameters is removed completely. So, if we try to set the parameters after loading the controller, they would fail as they are not declared in the first place. So, merging PR : ros-controls/ros2_controllers#795 would solve the issue for Humble but not for Rolling (I tried to reproduce the same by removing this allow_undeclared_parameters option in Humble and tried to load the parameters and then I've the following error). I was not able to test it in the rolling though. I believe it should be the same

[spawner-6] Set parameter sensor_name failed: parameter 'sensor_name' cannot be set because it was not declared
[spawner-6] Set parameter frame_id failed: parameter 'frame_id' cannot be set because it was not declared
[spawner-6] Set parameter publish_rate failed: parameter 'publish_rate' cannot be set because it was not declared
[spawner-6] [INFO] [1698741261.427305178] [spawner_imu_sensor_broadcaster]: Loaded parameters file "/home/user/pmb3_robot/install/pmb3_controller_configuration/share/pmb3_controller_configuration/config/imu_sensor_broadcaster.yaml" for imu_sensor_broadcaster
[spawner-6] [INFO] [1698741261.427854462] [spawner_imu_sensor_broadcaster]: Loaded /home/user/pmb3_robot/install/pmb3_controller_configuration/share/pmb3_controller_configuration/config/imu_sensor_broadcaster.yaml into imu_sensor_broadcaster

and when you are configuring the controller you have the following error

[gzserver-1] [INFO] [1698741280.396357231] [controller_manager]: Configuring controller 'imu_sensor_broadcaster'
[gzserver-1] [ERROR] [1698741280.398287946] [imu_sensor_broadcaster]: Exception thrown during init stage with message: Invalid value set during initialization for parameter 'sensor_name': Parameter 'sensor_name' cannot be empty 
[gzserver-1] 
[gzserver-1] [WARN] [1698741280.398347886] [imu_sensor_broadcaster]: Error occurred while doing error handling.
[gzserver-1] [ERROR] [1698741280.398381867] [controller_manager]: After configuring, controller 'imu_sensor_broadcaster' is in state 'unconfigured' , expected inactive.

So, I think the only way this can be solved is by having the separation of declaration of the parameters and fetching them later.

Ideally: If the changes look good, all ROS controllers might need to have something like this : saikishor/ros2_controllers#1

@tylerjw
Copy link
Contributor

tylerjw commented Oct 31, 2023

We talked about this in the ros2_control slack. The plan is to change where gpl calls get_parameter on ROS.

The goal is to separate calling declare_parameter from get_parameter for the use case of lifecycle nodes. To do that, we will move the get_parameter calls into the get_param function, and that function will return a new copy of the params struct instead of returning one that is kept as an internal state of the listener. The update function will no longer update an internal copy of the params struct and will only update the timestamp. The internal copy of the params struct is no longer needed.

@saikishor saikishor marked this pull request as draft November 2, 2023 15:51
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