Skip to content
This repository has been archived by the owner on Oct 7, 2021. It is now read-only.

Use regex for checking namespace for ROS-specific types #263

Closed
wants to merge 1 commit into from
Closed

Use regex for checking namespace for ROS-specific types #263

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 25 additions & 14 deletions rmw_opensplice_cpp/src/demangle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#include <string>
#include <algorithm>
#include <cassert>
#include <regex>
#include <string>
#include <vector>

#include "rcutils/logging_macros.h"
Expand All @@ -36,19 +38,24 @@ _demangle_if_ros_topic(const std::string & topic_name)
std::string
_demangle_if_ros_type(const std::string & dds_type_string)
{
std::string substring = "::msg::dds_::";
size_t substring_position = dds_type_string.find(substring);
std::regex dds_namespace_pattern(".*(::.*::dds_::).*");
std::smatch match;
if (
dds_type_string[dds_type_string.size() - 1] == '_' &&
substring_position != std::string::npos)
dds_type_string[dds_type_string.size() - 1] != '_' ||
!std::regex_match(dds_type_string, match, dds_namespace_pattern))
{
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;
// not a ROS type
return dds_type_string;
}
// not a ROS type
return dds_type_string;

// The first submatch is the whole string and the second is the parenthesized expression
assert(2u == match.size());
std::string substring = match[1].str();
size_t substring_position = dds_type_string.find(substring);
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;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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:

  1. We can add the namespace to the string: <pkg>/<ns>/<type_name>
    But I guess this will affect a number of downstream packages.
  2. I can assume the returned string will have the namespace ::action::dds_ at the rcl layer and do this parsing there.

Copy link
Member

Choose a reason for hiding this comment

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

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 srv namespace
  • the messages and services of an action are in the action namespace.

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.

Copy link
Member Author

@jacobperron jacobperron Mar 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Design PR: ros2/design#220

}

std::string
Expand Down Expand Up @@ -108,12 +115,16 @@ _demangle_service_from_topic(const std::string & topic_name)
std::string
_demangle_service_type_only(const std::string & dds_type_name)
{
std::string ns_substring = "::srv::dds_::";
size_t ns_substring_position = dds_type_name.find(ns_substring);
if (ns_substring_position == std::string::npos) {
std::regex dds_namespace_pattern(".*(::.*::dds_::).*");
std::smatch match;
if (!std::regex_match(dds_type_name, match, dds_namespace_pattern)) {
// not a ROS service type
return "";
}
// The first submatch is the whole string and the second is the parenthesized expression
assert(2u == match.size());
std::string ns_substring = match[1].str();
size_t ns_substring_position = dds_type_name.find(ns_substring);
auto suffixes = {
std::string("_Response_"),
std::string("_Request_"),
Expand Down