search

micro-ROS / rmw_microxrcedds

RMW implementation using Micro XRCE-DDS middleware.
Apache License 2.0
30 stars 23 forks source link

Should generate_service_topics(..) return an error if len(topic_name) > RMW_UXRCE_TOPIC_NAME_MAX_LENGTH? #253

Closed gavanderhoorn closed 2 years ago

gavanderhoorn commented 2 years ago

As per subject really.

I just ran into a situation where service 'topic' names were being truncated to 60 chars, which did not result in any warnings or errors, other than on the Agent side which complained with:

[WARN] [1652802737.011662749] [rmw_fastrtps_shared_cpp]: service topic has prefix but no suffix; report this: 'rr/my_namespace/follow_joint_trajectory/_action/send_goalRe'
[WARN] [1652802737.011679535] [rmw_fastrtps_shared_cpp]: service topic has prefix but no suffix; report this: 'rr/my_namespace/follow_joint_trajectory/_action/cancel_goal'
[WARN] [1652802737.011692818] [rmw_fastrtps_shared_cpp]: service topic has prefix but no suffix; report this: 'rr/my_namespace/follow_joint_trajectory/_action/get_resultR'

I notice generate_service_topics(..) does pass buffer_size to snprintf(..):

https://github.com/micro-ROS/rmw_microxrcedds/blob/b0f9724a2c638cf35b4b1c6003a58b6ddd1e7785/rmw_microxrcedds_c/src/utils.c#L123-L138

but it doesn't check the return value of snprintf(..), which could help detect the topic name was truncated (from here):

If the output was truncated due to this limit then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated.

For services specifically, buffer_size is set to RMW_UXRCE_TOPIC_NAME_MAX_LENGTH:

https://github.com/micro-ROS/rmw_microxrcedds/blob/a17896c16f86b925cd3ff9e5d6b7ccdda9824a09/rmw_microxrcedds_c/src/rmw_service.c#L127-L129

so it's known what the maximum is and it could be checked.

gavanderhoorn commented 2 years ago

For context: I ran into this with a node name of my_node, and a namespace set to my_namespace. An action server called follow_joint_trajectory will then have a fully-qualified name of /my_namespace/follow_joint_trajectory, which is already 37 chars long.

This resulted in the hidden services getting their names truncated.

This is not obvious, as those services are not (directly) created by application code, but are part of the hidden ROS API.

An error from the initialisation code could help debugging significantly.

gavanderhoorn commented 2 years ago

Does generate_topic_name(..) already do this:

https://github.com/micro-ROS/rmw_microxrcedds/blob/b0f9724a2c638cf35b4b1c6003a58b6ddd1e7785/rmw_microxrcedds_c/src/utils.c#L319-L334

?

Edit: should that be || instead of &&?

pablogs9 commented 2 years ago

Hi @gavanderhoorn, good catch.

Let us know if https://github.com/micro-ROS/rmw_microxrcedds/pull/254 solves this. Feel free to approve the PR if you are ok with it.

gavanderhoorn commented 2 years ago

I've not had a chance to test the (already merged) fix in #254.

I believe you've addressed my immediate issue though, so you could perhaps close this issue.

gavanderhoorn commented 2 years ago

Thanks again.

pablogs9 commented 2 years ago

Ok, I'm closing. Thanks for the report @gavanderhoorn !