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 controllers parameters of controller manager #1547

Closed
wants to merge 1 commit into from

Conversation

delihus
Copy link

@delihus delihus commented May 23, 2024

The issue is described here #1506.

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.

Thanks for opening a PR. Do you think you could add a test into test_spawner_unspawner.cpp for your usecase?

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Hello @delihus!

Thanks for the pull request and mainly for reporting the issue. Looking at your reported issue, I personally think that the issue is not in the spawner, but it is inside the controller_manager code. It is not handling the namespaces very nicely. You can fix it over there or I can fix it in the upcoming days.
Let us know.

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

@delihus I've reviewed again probably your approach is fine, one more thing why do you have to run your spawner with the same namespace again?

python3 spawner.py  joint_trajectory_controller -c controller_manager --controller-manager-timeout 10 --namespace panther --param-file /tmp/jtc.yaml --ros-args -r __ns:=/panther -p use_sim_time:=True

I think in the spawner you would also need to change this part controller_manager_name to also include the namespace, so this way you might avoid parsing the namespace again via __ns right?

response = call_set_parameters(
node=node, node_name=controller_manager_name, parameters=[parameter]
)

ideally, it should look something like this right?

python3 spawner.py  joint_trajectory_controller -c controller_manager --controller-manager-timeout 10 --namespace panther --param-file /tmp/jtc.yaml --ros-args -p use_sim_time:=True

@delihus
Copy link
Author

delihus commented May 24, 2024

@delihus I've reviewed again probably your approach is fine, one more thing why do you have to run your spawner with the same namespace again?

python3 spawner.py  joint_trajectory_controller -c controller_manager --controller-manager-timeout 10 --namespace panther --param-file /tmp/jtc.yaml --ros-args -r __ns:=/panther -p use_sim_time:=True

Gives:

[ruby $(which ign) gazebo-1] [ERROR] [1716541010.983187571] [panther.controller_manager]: The 'type' param was not defined for 'joint_trajectory_controller'.

I think in the spawner you would also need to change this part controller_manager_name to also include the namespace, so this way you might avoid parsing the namespace again via __ns right?

response = call_set_parameters(
node=node, node_name=controller_manager_name, parameters=[parameter]
)

ideally, it should look something like this right?

python3 spawner.py  joint_trajectory_controller -c controller_manager --controller-manager-timeout 10 --namespace panther --param-file /tmp/jtc.yaml --ros-args -p use_sim_time:=True

This gives:

ros2 run controller_manager  spawner  joint_trajectory_controller -c controller_manager --controller-manager-timeout 10 --namespace panther --param-file /tmp/tmpt7hc4zwy --ros-args -p use_sim_time:=True
[INFO] [1716540758.743235940] [spawner_joint_trajectory_controller]: Waiting for '/controller_manager' node to exist

@anath93
Copy link

anath93 commented May 31, 2024

@christophfroehlich @delihus For spawner to load namespaced controller correctly, try defining in your .yaml file like below to expose this while launching your controller manager

image

then this,

image

image

@saikishor
Copy link
Member

This gives:

ros2 run controller_manager  spawner  joint_trajectory_controller -c controller_manager --controller-manager-timeout 10 --namespace panther --param-file /tmp/tmpt7hc4zwy --ros-args -p use_sim_time:=True
[INFO] [1716540758.743235940] [spawner_joint_trajectory_controller]: Waiting for '/controller_manager' node to exist

@delihus Could you let me know if you made the changes to this part of the code to include the namespace?

response = call_set_parameters(
node=node, node_name=controller_manager_name, parameters=[parameter]
)

Because if you do it, then it should wait for the namespace node right?

@anath93
Copy link

anath93 commented Jun 3, 2024

This gives:

ros2 run controller_manager  spawner  joint_trajectory_controller -c controller_manager --controller-manager-timeout 10 --namespace panther --param-file /tmp/tmpt7hc4zwy --ros-args -p use_sim_time:=True
[INFO] [1716540758.743235940] [spawner_joint_trajectory_controller]: Waiting for '/controller_manager' node to exist

@delihus Could you let me know if you made the changes to this part of the code to include the namespace?

response = call_set_parameters(
node=node, node_name=controller_manager_name, parameters=[parameter]
)

Because if you do it, then it should wait for the namespace node right?

@saikishor Did you namespace the controller manger in node side like this ?

image

@saikishor
Copy link
Member

@saikishor Did you namespace the controller manger in node side like this ?

image

@delihus I know that you namespace it like that, but what I mean is that the in the below code if you in the node_name, instead of parsing the controller_manager_name pass already the namespaced one like controller_namespace + "/" + controller_manager_name and see if it works instead of having to namespace the spawner again with __ns. What do you say?

response = call_set_parameters(
node=node, node_name=controller_manager_name, parameters=[parameter]
)

@delihus
Copy link
Author

delihus commented Jun 12, 2024

@saikishor Did you namespace the controller manger in node side like this ?
image

@delihus I know that you namespace it like that, but what I mean is that the in the below code if you in the node_name, instead of parsing the controller_manager_name pass already the namespaced one like controller_namespace + "/" + controller_manager_name and see if it works instead of having to namespace the spawner again with __ns. What do you say?

response = call_set_parameters(
node=node, node_name=controller_manager_name, parameters=[parameter]
)

Yeah, you're right. This works.

    initial_joint_controller_spawner_started = Node(
        package="controller_manager",
        executable="spawner",
        arguments=[

            [device_namespace, "_joint_trajectory_controller"],
            "-t",
            "joint_trajectory_controller/JointTrajectoryController",
            "-c",
            [robot_namespace, "/controller_manager"],
            "--controller-manager-timeout",
            "10",
            "--param-file",
            namespaced_initial_joint_controllers_path,
        ],
    )

ros2 topic list:

/panther/ur3e_joint_trajectory_controller/controller_state
/panther/ur3e_joint_trajectory_controller/joint_trajectory
/panther/ur3e_joint_trajectory_controller/state
/panther/ur3e_joint_trajectory_controller/transition_event

What do you think to add the name of the controller_manager in logs?

From:

[spawner-6] [INFO] [1718182938.346910274] [spawner_ur3e_joint_trajectory_controller]: Configured and activated ur3e_joint_trajectory_controller

to:

[spawner-6] [INFO] [1718182938.346910274] [spawner_ur3e_joint_trajectory_controller]: Configured and activated ur3e_joint_trajectory_controller for /panther/controller_manager

Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

@delihus Could you add a test for this use-case?

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.

4 participants