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

Self test publishes the service under the node name, again #269

Merged
merged 13 commits into from
Jun 7, 2023

Conversation

ct2034
Copy link
Collaborator

@ct2034 ct2034 commented Dec 23, 2022

I am not sure if this is supposed to work. But the original ROS1 wiki indicated that there will be service called self_test per node. I can not see how the code will call them. If I run the code as it was on rolling, there will only be one self_test service that will be called. Advertising this same service from multiple nodes obviously doesn't work.

Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>
Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>
Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>
Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>
@ct2034
Copy link
Collaborator Author

ct2034 commented Jan 19, 2023

To document it here: I found out that the current ros2 implementation differs from the one in ros1. In particular, I compared noetic-devel @ 12ba2ee to rolling @ 12ab006.

ROS1 did create a service under [NODE_NAME]/self_test and you could perform the self_test with run_selftest [NODE_NAME].
On ROS2, the advertised service is not under the node's namespace, but simply called self_test and also the run_selftest command does not take an argument. This works as long as there is only one node that advertises this service, but not with multiple nodes that implement a self test.

Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>
Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>
Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>
Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>
@ct2034 ct2034 marked this pull request as ready for review January 25, 2023 19:37
@ct2034 ct2034 changed the title How is self_test supposed to work with multiple nodes that have a self_test Self test publishes the service under the node name, again Jan 25, 2023
@ct2034
Copy link
Collaborator Author

ct2034 commented Jan 25, 2023

Hi @trainman419 @bricerebsamen @Karsten1987
Could you please comment on this, why the API did change like that for ROS2 in the first place?
This change tries to recreate the API as it was in ROS1, because I think it makes more sense like this.

Signed-off-by: Christian Henkel <christian.henkel2@de.bosch.com>
@ct2034 ct2034 changed the base branch from stale/rolling to ros2 February 13, 2023 15:31
@ct2034
Copy link
Collaborator Author

ct2034 commented Feb 13, 2023

Friendly ping @trainman419 @bricerebsamen @Karsten1987

@ct2034 ct2034 added the ros2 PR tackling a ROS2 branch label Feb 13, 2023
base_interface_, service_interface_, "self_test", serviceCB, rmw_qos_profile_default,
nullptr);
base_interface_, service_interface_,
std::string(base_interface_->get_name()) + std::string("/self_test"),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

most important change here ...

@ct2034 ct2034 merged commit c8941bb into ros:ros2 Jun 7, 2023
@ct2034 ct2034 deleted the fix/making_sese_of_self_test branch June 7, 2023 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ros2 PR tackling a ROS2 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant