Use regex for checking namespace for ROS-specific types #263
Conversation
This enables support for ROS message types with namespaces other than 'msg' or 'srv' (e.g. 'action'). Signed-off-by: Jacob Perron <[email protected]>
jacobperron
changed the title
| std::string pkg = dds_type_string.substr(0, substring_position); | ||
| size_t start = substring_position + substring.size(); | ||
| std::string type_name = dds_type_string.substr(start, dds_type_string.length() - 1 - start); | ||
| return pkg + "/" + type_name; |
There was a problem hiding this comment.
The return value being <pkg>/<type_name> isn't going to work anymore - it already looses the namespace. So any code built on top won't be able to map this to the correct type.
There was a problem hiding this comment.
Are you worried about existing code or the new action graph API?
For the action graph API, I only wanted to be able to list types associated with the hidden action topics (which works fine with this change).
I think I see what you mean though, existing code would still work, but under the assumption that the type is still in the msg namespace, which isn't great.
I can see two solutions:
- We can add the namespace to the string:
<pkg>/<ns>/<type_name>
But I guess this will affect a number of downstream packages. - I can assume the returned string will have the namespace
::action::dds_at thercllayer and do this parsing there.
There was a problem hiding this comment.
but under the assumption that the type is still in the msg namespace
That is not the case. Messages are in the same namespace as the type they are derived from:
- the request and response messages of a service are in the
srvnamespace - the messages and services of an action are in the
actionnamespace.
We can add the namespace to the string:
<pkg>/<ns>/<type_name>
But I guess this will affect a number of downstream packages.
It would probably be good to return an actual tuple instead of a string with certain separators (since the logic for splitting will be all over the place). That conceptional change will certainly affect a lot of code / packages.
I can assume the returned string will have the namespace ::action::dds_ at the rcl layer and do this parsing there.
rcl should never see the dds_ part which is an implementation detail of the typesupport.
There was a problem hiding this comment.
In order to return a tuple for the type, we'd need to update the data structure rmw_names_and_types_t. How about the following structure?
typedef struct RMW_PUBLIC_TYPE rmw_names_and_types_t
{
rcutils_string_array_t names;
// The length of this array is the same as names.size
// Each element of an array list is a type represented by another array list of strings
// representing the fully qualified type name, e.g. ["pkg_name", "msg", "Empty"]
rcutils_array_list_t * types;
} rmw_names_and_types_t;If it sounds good to you, I will go ahead and start refactoring the affected packages.
There was a problem hiding this comment.
Sounds fine with me. I just think this needs a global consensus how we identify interfaces (across various different locations in the code as well as the CLI). Establishing that first (e.g. with a design article) will avoid discussions on how to do it later.
There was a problem hiding this comment.
Design PR: ros2/design#220
|
Replaced by #268 |
This enables support for ROS message types with namespaces other than 'msg' or 'srv' (e.g. 'action').
Related to ros2/ros2cli#202. This is needed for querying action types to support the Action CLI.