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

Update matching for lifecycle service types to support Opensplice #106

Closed
wants to merge 1 commit into from

Conversation

dhood
Copy link
Member

@dhood dhood commented Jun 22, 2018

@ironmig noticed that ros2 service list wasn't reporting Opensplice lifecycle nodes properly (e.g. ros2 run lifecycle lifecycle_talker).

The service name and type for opensplice that's reported by get_service_names_and_types is:

Topic: /lc_talker/get_state
Type: 'lifecycle_msgs/Sample_GetState'

I took a quick look at the opensplice typesupport, and it seems to be on purpose that we use samples vs msgs names so I assume that that is a requirement of opensplice's typesupport. As such this PR loosens the check to match it, rather than attempting to remove Sample_ from the type. It may very well be feasible to make get_service_names_and_types report the type without having Sample_ in it, but if so we can do that at a later date IMO.

@dhood dhood added the in review Waiting for review (Kanban column) label Jun 22, 2018
@dhood dhood self-assigned this Jun 22, 2018
@dhood dhood requested a review from dirk-thomas June 22, 2018 02:19
@Karsten1987
Copy link
Contributor

I can confirm that this resolves the problem.

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Jun 22, 2018

Tested this and now instead of returning "node not found" it crashes: ros2 lifecycle get lc_talker.

Failed to wait on wait set: failed to detach guard condition, at /home/mikael/work/ros2/bouncy_ws/src/ros2/rmw_opensplice/rmw_opensplice_cpp/src/rmw_wait.cpp:375, at /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl/src/rcl/wait.c:647

@dhood Is this something you experienced when testing this change?

I think it's worth fixing the reported type to not include the Sample as this is messing with other tools as well. For example if I want to call the service directly with ros2 service call /lc_talker/get_state lifecycle_msgs/Sample_GetState (the types provided by the autocopletion) it will crash because the our message structures have no idea of what Sample_GetState is.

So I recommend fixing it in get_service_names_and_types directly rather than working around it with this patch.

I'll try to submit a patch later today

@dirk-thomas
Copy link
Member

I am not sure if this is the right way to address the problem. Imo the underlying API needs to provide the same information across all rmw impl. otherwise all user level code has to handle the difference. Therefore I think this should be patched in rmw_opensplice_cpp instead.

@dhood
Copy link
Member Author

dhood commented Jun 22, 2018

Yeah, realising that what this API returns is representing ROS msg types as opposed to actual DDS type, it's reasonable to change that conversion layer without modifying the type sent on the wire. Sounds like @mikaelarguedas is on it

@mikaelarguedas
Copy link
Member

Opened ros2/rmw_opensplice#234 with the fix I had in mind

@tfoote
Copy link
Contributor

tfoote commented Jun 23, 2018

superseded by ros2/rmw_opensplice#234

@tfoote tfoote closed this Jun 23, 2018
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Jun 23, 2018
@tfoote tfoote deleted the match_opensplice_lifecycle branch June 23, 2018 15:59
esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
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.

5 participants