-
Notifications
You must be signed in to change notification settings - Fork 162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor rcl into a functional library #5
Conversation
I'm going to try and push the implementation of the headers and the corresponding tests tonight, but they are currently broken, but I figured people could go ahead and start looking at the functions and their documentation to make comments while I work on that. |
it changes with each successful call to rcl_init
specifically I: - used more descriptive error codes - added some of the CMake logic - More .c files - Changed some of the documentation to be clearer
@@ -3,11 +3,36 @@ cmake_minimum_required(VERSION 2.8.3) | |||
project(rcl) | |||
|
|||
find_package(ament_cmake REQUIRED) | |||
find_package(rcl_interfaces REQUIRED) | |||
find_package(rmw REQUIRED) | |||
find_package(rosidl_generator_cpp REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this will become rosidl_generator_cpp once Dirk finishes C typesupport?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I can do it now. 483ddfd
Looking at the Allocator interface here, I'm considering of changing the user allocator interface in rclcpp to work in a similar way. That is, the user provides allocate and deallocate functions that could be bound to some state, then rclcpp constructs an allocator compliant with the C++ stdlib requirements behind the scenes for when it needs to pass the allocator to stdlib structures. Then rclcpp could pass the allocate and deallocate functions from the user directly to rcl. |
That would probably work fine. The downside of the C approach is that you loose type safety (state object is |
* node to use the ROS domain ID set in the ROS_DOMAIN_ID environment | ||
* variable, or on some systems 0 if the environment variable is not set. | ||
* | ||
* \TODO(wjwwood): Should we put a limit on the ROS_DOMAIN_ID value, that way |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the limits on domain ID enforced by Opensplice and Connext? If this is something that varies by middleware implementation, I don't think it needs to be enforced at the rcl layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question. I don't know what the limits are for the DDS vendors or even if the specification deals with that detail. This remains an open question for me.
The Linux server is busy, so I'll start one of those later, but here are OS X and Windows for this branch: |
@tfoote @dirk-thomas I have the same problem on my OS X job, e.g. http://ci.ros2.org/job/ci_osx/636/testReport/junit/projectroot/test/test_memory_tools/. |
Starting a Linux CI job: http://ci.ros2.org/job/ci_linux/759/ |
I think this is ready to merge after CI: |
Hopefully fixed compiler warnings: |
Hopefully fixed OS X missing library: |
+1 |
Thanks for the fix ups @dirk-thomas, I'm going to merge and I'll address any new failures in a follow up pr if needed. 🎉 |
refactor rcl into a functional library
…vents-executor Revert "void return on set_events_executor_callback"
Currently
rcl
is just an idea and not being used. This pull request will refactor it into a working library which can be used by client libraries and which is a super set of thermw
functionality. The scope of this pull request is just for initialization, nodes, publishers, subscriptions, guard conditions, and waiting. This will allow prototyping of the Python API implementation while we work on services (client and server), execution logic, and parameters separately in a follow up pull request.This is a work in progress still, I need to:
[in]
,[out]
, and[inout]
is consistentconst
-ness of parameters and return types.which is rmw implementation agnosticpublishersubscriptiontimerwaitAnd then obviously iterate on feedback from review.
I'm using C11 atomics for their semantic value, on unsupported OS's there are other options: