Fix service definition parsing on ROS rolling #293
Conversation
Request and Response message files are not available anymore as of ros2/rosidl#753. This patch parses the service's .srv file instead.
| constexpr char SEP[] = "---\n"; | ||
|
|
||
| const auto definitions = split_string(service_definition, SEP); | ||
| if (definitions.size() != 2) { |
There was a problem hiding this comment.
Is it legal for the response to be totally empty (no newline) e.g. something like "int a\n---" ?
There was a problem hiding this comment.
Yes, I believe that's legal, also for the request. I will test if that's an issue
There was a problem hiding this comment.
if so, maybe using "---\n" as the separator won't work
There was a problem hiding this comment.
"---\n" is correct:
A service file contains two message definitions which are separated by a line which only contains three dashes:
There was a problem hiding this comment.
However, colcon doesn't raise an error if the service definition ends with --- without final LF. I will have to cover that case as well 😭
| definition_id, MessageSpec(definition_id.format, definition, package)); | ||
| } | ||
| if (subfolder == "action") { | ||
| const auto [goalDef, resultDef, feedbackDef] = split_action_msg_definition(contents); |
There was a problem hiding this comment.
Interesting – didn't realize we were already doing a manual split for .action but not .srv. Why were they different?
There was a problem hiding this comment.
For services it was much easier to use the _Request and _Response msg definitions which basically didn't need any additional code. For action types, these auto-generated msg definitions didn't exist.
achim-k
force-pushed
the
achim/fg-7402-fix-service-definition-parsing-on-rolling
branch
from
d1c4734
to
51ef884
Compare
| // The service type name includes the subtype which we have to remove to get the service name. | ||
| // Type name: SetBool_Request -> Service name: SetBool |
There was a problem hiding this comment.
I thought these don't exist anymore? Does the code have to handle both with & without the suffix because they exist in <=iron but not in >iron?
There was a problem hiding this comment.
We still use _Request and _Response as type name suffixes when advertising the services as we can't (or rather shouldn't) advertise request and response types with the same type name.
jtbandes
deleted the
achim/fg-7402-fix-service-definition-parsing-on-rolling
branch
Changelog
Fix service definition parsing on ROS rolling
Description
Prior to this change, we used the auto-generated
_Request.msgand_Response.msgmessage definitions for parsing a service's request and response definition. However, these message definitions have been removed in ros2/rosidl#753 causing foxglove bridge to raise warnings that the service definition could not be found. This PR changes the service definition parsing to lookup the service definition using the.srvfile and splitting the content using the===seperator to obtain the request and response definition.