improve rcl_action tests failures non-default RMW impls.#1301
improve rcl_action tests failures non-default RMW impls.#1301fujitatomoya wants to merge 1 commit intorollingfrom
Conversation
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
| // depending on rmw implementation, this call may set an error string (e.g. "unimplemented") | ||
| // so we must call rcutils_reset_error() in both branches to avoid leaking a stale error | ||
| // state out of subscription init. | ||
| rcutils_reset_error(); |
There was a problem hiding this comment.
i think this makes sense because this is being called in ROS 2 system, not by user application. and eventually construction of subscription succeeds even if rmw implementation does not support content filtering feature. so we should reset the error strings here.
if the user application calls rmw_subscription_get_content_filter on purpose, the error strings will stay and user application expects the message unimplemented....
There was a problem hiding this comment.
uhm, just move it out of the if/else entirely ?
Anyway, I am confused about what is going on here, why are we calling this in the constructor in the first place and not in rcl_subscription_is_cft_supported ?
Also I can't find any place, were rcl_subscription_is_cft_supported is actually called in the whole stack.
There was a problem hiding this comment.
i think we need to call cft API to see if that is supported or not at the construction.
Also I can't find any place, were rcl_subscription_is_cft_supported is actually called in the whole stack.
this is planned to use upper layer for improvement, #1287 and ros2/rclcpp#3034
There was a problem hiding this comment.
uhm, just move it out of the if/else entirely ?
The same suggestion.
|
@Barry-Xu-2018 can you review this? |
|
Pulls: #1301
|
|
Pulls: #1301
|
|
i started 2 CI with/without |
| // depending on rmw implementation, this call may set an error string (e.g. "unimplemented") | ||
| // so we must call rcutils_reset_error() in both branches to avoid leaking a stale error | ||
| // state out of subscription init. | ||
| rcutils_reset_error(); |
There was a problem hiding this comment.
uhm, just move it out of the if/else entirely ?
Anyway, I am confused about what is going on here, why are we calling this in the constructor in the first place and not in rcl_subscription_is_cft_supported ?
Also I can't find any place, were rcl_subscription_is_cft_supported is actually called in the whole stack.
|
I just went a bit over the code and directly found more stuff that is wrong here. rcl/rcl/src/rcl/subscription.c Line 488 in c5fe2bf Never returns true, even if content filtering is actually active. @iuhilnehc-ynos can you have a look at this ? |
if the content filtering expression and parameters are not set (means using content filtering), this returns false. that is expected behavior.
unfortunately he is not working with us anymore... i do not think we can get reply from him... |
There is no place, were is_cft_enabled is ever set to true, this looks strange. Also there is no test that it ever returns true, only tests for false. |
|
hmmm that is strange, true case should be tested with cft supported rmws... @Barry-Xu-2018 can you check on this? for this PR, i am okay with either way. keeping the caching |
Whether an RMW implementation supports content filters is fixed, so I think it should be determined during subscription initialization. The upper-level rclcpp/rclpy can directly provide an is_cft_support() function, instead of attempting to configure a content filter and only discovering that the RMW implementation does not support it when it returns "unsupported." This approach makes the code clearer. The original logic has already been optimized based on this way. If using the lazy approach, two functions need to be modified. When the RMW implementation does not support content filters and their RMW functions return "unsupported", we need to set is_cft_supported accordingly.
|
Here is my understanding. bool
rcl_subscription_is_cft_enabled(const rcl_subscription_t * subscription)
{
if (!rcl_subscription_is_valid(subscription)) {
return false;
}
return subscription->impl->rmw_handle->is_cft_enabled;
}
rmw_ret_t
rmw_subscription_set_content_filter(
rmw_subscription_t * subscription,
const rmw_subscription_content_filter_options_t * options)
{
RMW_CHECK_ARGUMENT_FOR_NULL(subscription, RMW_RET_INVALID_ARGUMENT);
RMW_CHECK_ARGUMENT_FOR_NULL(options, RMW_RET_INVALID_ARGUMENT);
RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
subscription,
subscription->implementation_identifier,
eprosima_fastrtps_identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
rmw_ret_t ret = rmw_fastrtps_shared_cpp::__rmw_subscription_set_content_filter(
subscription, options);
auto info = static_cast<const CustomSubscriberInfo *>(subscription->data);
subscription->is_cft_enabled = (info && info->filtered_topic_); <<=== Here
return ret;
}Even if |
Actually, it was tested. Use rcl/rcl/test/rcl/test_subscription.cpp Lines 937 to 974 in c5fe2bf You’ll notice that the test doesn’t directly check the return value of |
|
@Barry-Xu-2018 Thanks for the explanation on where and how is_cft_enabled is set and manipulated. This now opens new question for me : E.g.: // Check if the subscription supports content filtering
// If RMW implementation doesn't support content filtering, calling
// rmw_subscription_get_content_filter() with unavailable parameters will always return
// RMW_RET_UNSUPPORTED.
if (rmw_subscription_get_content_filter(
subscription->impl->rmw_handle, NULL, NULL) == RMW_RET_UNSUPPORTED)
{
subscription->impl->rmw_handle->is_cft_supported = false;
} else {
rcutils_reset_error();
subscription->impl->rmw_handle->is_cft_supported = true;
}This code block should not be present at all, subscription->impl->rmw_handle->is_cft_supportedmust contain the correct value after rmw_create_subscription. Anything else I would consider a bug in the rmw implementation. |
You’re right, this value really should be set in rmw_xxx. At the time when we designed the RMW CFT interface, we indeed did not consider the existence of the variable subscription->impl->rmw_handle->is_cft_supported (I added this value recently. ros2/rmw#415). Now, it would be quite troublesome to require each rmw_xxx to modify and fill in this value themselves, so I used a workaround: during initialization in rcl, |
|
I would argue that the design is now broken. And, as only fastDDS supports this anyway, we only need to do one patch. For the rest it should |
|
@jmachowinski @Barry-Xu-2018 thanks for the comments. so after all, i agree that we should put that in rmw implementation, not in rcl. with that, we do not even need this PR i believe.
RTI ConnextDDS also supports this. |
Thank you for your comments. I will make changes to rmw_fastdds/rmw_connext to remove this workaround at the RCL layer. |
I'll handle it. |
|
PRs have been created. |
|
Closing this PR in favor for #1304 |
3 participants
Description
different approach from ros2/rmw_cyclonedds#565
this PR fixes the following failures with
rmw_cyclonedds_cpp,this PR just call
rcutils_reset_errorright afterrmw_subscription_get_content_filterduring the construction of subscription. this is being done in the ROS 2 system library, not issued by user application by purpose. so we should reset the error strings to avoid leaking a stale error state out of subscription init.this can fix the many warnings at once. the thing is that, some unit tests are only running with default rmw implementation which is rmw_fastrtps that supports content filtering feature. so those warnings are not observed before unless specific rmw configuration is set to start the test.
part of ros2/rmw_cyclonedds#550
Is this user-facing behavior change?
No
Did you use Generative AI?
No
Additional Information