rclc icon indicating copy to clipboard operation
rclc copied to clipboard

[foxy] feature allows change of message pointer

Open BrettRD opened this issue 3 years ago • 5 comments

Following https://github.com/ros2/rclc/issues/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

BrettRD avatar Aug 18 '21 01:08 BrettRD

Codecov Report

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

Impacted file tree graph

@@            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%) :arrow_up:
rclc/src/rclc/executor_handle.c 77.31% <92.30%> (+7.45%) :arrow_up:

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.

codecov-commenter avatar Aug 18 '21 01:08 codecov-commenter

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

JanStaschulat avatar Aug 19 '21 10:08 JanStaschulat

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.

BrettRD avatar Aug 19 '21 12:08 BrettRD

@BrettRD are you still working on this pull request?

JanStaschulat avatar Apr 01 '22 14:04 JanStaschulat

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.

BrettRD avatar Apr 02 '22 03:04 BrettRD