Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor rcl into a functional library #5

Merged
merged 71 commits into from
Dec 18, 2015
Merged

refactor rcl into a functional library #5

merged 71 commits into from
Dec 18, 2015

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Nov 24, 2015

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 the rmw 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:

  • redesign the headers; removing functions for things out scope (services, etc...)
    • I need to go over all the functions and think about alternative return code (not just ok and error)
    • I need to make sure the use of [in], [out], and [inout] is consistent
    • I need to consider const-ness of parameters and return types.
    • I need to address as many of the TODO's in the code as possible (maybe after feedback)
    • I need to add macros for visibility.
    • I need to add warnings about not checking return codes.
  • implement the headers in a shared library which is rmw implementation agnostic
  • implement tests which use the rcl functions:
    • allocator
    • guard_condition
    • node
    • publisher
    • rcl
    • subscription
    • time
    • timer
    • wait
    • Some tests will be deferred to separate pr's.

And then obviously iterate on feedback from review.

I'm using C11 atomics for their semantic value, on unsupported OS's there are other options:

@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Nov 24, 2015
@wjwwood
Copy link
Member Author

wjwwood commented Nov 24, 2015

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.

@tfoote tfoote changed the title refactor rcl into a functioal library refactor rcl into a functional library Nov 24, 2015
@wjwwood
update allocator to properly support realloc
4b55f8e
@wjwwood
more changes (I will squash this commit)
c24a214 
specifically I:
- used more descriptive error codes
- added some of the CMake logic
- More .c files
- Changed some of the documentation to be clearer
@wjwwood wjwwood mentioned this pull request Dec 1, 2015
Merged
jacquelinekay
jacquelinekay reviewed Dec 2, 2015
View reviewed changes
rcl/CMakeLists.txt
@@ -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)
Copy link
Contributor

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?

Copy link
Member Author

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

@jacquelinekay
Copy link
Contributor

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.

@wjwwood
Copy link
Member Author

wjwwood commented Dec 2, 2015

That would probably work fine. The downside of the C approach is that you loose type safety (state object is void *). When the allocator class (which the user defined) is a template argument then the type safety can be preserved. Also you have to consider how to declare STL containers which are templated on an allocator as members of classes which take allocators in the rclcpp interface.

jacquelinekay
jacquelinekay reviewed Dec 2, 2015
View reviewed changes
rcl/include/rcl/node.h
* 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
Copy link
Contributor

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.

Copy link
Member Author

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.

@wjwwood
Copy link
Member Author

wjwwood commented Dec 17, 2015

The Linux server is busy, so I'll start one of those later, but here are OS X and Windows for this branch:

@wjwwood
Copy link
Member Author

wjwwood commented Dec 17, 2015

@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/.

@wjwwood wjwwood mentioned this pull request Dec 17, 2015
Merged
@wjwwood
Copy link
Member Author

wjwwood commented Dec 17, 2015

Starting a Linux CI job: http://ci.ros2.org/job/ci_linux/759/

@wjwwood
Copy link
Member Author

wjwwood commented Dec 18, 2015

I think this is ready to merge after CI:

@dirk-thomas
Copy link
Member

Hopefully fixed compiler warnings:

@dirk-thomas
Copy link
Member

Hopefully fixed OS X missing library:

http://ci.ros2.org/job/ci_osx/661/

@dirk-thomas
Copy link
Member

+1

@wjwwood
Copy link
Member Author

wjwwood commented Dec 18, 2015

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. 🎉

wjwwood added a commit that referenced this pull request Dec 18, 2015
@wjwwood
Merge pull request #5 from ros2/rcl_refactor
083f6ec 
refactor rcl into a functional library
@wjwwood wjwwood merged commit 083f6ec into master Dec 18, 2015
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Dec 18, 2015
@wjwwood wjwwood deleted the rcl_refactor branch December 18, 2015 19:13
@wjwwood wjwwood mentioned this pull request Mar 21, 2017
Merged
BorjaOuterelo referenced this pull request in micro-ROS/rcl Nov 5, 2018
@esteve
Merge pull request #5 from ros2/fix-includes
66d35e9 
Fix includes
@xabxx xabxx mentioned this pull request Apr 29, 2019
Merged
ivanpauno pushed a commit that referenced this pull request Jan 2, 2020
@dirk-thomas
Merge pull request #5 from ros2/use_ament_lint_auto
45b0673 
use ament_lint_auto
mauropasse pushed a commit to mauropasse/rcl that referenced this pull request Oct 20, 2020
Merge pull request ros2#5 from irobot-ros/revert-4-mauro-irobot/add-e…
55d37ca 
…vents-executor

Revert "void return on set_events_executor_callback"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wjwwood wjwwood

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wjwwood @jacquelinekay @dirk-thomas