rclc
rclc copied to clipboard
[foxy] feature allows change of message pointer
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
andrclc_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 Report
Merging #190 (417a107) into foxy (d711b28) will increase coverage by
2.93%
. The diff coverage is85.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%) |
: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.
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
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 are you still working on this pull request?
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.