rosidl
rosidl copied to clipboard
ros2
Investigate using rcutils allocators for the C generators
Based on a comment at https://github.com/ros2/rosidl/pull/300#issuecomment-432038951, we should see if we can/should use the rcutils allocators when generating the C code for messages/services. Then we could potentially create a custom allocator to test that messages are being properly freed on error paths.
@clalancette if it's alright with you, I'd be more than happy to look into this.
@allenh1 We should probably coordinate with @wjwwood a bit on this, as he has "adding allocators" as part of the larger memory management list here: https://github.com/ros2/ros2/issues/590 . @wjwwood Do you have a plan to start working on that, or should we let @allenh1 loose on it?
I’m hesitant to turn away help but I have already started working on this in part. Specifically I’ve been adding allocator calls to the typesupport functions responsible for converting ros types to dds types. Which would in turn pass that allocator to the rosidl functions described here. Let me see if there’s a reasonable place to do separation of labor in this and get back to you.
I'm interested in ROS2 realtype behavior, and checking malloc call(see Memory management in Introduction to Real-time Systems).
I found some malloc calls such as rosidl_generator_c__String__init in rosidl/rosidl_generator_c/src/string_functions.c.
This is called at RCLCPP_INFO log macro, so called in run-time(ex. demo_nodes_cpp/src/topics/talker.cpp).
And this is also called at convert_dds_to_ros of rosidl_typesupport_{DDS}, so I wonder this may affect subscription.
@wjwwood How is the situation on this issue?
If the problem is unsolved, I'm thinking of writing a small patch.
(As rcl package uses static rcl_allocator_t, I plan to convey rcl allocator to this layer and replace malloc() with allocate() of rcl allocator.)
If the problem is unsolved, I'm thinking of writing a small patch. (As rcl package uses static rcl_allocator_t, I plan to convey rcl allocator to this layer and replace malloc() with allocate() of rcl allocator.)
It is still unsolved, and I think changing the functions that use malloc internally so that they take a rcutils_allocator_t (I think using rcl_allocator_t might cause a circular reference as rcl uses some of the message types), is the right way to go.
I'll also add that we haven't done this yet because right now the C messages are mostly only used by Python, and the C++ ones have the allocator template argument to control their behavior (though that has issues too). The fact that logging creates a C message instance that uses malloc (for the strings I guess) is a relatively new change, and many of our users that are doing real-time replace the logging implementation anyways (so that it does not publish and therefore does not need to make a new message instance).
@wjwwood Thank you for reply.
rcutils_allocator_t (snip) is the right way to go.
I see.
many of our users that are doing real-time replace the logging implementation anyways
Yeah, logging seems not a big problem.
How do you think the priority of below problems?
- malloc-using functions are called in
rosidl_typesupport_<DDS>layers. For example,rosidl_generator_c__String__initis called at rosidl_typesupport_connext/rosidl_typesupport_connext_c/resource/msg__type_support_c.cpp.em The function name of caller is_@(message.structure.namespaced_type.name)__convert_dds_to_ros. This is a parser and may be called in run-time (refered in the first my post). - Other malloc-using function in rosidl such as rosidl/rosidl_generator_c/src/primitives_sequence_functions.c. They seems for more general purpose.
As break-points on these functions doesn't stop the program, I cannot judge the seriousness of these right now.
How do you think the priority of below problems?
Both of those are in the C type support (as opposed to the C++ type support). The only thing using the C type support right now is the Python client library (rclpy) and some tests in rcl, so I don't think they are hitting anyone at the moment (not urgent). But they should be addressed at some point. Basically anywhere we use malloc or new we should allow the user to override that somehow.
For the C++ equivalents, we are just using new and delete instead of malloc. But that can be controlled with the allocator template argument.
I think all calls to malloc and free have been replaced with the default allocator provided by rcutils in https://github.com/ros2/rosidl/pull/584
However, I think we should make further changes to accept a non-default allocator. I am planning to look into this.
@jacobperron as commented by @y-okumura-isp, in micro-ROS we added a setter for the default allocator in our rcutils fork: https://github.com/micro-ROS/rcutils/blob/85efa4ad786fb6920ae92156ab439dec0315299b/src/allocator.c#L87
This allows a basic operation with custom allocators such as the FreeRTOS's heap managers.
I guess being able modify the default allocator is a valid solution, but it doesn't follow the pattern we use in many places in rcl where we pass in an allocator per function. Although passing in an allocator every time is more verbose, it is more flexible since we have the option to use different allocators in the same application. We can still have an API for creating messages with the default allocator.