to support a feature of content filtered topic #1

iuhilnehc-ynos · 2020-12-02T02:35:38Z

temporary PR in my repository

Signed-off-by: Chen Lihui <[email protected]>

rcl/include/rcl/subscription.h

Signed-off-by: Chen Lihui <[email protected]>

fujitatomoya · 2021-02-08T05:58:32Z

rcl/include/rcl/subscription.h

+ *
+ * \param[in] option The structure which its resources have to be deallocated.
+ * \return `RCL_RET_OK` if the memory was successfully freed, or
+ * \return `RCL_RET_INVALID_ARGUMENT` if log_levels is NULL, or


Suggested change

* \return `RCL_RET_INVALID_ARGUMENT` if log_levels is NULL, or

* \return `RCL_RET_INVALID_ARGUMENT` if option is NULL, or

fujitatomoya · 2021-02-08T06:07:43Z

rcl/include/rcl/subscription.h

+ * Lock-Free          | Maybe [1]
+ *
+ * \param[in] subscription the subscription object to inspect.
+ * \param[in] filter_expression An filter expression to set.


Suggested change

* \param[in] filter_expression An filter expression to set.

* \param[in] filter_expression A filter expression to set.

fujitatomoya · 2021-02-08T06:10:26Z

rcl/include/rcl/subscription.h

+ *   it can be NULL if there is no placeholder in filter_expression.
+ * \return `RCL_RET_OK` if the query was successful, or
+ * \return `RCL_RET_INVALID_ARGUMENT` if `subscription` is NULL, or
+ * \return `RCL_RET_INVALID_ARGUMENT` if `filter_expression` is NULL, or


How we can reset the filtering on subscription? i think that there should be a way to do that, since the application is likely to do the following.

create subscription.

receive messages via subscription.

set content filtering.

receive messages via cft subscription.

reset content filtering.

receive messages via subsciption.

I was thinking as following,

If a subscription can be filtered, users should set this subscription with filter_expression(""), not NULL.

receive all messages

update filter_expression with custom value by rcl_subscription_set_cft_expression_parameters, such as "data MATCH 'Hello World: 5'"

receive messages via cft subscription

set filter_expression("") to reset.

receive all messages

okay, i see and it works

okay, i see and it works

I am sorry. After checking and trying to reproduce the above case, I found it's not supported.

Currently,
User must set content filter topic with a non-empty string for subscription at the beginning, and then update with a new filter (not empty string).
it means if users create a subscription without a content filter expression, this subscription can't subscribe to the content filter topic anymore.

don't be 😄 we just trying some new stuff, there will be something like this.

okay that is said, the followings should be done in user application aspect?

user creates normal subscription.

if user wants to use contentfilter, application provides the filtering expression and parameters which are not NULL. (actually this is the 1st time to create contentfilteredtopic based on the parent topic)

user can only have events from contentfilteredtopic but not from parent topic.

user can change and modify the filtering expression and parameters which are not NULL.

if the user wants to use normal topic (in other words, parent topic) user can specify NULL filtering expression and parameters? rmw takes NULL expression and then in the backend it will delete the contentfilteredtopic?

i am not sure if anything like this feasible, but if we do, it just appears to be the filtering expression and parameters interfaces only for user application. in other words, it can conceal creating/destroying contentfilteredtopic to user application, but rmw should take care of.

fujitatomoya · 2021-02-08T06:15:57Z

rcl/include/rcl/subscription.h

+ * \return `RCL_RET_OK` if the query was successful, or
+ * \return `RCL_RET_INVALID_ARGUMENT` if `subscription` is NULL, or
+ * \return `RCL_RET_INVALID_ARGUMENT` if `filter_expression` is NULL, or
+ * \return `RCL_RET_INCORRECT_RMW_IMPLEMENTATION` if the `node` implementation


RCL_RET_INCORRECT_RMW_IMPLEMENTATION is going to be defined in rcl later? i do not see this is already supported in rcl. and actually never used.

I'll remove it

fujitatomoya · 2021-02-08T07:01:56Z

rcl/src/rcl/subscription.c

-  subscription->impl->options = *options;
+  ret = rcl_subscription_options_copy(options, &subscription->impl->options);
+  if (RCL_RET_OK != ret) {
+    ret = RCL_RET_ERROR;


why we need to overwrite the error code here?

fujitatomoya · 2021-02-08T07:42:40Z

rcl/src/rcl/subscription.c

@@ -116,8 +119,12 @@ rcl_subscription_init(
  }
  subscription->impl->actual_qos.avoid_ros_namespace_conventions =
    options->qos.avoid_ros_namespace_conventions;
-  // options
-  subscription->impl->options = *options;
+  ret = rcl_subscription_options_copy(options, &subscription->impl->options);


I think at this time, calling rnw_create_subscription is to create parent subscription for content filtering. so actually it just saves filter_expression and expression_parameters, not to be used during creating subscription. am i understanding right? and beside, at this moment, it is likely that filter_expression and expression_parameters are empty, right? because rcl_subscription_set_cft_expression_parameters cannot be called before.

I think at this time, calling rnw_create_subscription is to create parent subscription for content filtering. so actually it just saves filter_expression and expression_parameters, not to be used during creating subscription. am i understanding right?

The concrete rmw(rmw_connextdds) had already directly used filter_expression and expression_parameters

rcl/rcl/src/rcl/subscription.c

Line 101 in 6ef26c8

subscription->impl->rmw_handle = rmw_create_subscription(

,
I thought that these two parameters should be backed up into rcl_subscription_impl_t, so it can prevent from accessing a pointer that might be changed outside.

and beside, at this moment, it is likely that filter_expression and expression_parameters are empty, right? because rcl_subscription_set_cft_expression_parameters cannot be called before.

Yes.

fujitatomoya · 2021-02-08T07:49:55Z

rcl/src/rcl/subscription.c

+    }
+  }
+
+  if (src->rmw_subscription_options.expression_parameters) {


probably return error if filter_expression is not null but expression_parameters is null. i believe this case cannot exist.

I'll move it inside the above if statement.

iuhilnehc-ynos

Thank you for your review, I'll update it

iuhilnehc-ynos · 2021-02-22T02:55:39Z

rcl/src/rcl/subscription.c

@@ -116,8 +119,12 @@ rcl_subscription_init(
  }
  subscription->impl->actual_qos.avoid_ros_namespace_conventions =
    options->qos.avoid_ros_namespace_conventions;
-  // options
-  subscription->impl->options = *options;
+  ret = rcl_subscription_options_copy(options, &subscription->impl->options);


I think at this time, calling rnw_create_subscription is to create parent subscription for content filtering. so actually it just saves filter_expression and expression_parameters, not to be used during creating subscription. am i understanding right?

The concrete rmw(rmw_connextdds) had already directly used filter_expression and expression_parameters

rcl/rcl/src/rcl/subscription.c

Line 101 in 6ef26c8

subscription->impl->rmw_handle = rmw_create_subscription(

,
I thought that these two parameters should be backed up into rcl_subscription_impl_t, so it can prevent from accessing a pointer that might be changed outside.

and beside, at this moment, it is likely that filter_expression and expression_parameters are empty, right? because rcl_subscription_set_cft_expression_parameters cannot be called before.

Yes.

iuhilnehc-ynos · 2021-02-22T02:56:56Z

rcl/src/rcl/subscription.c

+    }
+  }
+
+  if (src->rmw_subscription_options.expression_parameters) {


I'll move it inside the above if statement.

iuhilnehc-ynos · 2021-02-22T02:57:22Z

rcl/include/rcl/subscription.h

+ *
+ * \param[in] option The structure which its resources have to be deallocated.
+ * \return `RCL_RET_OK` if the memory was successfully freed, or
+ * \return `RCL_RET_INVALID_ARGUMENT` if log_levels is NULL, or


iuhilnehc-ynos · 2021-02-22T02:57:28Z

rcl/include/rcl/subscription.h

+ * Lock-Free          | Maybe [1]
+ *
+ * \param[in] subscription the subscription object to inspect.
+ * \param[in] filter_expression An filter expression to set.


Signed-off-by: Chen Lihui <[email protected]>

iuhilnehc-ynos force-pushed the topic-cft-support branch from 01ed156 to 8ef3b0d Compare December 2, 2020 07:26

to support a feature of content filtered topic

8ef3b0d

Signed-off-by: Chen Lihui <[email protected]>

Barry-Xu-2018 reviewed Dec 3, 2020

View reviewed changes

rcl/include/rcl/subscription.h Outdated Show resolved Hide resolved

rcl/include/rcl/subscription.h Outdated Show resolved Hide resolved

Chen Lihui added 2 commits December 3, 2020 15:58

Update function description

13b0dde

Signed-off-by: Chen Lihui <[email protected]>

Nit.

6ef26c8

Signed-off-by: Chen Lihui <[email protected]>

fujitatomoya reviewed Feb 8, 2021

View reviewed changes

iuhilnehc-ynos commented Feb 22, 2021

View reviewed changes

Update based on review

938876c

Signed-off-by: Chen Lihui <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

to support a feature of content filtered topic #1

to support a feature of content filtered topic #1

iuhilnehc-ynos commented Dec 2, 2020

fujitatomoya Feb 8, 2021

iuhilnehc-ynos Feb 22, 2021

fujitatomoya Feb 8, 2021

iuhilnehc-ynos Feb 22, 2021

fujitatomoya Feb 8, 2021

iuhilnehc-ynos Feb 22, 2021

fujitatomoya Feb 22, 2021

iuhilnehc-ynos Feb 22, 2021

fujitatomoya Feb 22, 2021

fujitatomoya Feb 8, 2021

iuhilnehc-ynos Feb 22, 2021

fujitatomoya Feb 8, 2021

iuhilnehc-ynos Feb 22, 2021

fujitatomoya Feb 8, 2021

iuhilnehc-ynos Feb 22, 2021

fujitatomoya Feb 8, 2021

iuhilnehc-ynos Feb 22, 2021

iuhilnehc-ynos left a comment

iuhilnehc-ynos Feb 22, 2021

iuhilnehc-ynos Feb 22, 2021

iuhilnehc-ynos Feb 22, 2021

iuhilnehc-ynos Feb 22, 2021

	* \return `RCL_RET_INVALID_ARGUMENT` if log_levels is NULL, or
	* \return `RCL_RET_INVALID_ARGUMENT` if option is NULL, or

	* \param[in] filter_expression An filter expression to set.
	* \param[in] filter_expression A filter expression to set.

to support a feature of content filtered topic #1

Are you sure you want to change the base?

to support a feature of content filtered topic #1

Conversation

iuhilnehc-ynos commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment