-
Notifications
You must be signed in to change notification settings - Fork 42
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
Adds a context pointer to subscriptions #107
Changes from all commits
54ec3ca
b471d64
faa3591
fc6e563
a201b6d
bc5e7a8
cd31f61
764708a
a94abfd
d2cfb1a
0ed3bf0
d6eeb93
26655f6
dfec98b
d4221fb
a1ff0f7
ba33010
38bb02a
84dd490
11501a7
c7e09de
dcebfee
d75c2f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,3 +33,5 @@ eProsima | |
Julián Bermúdez Ortega <[email protected]> | ||
|
||
Loïc Dauphin <[email protected]> | ||
|
||
Brett Downing <[email protected]> |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -222,6 +222,7 @@ rclc_executor_add_subscription( | |||||||||||||||||||||||
executor->handles[executor->index].subscription = subscription; | ||||||||||||||||||||||||
executor->handles[executor->index].data = msg; | ||||||||||||||||||||||||
executor->handles[executor->index].callback = callback; | ||||||||||||||||||||||||
executor->handles[executor->index].callback_type = CB_WITHOUT_REQUEST_ID; | ||||||||||||||||||||||||
executor->handles[executor->index].invocation = invocation; | ||||||||||||||||||||||||
executor->handles[executor->index].initialized = true; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
@@ -244,6 +245,56 @@ rclc_executor_add_subscription( | |||||||||||||||||||||||
return ret; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
rcl_ret_t | ||||||||||||||||||||||||
rclc_executor_add_subscription_with_context( | ||||||||||||||||||||||||
rclc_executor_t * executor, | ||||||||||||||||||||||||
rcl_subscription_t * subscription, | ||||||||||||||||||||||||
void * msg, | ||||||||||||||||||||||||
rclc_subscription_callback_with_context_t callback, | ||||||||||||||||||||||||
void * context, | ||||||||||||||||||||||||
rclc_executor_handle_invocation_t invocation) | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
RCL_CHECK_ARGUMENT_FOR_NULL(executor, RCL_RET_INVALID_ARGUMENT); | ||||||||||||||||||||||||
RCL_CHECK_ARGUMENT_FOR_NULL(subscription, RCL_RET_INVALID_ARGUMENT); | ||||||||||||||||||||||||
RCL_CHECK_ARGUMENT_FOR_NULL(msg, RCL_RET_INVALID_ARGUMENT); | ||||||||||||||||||||||||
RCL_CHECK_ARGUMENT_FOR_NULL(callback, RCL_RET_INVALID_ARGUMENT); | ||||||||||||||||||||||||
rcl_ret_t ret = RCL_RET_OK; | ||||||||||||||||||||||||
// array bound check | ||||||||||||||||||||||||
if (executor->index >= executor->max_handles) { | ||||||||||||||||||||||||
RCL_SET_ERROR_MSG("Buffer overflow of 'executor->handles'. Increase 'max_handles'"); | ||||||||||||||||||||||||
return RCL_RET_ERROR; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// assign data fields | ||||||||||||||||||||||||
executor->handles[executor->index].type = SUBSCRIPTION; | ||||||||||||||||||||||||
executor->handles[executor->index].subscription = subscription; | ||||||||||||||||||||||||
executor->handles[executor->index].data = msg; | ||||||||||||||||||||||||
executor->handles[executor->index].subscription_callback_with_context = callback; | ||||||||||||||||||||||||
executor->handles[executor->index].callback_type = CB_WITH_CONTEXT; | ||||||||||||||||||||||||
executor->handles[executor->index].invocation = invocation; | ||||||||||||||||||||||||
executor->handles[executor->index].initialized = true; | ||||||||||||||||||||||||
executor->handles[executor->index].callback_context = context; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// increase index of handle array | ||||||||||||||||||||||||
executor->index++; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// invalidate wait_set so that in next spin_some() call the | ||||||||||||||||||||||||
// 'executor->wait_set' is updated accordingly | ||||||||||||||||||||||||
if (rcl_wait_set_is_valid(&executor->wait_set)) { | ||||||||||||||||||||||||
ret = rcl_wait_set_fini(&executor->wait_set); | ||||||||||||||||||||||||
if (RCL_RET_OK != ret) { | ||||||||||||||||||||||||
RCL_SET_ERROR_MSG("Could not reset wait_set in rclc_executor_add_subscription_with_context."); | ||||||||||||||||||||||||
return ret; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
executor->info.number_of_subscriptions++; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Added a subscription."); | ||||||||||||||||||||||||
return ret; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
rcl_ret_t | ||||||||||||||||||||||||
rclc_executor_add_timer( | ||||||||||||||||||||||||
rclc_executor_t * executor, | ||||||||||||||||||||||||
|
@@ -460,7 +511,7 @@ rclc_executor_add_service_with_context( | |||||||||||||||||||||||
executor->handles[executor->index].callback_type = CB_WITH_CONTEXT; | ||||||||||||||||||||||||
executor->handles[executor->index].invocation = ON_NEW_DATA; // invoce when request came in | ||||||||||||||||||||||||
executor->handles[executor->index].initialized = true; | ||||||||||||||||||||||||
executor->handles[executor->index].service_context = context; | ||||||||||||||||||||||||
executor->handles[executor->index].callback_context = context; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
// increase index of handle array | ||||||||||||||||||||||||
executor->index++; | ||||||||||||||||||||||||
|
@@ -923,10 +974,28 @@ _rclc_execute(rclc_executor_handle_t * handle) | |||||||||||||||||||||||
if (invoke_callback) { | ||||||||||||||||||||||||
switch (handle->type) { | ||||||||||||||||||||||||
case SUBSCRIPTION: | ||||||||||||||||||||||||
if (handle->data_available) { | ||||||||||||||||||||||||
handle->callback(handle->data); | ||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||
handle->callback(NULL); | ||||||||||||||||||||||||
switch (handle->callback_type) { | ||||||||||||||||||||||||
case CB_WITHOUT_REQUEST_ID: | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes.
I don't think it needs another callback-type, just a better name.
rclc/rclc/include/rclc/executor_handle.h Lines 139 to 148 in c7e09de
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree. Initally, we only had services, subscriptions, clients. Now we have multiple types of callbacks for subscriptions, services and clients:
It would make sense to refactor handle->type and add e..g the request-id is currently not used in services with context. @norro. It looks like that the implementation of the client callback with context is not properly called: it only calls the client callback without context: Line 976 in 0263722
You never use a client callback with context? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added an issue for that :#115 |
||||||||||||||||||||||||
if (handle->data_available) { | ||||||||||||||||||||||||
handle->callback(handle->data); | ||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||
handle->callback(NULL); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
break; | ||||||||||||||||||||||||
case CB_WITH_CONTEXT: | ||||||||||||||||||||||||
if (handle->data_available) { | ||||||||||||||||||||||||
handle->subscription_callback_with_context( | ||||||||||||||||||||||||
handle->data, | ||||||||||||||||||||||||
handle->callback_context); | ||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||
handle->subscription_callback_with_context( | ||||||||||||||||||||||||
NULL, | ||||||||||||||||||||||||
handle->callback_context); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
break; | ||||||||||||||||||||||||
default: | ||||||||||||||||||||||||
PRINT_RCLC_ERROR(rclc_execute, unknown_callback_type); | ||||||||||||||||||||||||
break; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
break; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
@@ -955,7 +1024,7 @@ _rclc_execute(rclc_executor_handle_t * handle) | |||||||||||||||||||||||
handle->service_callback_with_context( | ||||||||||||||||||||||||
handle->data, | ||||||||||||||||||||||||
handle->data_response_msg, | ||||||||||||||||||||||||
handle->service_context); | ||||||||||||||||||||||||
handle->callback_context); | ||||||||||||||||||||||||
break; | ||||||||||||||||||||||||
default: | ||||||||||||||||||||||||
PRINT_RCLC_ERROR(rclc_execute, unknown_callback_type); | ||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -268,6 +268,20 @@ void int32_callback5(const void * msgin) | |||||
} | ||||||
} | ||||||
|
||||||
void int32_callback_with_context(const void * msgin, void * context) | ||||||
{ | ||||||
const std_msgs__msg__Int32 * msg = (const std_msgs__msg__Int32 *)msgin; | ||||||
if (msg == NULL) { | ||||||
printf("(int32_callback_with_context): msg is NULL\n"); | ||||||
} | ||||||
if (context == NULL) { | ||||||
printf("(int32_callback_with_context): context is NULL\n"); | ||||||
} else { | ||||||
// This side effect allows the test to check that the subscription received its context | ||||||
int32_t * sub_context_value = reinterpret_cast<int32_t *>( context ); | ||||||
*sub_context_value = msg->data; | ||||||
} | ||||||
} | ||||||
|
||||||
void service_callback(const void * req_msg, void * resp_msg) | ||||||
{ | ||||||
|
@@ -697,6 +711,66 @@ TEST_F(TestDefaultExecutor, executor_add_subscription) { | |||||
EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str; | ||||||
} | ||||||
|
||||||
TEST_F(TestDefaultExecutor, executor_add_subscription_with_context) { | ||||||
rcl_ret_t rc; | ||||||
rclc_executor_t executor; | ||||||
// test with normal arguemnt and NULL pointers as arguments | ||||||
rc = rclc_executor_init(&executor, &this->context, 10, this->allocator_ptr); | ||||||
EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str; | ||||||
|
||||||
int32_t sub_context_value = 0; | ||||||
void * sub_context_ptr = reinterpret_cast<void *>( &sub_context_value ); | ||||||
|
||||||
// normal case | ||||||
rc = rclc_executor_add_subscription_with_context( | ||||||
&executor, &this->sub1, &this->sub1_msg, | ||||||
&int32_callback_with_context, sub_context_ptr, ON_NEW_DATA); | ||||||
EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str; | ||||||
size_t num_subscriptions = 1; | ||||||
EXPECT_EQ(executor.info.number_of_subscriptions, num_subscriptions) << | ||||||
"number of subscriptions is expected to be one"; | ||||||
|
||||||
// test NULL pointer for executor | ||||||
rc = rclc_executor_add_subscription_with_context( | ||||||
NULL, &this->sub1, &this->sub1_msg, &int32_callback_with_context, | ||||||
sub_context_ptr, ON_NEW_DATA); | ||||||
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rc) << rcl_get_error_string().str; | ||||||
rcutils_reset_error(); | ||||||
EXPECT_EQ(executor.info.number_of_subscriptions, num_subscriptions) << | ||||||
"number of subscriptions is expected to be one"; | ||||||
|
||||||
// test NULL pointer for subscription | ||||||
rc = rclc_executor_add_subscription_with_context( | ||||||
&executor, NULL, &this->sub1_msg, &int32_callback_with_context, | ||||||
sub_context_ptr, ON_NEW_DATA); | ||||||
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rc) << rcl_get_error_string().str; | ||||||
rcutils_reset_error(); | ||||||
EXPECT_EQ(executor.info.number_of_subscriptions, num_subscriptions) << | ||||||
"number of subscriptions is expected to be one"; | ||||||
|
||||||
// test NULL pointer for message | ||||||
rc = rclc_executor_add_subscription_with_context( | ||||||
&executor, &this->sub1, NULL, &int32_callback_with_context, | ||||||
sub_context_ptr, ON_NEW_DATA); | ||||||
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rc) << rcl_get_error_string().str; | ||||||
rcutils_reset_error(); | ||||||
EXPECT_EQ(executor.info.number_of_subscriptions, num_subscriptions) << | ||||||
"number of subscriptions is expected to be one"; | ||||||
|
||||||
// test NULL pointer for callback | ||||||
rc = rclc_executor_add_subscription_with_context( | ||||||
&executor, &this->sub1, &this->sub1_msg, NULL, | ||||||
sub_context_ptr, ON_NEW_DATA); | ||||||
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rc) << rcl_get_error_string().str; | ||||||
rcutils_reset_error(); | ||||||
EXPECT_EQ(executor.info.number_of_subscriptions, num_subscriptions) << | ||||||
"number of subscriptions is expected to be one"; | ||||||
|
||||||
// tear down | ||||||
rc = rclc_executor_fini(&executor); | ||||||
EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str; | ||||||
} | ||||||
|
||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would need a test case where you add a subscription with a context, publish data and check if the data was correctly handled, like this one for rclc/rclc/test/rclc/test_executor.cpp Line 2347 in 0263722
or a simplified version of this test case rclc/rclc/test/rclc/test_executor.cpp Line 1241 in 0263722
in your test case you only need one subsction. e.g. add_subscription to executor , publish a topic, wait for some time, call spin_some and then copy the received data and the context data in the subscription_with_context_callback to a global variable. These ones you can then check in the test case. Would be great, if you could add an example in rclc_examples repository that shows how to use this feature. You could use this one as a base-line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test added, Example added. |
||||||
TEST_F(TestDefaultExecutor, executor_add_subscription_too_many) { | ||||||
rcl_ret_t rc; | ||||||
rclc_executor_t executor; | ||||||
|
@@ -2447,6 +2521,47 @@ TEST_F(TestDefaultExecutor, executor_test_service_with_context) { | |||||
example_interfaces__srv__AddTwoInts_Response__fini(&cli_resp); | ||||||
} | ||||||
|
||||||
TEST_F(TestDefaultExecutor, executor_test_subscription_with_context) { | ||||||
// This unit test tests, that a subscription with context receives the correct context pointer | ||||||
rcl_ret_t rc; | ||||||
rclc_executor_t executor; | ||||||
executor = rclc_executor_get_zero_initialized_executor(); | ||||||
rc = rclc_executor_init(&executor, &this->context, 10, this->allocator_ptr); | ||||||
EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str; | ||||||
|
||||||
// prepare a context pointer carrying a value that differs from the published value | ||||||
int32_t sub_context_value = 0; | ||||||
void * sub_context_ptr = reinterpret_cast<void *>( &sub_context_value ); | ||||||
|
||||||
std_msgs__msg__Int32__init(&this->pub1_msg); | ||||||
this->pub1_msg.data = 42; | ||||||
|
||||||
// create a subscription on the same topic as our publisher | ||||||
EXPECT_EQ(executor.info.number_of_subscriptions, (size_t) 0) << | ||||||
"number of subscriptions was not initialised at zero"; | ||||||
|
||||||
rc = rclc_executor_add_subscription_with_context( | ||||||
&executor, &this->sub1, &this->sub1_msg, | ||||||
&int32_callback_with_context, sub_context_ptr, ON_NEW_DATA); | ||||||
EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str; | ||||||
EXPECT_EQ(executor.info.number_of_subscriptions, (size_t) 1) << | ||||||
"number of subscriptions is expected to be one"; | ||||||
|
||||||
// run the callback | ||||||
rc = rcl_publish(&this->pub1, &this->pub1_msg, nullptr); | ||||||
EXPECT_EQ(RCL_RET_OK, rc) << " pub1 not published"; | ||||||
std::this_thread::sleep_for(rclc_test_sleep_time); | ||||||
rclc_executor_spin_some(&executor, rclc_test_timeout_ns); | ||||||
|
||||||
// check the side effect | ||||||
EXPECT_EQ(this->pub1_msg.data, sub_context_value) << | ||||||
"subscription did not alter context value"; | ||||||
|
||||||
// tear down | ||||||
rc = rclc_executor_fini(&executor); | ||||||
EXPECT_EQ(RCL_RET_OK, rc) << rcl_get_error_string().str; | ||||||
} | ||||||
|
||||||
TEST_F(TestDefaultExecutor, executor_test_guard_condition) { | ||||||
// Test guard_condition. | ||||||
rcl_ret_t rc; | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you want to change this? Default is
CB_UNDEFINED
The type
CB_WITH_REQUEST_ID
andCB_WITHOUT_REQUEST_ID
is currently used for services/clients.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also prefer
CB_WITHOUT_CONTEXT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subscriptions without context need something in their
callback_type
that is well-defined, but has neither request-id nor context.CB_WITHOUT_CONTEXT
sounds more generic, but is no more informative thanCB_WITHOUT_REQUEST_ID
, and would require another enum value.(excuse me if this argument is already settled elsewhere, I'm catching up on the recent activity)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but
CB_WITHOUT_REQUEST_ID
only makes sense for services and clients.CB_WITHOUT_CONTEXT
is informative, that it is a normal callback. If you prefer another wording, how aboutCB_DEFAULT
as a neutral enum value.