[foxy] feature allows change of message pointer

[BrettRD]

Following #186 This PR adds:

• the ability to change most message pointers passed into callbacks (useful for queues of complex pre-allocated messages)
• a little bit more type safety to rclc_executor_remove_subscription()
• a convenience function to reduce diff sizes when implementing rclc_executor_add_client_with_context and rclc_executor_add_guard_condition_with_context

API additions:
replace a message pointer when only the message pointer is known (useful for context-free callbacks)

• rclc_executor_swap_subscription_message
• rclc_executor_swap_client_response
• rclc_executor_swap_service_request

change a message pointer when the rcl handle is known (preferred)

• rclc_executor_change_subscription_message
• rclc_executor_change_client_response
• rclc_executor_change_service_request

Omissions:
rclc_executor_swap_service_response requires slightly different machinery, and will be left out of this pull request

Remaining work:
The doc strings need adjustments to show some thread-safety caveats (run during respective callbacks or not during spin)
The tests still need writing, as do regression tests to demonstrate the previous type-safety flaw
previously:

rclc_executor_t executor;
rcl_timer_t timer;
rclc_executor_add_timer(&executor, &timer);
rclc_executor_remove_subscription(&executor, &timer); 
// the last line returns ok, removes the timer handle, and decrements the subscription count

after writing this, I've realised that similar performance can come from changing only the variable-sized parts of a message without needing to interact with the executor, but this API still has some utility for complex nested messages

[codecov-commenter]

Codecov Report

Merging #190 (417a107) into foxy (d711b28) will increase coverage by 2.93%.
The diff coverage is 85.62%.

@@            Coverage Diff             @@
##             foxy     #190      +/-   ##
==========================================
+ Coverage   59.94%   62.87%   +2.93%     
==========================================
  Files          11       11              
  Lines        1016     1142     +126     
  Branches      331      367      +36     
==========================================
+ Hits          609      718     +109     
- Misses        275      280       +5     
- Partials      132      144      +12     

Impacted Files	Coverage Δ
rclc/src/rclc/executor.c	59.15% <84.39%> (+3.38%)	⬆️
rclc/src/rclc/executor_handle.c	77.31% <92.30%> (+7.45%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d711b28...417a107. Read the comment docs.

[JanStaschulat]

Could you add an example in rclc_examples demonstrating the new feature?

Especially I'd like to understand, how data consistency is ensured: Consider the case, in which handle->data is processed via a queue in a different thread while potentially spin_some is called by the Executor again and new data is assigned to handle->data. Then this data will be corrupted. In the (single-threaded) rclc-Executor it is assumed, that after calling the subscription callback, handle->data can be used for a new message.

See my comment in the issue #186

[BrettRD]

This feature is intended to be used with complex messages where copy is too expensive and mesages need to be moved by pointer, and the application pre-allocates more than one message.

Calling rclc_executor_change_subscription_message will completely remove the message in handle->data from the executor, making it safe to place the callback argument (message pointer, previous contents of handle->data) into a processing queue.
The new (allocated and initialised) message pointer placed in handle->data should not be touched by another thread until the next callback.

The respective subscription callback is the only sane place to call these functions.

For most message types, this is unnecessary:
Messages without variable-length components are cheap and easy to copy,
Messages with one large variable length component (std_msgs__string and sensor_msgs__image) already allow the user to reach into msg->data.data to re-assign the large internal buffer from a buffer pool, then shallow-copy the rest.

For deeply nested messages, this pointer shuffling saves the user from executing a deep-copy, or implementing a complicated shallow-copy-and-re-nesting

I'll build an example.

[JanStaschulat]

LGTM, just minor improvments and

• improve test cases (e.g. for other rcl_handles).
• an example in rclc_example package
• concept for data consistency (as discussed in the issue)

I have not completely understood, how you want to use this function in your application.
Do you want to change/swap the message every time after you have processed it? Or only once in the beginning? Does then the handle->data pointer stay alive in the queue (which is processed in a different thread)?

[JanStaschulat]

You might want to use this macro to test any pointer argument for NULL in all added functions. Example:
RCL_CHECK_FOR_NULL_WITH_MSG(executor, "executor is NULL", return RCL_RET_INVALID_ARGUMENT);

rclc/rclc/src/rclc/executor.c

Line 99 in 865b02b

RCL_CHECK_FOR_NULL_WITH_MSG(executor, "executor is NULL", return RCL_RET_INVALID_ARGUMENT);

[BrettRD]

Good call, but that one needs to stay as a special-case; _rclc_executor_find_handle does not return a rcl_ret_t, so RCL_CHECK_FOR_NULL_WITH_MSG would cause a return type mismatch compile error.

For this function, NULL is a type-correct value with a valid meaning (no handle found), and every function that calls static rclc_executor_handle_t * _rclc_executor_find_handle() already uses RCL_CHECK_FOR_NULL_WITH_MSG to test the executor.

[JanStaschulat]

It should be a black box test. So actually, you did not test the functionality of rclc_executor_handle_reduced_type that reduces SUBSCRIPTION_xxx to SUBSCRIPTION and ... CLIENT_xxx to CLIENT. A mafunctioning implementation could just reduce everything to NONE and this test would always pass.

Sorry, to be picky, but I follow the principle of "test driven development". For every functionality there needs to be a proper test case that shows the correctness of the implementation (in black box testing). This might be tedious for these simple function, but when you start developing more complex software, good unit tests become very valuable.

I suggest sth like this:

full_type = SUBSCRIPTION;
reduced_type = rclc_executor_handle_reduced_type(full_type);
EXPECT_EQ(reduced_type, SUBSCRIPTION << "expected SUBSCRIPTION";

full_type = SUBSCRIPTION_WITH_CONTEXT;
reduced_type = rclc_executor_handle_reduced_type(full_type);
EXPECT_EQ(reduced_type, SUBSCRIPTION << "expected SUBSCRIPTION";

full_type = TIMER;
reduced_type = rclc_executor_handle_reduced_type(full_type);
EXPECT_EQ(reduced_type, TIMER << "expected TIMER";

full_type = CLIENT;
reduced_type = rclc_executor_handle_reduced_type(full_type);
EXPECT_EQ(reduced_type, CLIENT << "expected CLIENT";

full_type = CLIENT_WITH_REQ_ID;
reduced_type = rclc_executor_handle_reduced_type(full_type);
EXPECT_EQ(reduced_type, CLIENT << "expected CLIENT";
...

[BrettRD]

I didn't want to put in an exhaustive test because an exhaustive test would look complete but (being a re-implementaton) would pass on all of the most likely failure cases, especially New types.
codecov only flagged the default case as unexplored, and strict idempotency is that case's strongest property.

I'll add some more cases and explain that better in the test code.

[JanStaschulat]

LGTM, these test cases look great to me. Please also add test cases for client, service, timer, gc. I guess, you can very easily copy-and-paste these test cases to be used for other rcl_handles.

[JanStaschulat]

@BrettRD are you still working on this pull request?

[BrettRD]

I had to abandon micro-ros and ESP32 as a platform because the data handling implementations were too inefficient.
The amount of time spent copying data buffers out of micro-ros, and into the esp-adf completely blew the processing budget of the platform.
Without zero-copy features like this, rclc is unsuitable for high-performance applications.

This feature allows a subscriber to loan memory to the executor, and zero-copy move the subscription data into the esp-adf.
The swap syntax ensures that if this call is made within the executor's thread (ie, at the end of the subscription callback), the executor will always have a suitable buffer to write the subscription into.\

I believe the requested changes to the tests are complete.
The doc strings of the swap functions describe their threading consistency requirements
I'm unlikely to have time to write an example.