Skip to content

Conversation

@gonzodepedro
Copy link

@gonzodepedro gonzodepedro commented Feb 7, 2019

The idea is to have a bool value in init_options to enable the rmw_preallocate feature.

Connects to ros2/rmw#159

@gonzodepedro gonzodepedro requested a review from wjwwood February 7, 2019 16:38
@gonzodepedro gonzodepedro added the in progress Actively being worked on (Kanban column) label Feb 7, 2019
@hidmic hidmic changed the title Proposol of changes for RMW_Preallocate. Proposal of changes for RMW_Preallocate. Feb 7, 2019
rcl_publisher_options_t options;
rcl_context_t * context;
rmw_publisher_t * rmw_handle;
rmw_publisher_allocation_t * allocation;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot just store a publish preallocation in the publisher because it is not thread safe. The preallocation needs to be created by the user or by rclcpp in order to ensure it is used in a thread safe manner.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. In that case the RCL should be changed or augmented to allow the user to pass the allocation to RCL.

@gonzodepedro gonzodepedro added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 26, 2019
@gonzodepedro gonzodepedro force-pushed the gonzodepedro/rmw_preallocate branch from ac7a66f to c03e4fb Compare March 28, 2019 19:37
@gonzodepedro
Copy link
Author

Currently the test_memory_prealloc tests are built with the LD_BIND_NOW=1 option. This is because without that option the tests fail. Apparently the dynamic linker is allocating memory on run-time and that makes the memory tests fail.

@wjwwood
Copy link
Member

wjwwood commented Apr 3, 2019

Apparently the dynamic linker is allocating memory on run-time and that makes the memory tests fail.

At what step? publish? The library should already be loaded at that point. If it is TLS, we have functions to address specific cases of that, e.g.: https://github.com/ros2/rcutils/blob/fe82622f3c9d76400ad6065229777aa013fb8628/include/rcutils/error_handling.h#L101-L141

@jacobperron
Copy link
Member

I think this is connected to the wrong card. The description should be updated to connect to ros2/rmw#159, right?

@gonzodepedro
Copy link
Author

I think this is connected to the wrong card. The description should be updated to connect to ros2/rmw#159, right?

You are right. It is now connected to the right card. Thanks

Gonzalo de Pedro added 6 commits April 30, 2019 09:31
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
@mjcarroll mjcarroll force-pushed the gonzodepedro/rmw_preallocate branch from 5042aa5 to ae8e0ca Compare April 30, 2019 18:40
@mjcarroll mjcarroll mentioned this pull request May 1, 2019
@mjcarroll mjcarroll closed this May 1, 2019
@mjcarroll mjcarroll removed the in review Waiting for review (Kanban column) label May 1, 2019
@wjwwood wjwwood deleted the gonzodepedro/rmw_preallocate branch April 22, 2021 17:58
skyegalaxy pushed a commit to irobot-ros/rcl that referenced this pull request Mar 12, 2025
* Baseline test and force threads to yield.

* Add timer tracking for executor.

* Add locking and test happy-path exit conditions.

* Move logic to multi_threaded_executor

* Address reviewer feedback by reducing scope of set.

* Expand tolerance on testing.

* comment fixup

Otherwise it seemed to me like it would yield twice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants