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

add content-filtered-topic interfaces #1561

Merged
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
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
5 changes: 5 additions & 0 deletions rclcpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ cmake_minimum_required(VERSION 3.12)

project(rclcpp)

option(DEFINE_CONTENT_FILTER "Content filter feature support" ON)
if(DEFINE_CONTENT_FILTER)
add_definitions(-DCONTENT_FILTER_ENABLE)
endif()

Comment on lines +5 to +9
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this will be always on as ROS 2 rclcpp library, but user explicitly enable this when they want to use ContentFitleredTopic, otherwise build fails. my understanding correct? @wjwwood could we have feedback on this, if this is what we want?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wjwwood friendly ping.

Copy link
Member

Choose a reason for hiding this comment

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

I think this doesn't really help much, particularly if we don't have equivalent flags in rcl/rmw.

Copy link
Member

Choose a reason for hiding this comment

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

As @ivanpauno mentioned, this isn't really accomplishing what we wanted, so let's just remove it and provide access to CFT uniformly for now. We'll just have to document the known issues.

find_package(Threads REQUIRED)

find_package(ament_cmake_ros REQUIRED)
Expand Down
41 changes: 41 additions & 0 deletions rclcpp/include/rclcpp/subscription_base.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,47 @@ class SubscriptionBase : public std::enable_shared_from_this<SubscriptionBase>
event_handlers_[event_type]->clear_on_ready_callback();
}

#ifdef CONTENT_FILTER_ENABLE
/// Check if content filtered topic feature of the subscription instance is enabled.
/**
* \return boolean flag indicating if the content filtered topic of this subscription is enabled.
*/
RCLCPP_PUBLIC
bool
is_cft_enabled() const;

/// Set the filter expression and expression parameters for the subscription.
/**
* \param[in] filter_expression An filter expression to set.
* \sa SubscriptionOptionsBase::ContentFilterOptions::filter_expression
* Use empty string ("") can reset (or clear) the content filter setting of a subscription.
* \param[in] expression_parameters Array of expression parameters to set.
Copy link
Collaborator

@Barry-Xu-2018 Barry-Xu-2018 Mar 26, 2022

Choose a reason for hiding this comment

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

The size of the vector have a limitation which is described in
https://github.com/ros2/rmw/blob/d778df84af42cdbc5458361a2fe220317494634c/rmw/include/rmw/subscription_content_filter_options.h#L42-L45

   * It can be NULL if there is no "%n" tokens placeholder in filter_expression.
   * The maximum index number must be smaller than 100.
   */
  rcutils_string_array_t expression_parameters;

Do we need to describe this limitation here ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we do, since this would be the 1st place for user application.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

used the '\sa ContentFilterOptions::expression_parameters' for set_content_filter
, so I just updated the information here.

* \sa SubscriptionOptionsBase::ContentFilterOptions::expression_parameters
* \throws RCLBadAlloc if memory cannot be allocated
* \throws RCLError if an unexpect error occurs
*/
RCLCPP_PUBLIC
virtual
void
set_content_filter(
const std::string & filter_expression,
const std::vector<std::string> & expression_parameters = {});

/// Get the filter expression and expression parameters for the subscription.
/**
* \param[out] filter_expression An filter expression to get.
* \param[out] expression_parameters Array of expression parameters to get.
* \throws RCLBadAlloc if memory cannot be allocated
* \throws RCLError if an unexpect error occurs
*/
RCLCPP_PUBLIC
virtual
void
get_content_filter(
std::string & filter_expression,
std::vector<std::string> & expression_parameters) const;
#endif // CONTENT_FILTER_ENABLE

protected:
template<typename EventCallbackT>
void
Expand Down
32 changes: 32 additions & 0 deletions rclcpp/include/rclcpp/subscription_options.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,22 @@ struct SubscriptionOptionsBase
TopicStatisticsOptions topic_stats_options;

QosOverridingOptions qos_overriding_options;

#ifdef CONTENT_FILTER_ENABLE
/// Options to configure content filtered topic in the subscription.
struct ContentFilterOptions
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's better to define this outside rclcpp::SubscriptionOptions, as it's easier to write rclcpp::ContentFilterOptions than rclcpp::SubscriptionOptions::ContentFilterOptions.
I don't mind strongly though.

{
/// Filter expression is similar to the WHERE part of an SQL clause
std::string filter_expression;
/**
* Expression parameters is the tokens placeholder ‘parameters’ (i.e., "%n" tokens begin from 0)
* in the filter_expression
*/
std::vector<std::string> expression_parameters;
};

ContentFilterOptions content_filter_options;
#endif // CONTENT_FILTER_ENABLE
};

/// Structure containing optional configuration for Subscriptions.
Expand Down Expand Up @@ -123,6 +139,22 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase
rmw_implementation_payload->modify_rmw_subscription_options(result.rmw_subscription_options);
}

#ifdef CONTENT_FILTER_ENABLE
// Copy content_filter_options into rcl_subscription_options.
if (!content_filter_options.filter_expression.empty()) {
std::vector<const char *> cstrings =
get_c_vector_string(content_filter_options.expression_parameters);
rcl_ret_t ret = rcl_subscription_options_set_content_filter_options(
get_c_string(content_filter_options.filter_expression),
cstrings.size(),
cstrings.data(),
&result);
if (RCL_RET_OK != ret) {
rclcpp::exceptions::throw_from_rcl_error(
ret, "failed to set content_filter_options");
}
}
#endif // CONTENT_FILTER_ENABLE
return result;
}

Expand Down
9 changes: 9 additions & 0 deletions rclcpp/include/rclcpp/utilities.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,15 @@ RCLCPP_PUBLIC
const char *
get_c_string(const std::string & string_in);

/// Return the std::vector of C string from the given std::vector<std::string>.
/**
* \param[in] string_in is a std::string
* \return the std::vector of C string from the std::vector<std::string>
*/
RCLCPP_PUBLIC
std::vector<const char *>
get_c_vector_string(const std::vector<std::string> & strings);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use string_in in above description


} // namespace rclcpp

#endif // RCLCPP__UTILITIES_HPP_
88 changes: 88 additions & 0 deletions rclcpp/src/rclcpp/subscription_base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#include <unordered_map>
#include <vector>

#include "rcpputils/scope_exit.hpp"

#include "rclcpp/exceptions.hpp"
#include "rclcpp/expand_topic_or_service_name.hpp"
#include "rclcpp/experimental/intra_process_manager.hpp"
Expand Down Expand Up @@ -354,3 +356,89 @@ SubscriptionBase::set_on_new_message_callback(
throw_from_rcl_error(ret, "failed to set the on new message callback for subscription");
}
}

#ifdef CONTENT_FILTER_ENABLE
bool
SubscriptionBase::is_cft_enabled() const
{
return rcl_subscription_is_cft_enabled(subscription_handle_.get());
}

void
SubscriptionBase::set_content_filter(
const std::string & filter_expression,
const std::vector<std::string> & expression_parameters)
{
rcl_subscription_content_filter_options_t options =
rcl_get_zero_initialized_subscription_content_filter_options();

std::vector<const char *> cstrings =
get_c_vector_string(expression_parameters);
rcl_ret_t ret = rcl_subscription_content_filter_options_init(
get_c_string(filter_expression),
cstrings.size(),
cstrings.data(),
&options);
if (RCL_RET_OK != ret) {
rclcpp::exceptions::throw_from_rcl_error(
ret, "failed to init subscription content_filtered_topic option");
}
RCPPUTILS_SCOPE_EXIT(
{
rcl_ret_t ret = rcl_subscription_content_filter_options_fini(&options);
if (RCL_RET_OK != ret) {
RCLCPP_ERROR(
rclcpp::get_logger("rclcpp"),
"Failed to fini subscription content_filtered_topic option: %s",
rcl_get_error_string().str);
rcl_reset_error();
}
});

ret = rcl_subscription_set_content_filter(
subscription_handle_.get(),
&options);

if (RCL_RET_OK != ret) {
rclcpp::exceptions::throw_from_rcl_error(ret, "failed to set cft expression parameters");
}
}

void
SubscriptionBase::get_content_filter(
std::string & filter_expression,
std::vector<std::string> & expression_parameters) const
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using arguments to get the output, I would rather use ContentFilterOptions as a return value.

Suggested change
void
SubscriptionBase::get_content_filter(
std::string & filter_expression,
std::vector<std::string> & expression_parameters) const
ContentFilterOptions
SubscriptionBase::get_content_filter() const

{
rcl_subscription_content_filter_options_t options =
rcl_get_zero_initialized_subscription_content_filter_options();

rcl_ret_t ret = rcl_subscription_get_content_filter(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could cache in rclcpp the last set ContentFilterOptions instead of converting each time (?).
I don't mind strongly though.

subscription_handle_.get(),
&options);

if (RCL_RET_OK != ret) {
rclcpp::exceptions::throw_from_rcl_error(ret, "failed to get cft expression parameters");
}

RCPPUTILS_SCOPE_EXIT(
{
rcl_ret_t ret = rcl_subscription_content_filter_options_fini(&options);
if (RCL_RET_OK != ret) {
RCLCPP_ERROR(
rclcpp::get_logger("rclcpp"),
"Failed to fini subscription content_filtered_topic option: %s",
rcl_get_error_string().str);
rcl_reset_error();
}
});

rmw_subscription_content_filter_options_t & content_filter_options =
options.rmw_subscription_content_filter_options;
filter_expression = content_filter_options.filter_expression;

for (size_t i = 0; i < content_filter_options.expression_parameters.size; ++i) {
expression_parameters.push_back(
content_filter_options.expression_parameters.data[i]);
}
}
#endif // CONTENT_FILTER_ENABLE
13 changes: 13 additions & 0 deletions rclcpp/src/rclcpp/utilities.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -214,4 +214,17 @@ get_c_string(const std::string & string_in)
return string_in.c_str();
}

std::vector<const char *>
get_c_vector_string(const std::vector<std::string> & strings)
{
std::vector<const char *> cstrings;
cstrings.reserve(strings.size());

for (size_t i = 0; i < strings.size(); ++i) {
cstrings.push_back(strings[i].c_str());
}

return cstrings;
}

} // namespace rclcpp
19 changes: 19 additions & 0 deletions rclcpp/test/rclcpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -733,3 +733,22 @@ ament_add_gtest(test_graph_listener test_graph_listener.cpp)
if(TARGET test_graph_listener)
target_link_libraries(test_graph_listener ${PROJECT_NAME} mimick)
endif()

if(DEFINE_CONTENT_FILTER)
function(test_subscription_content_filter_for_rmw_implementation)
set(rmw_implementation_env_var RMW_IMPLEMENTATION=${rmw_implementation})
ament_add_gmock(test_subscription_content_filter${target_suffix}
test_subscription_content_filter.cpp
ENV ${rmw_implementation_env_var}
)
if(TARGET test_subscription_content_filter${target_suffix})
target_link_libraries(test_subscription_content_filter${target_suffix} ${PROJECT_NAME} mimick)
ament_target_dependencies(test_subscription_content_filter${target_suffix}
"rcpputils"
"rosidl_typesupport_cpp"
"test_msgs"
)
endif()
endfunction()
call_for_each_rmw_implementation(test_subscription_content_filter_for_rmw_implementation)
endif()
Loading