Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

refactor to support init options and context #308

Merged
merged 4 commits into from
Nov 30, 2018
Merged

refactor to support init options and context #308

merged 4 commits into from
Nov 30, 2018

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Nov 19, 2018

Connects to ros2/rmw#154

@serge-nikulin FYI this is an example of what the disruption to an existing rmw implementation will look like for the init options and context changes.

@wjwwood wjwwood added enhancement New feature or request in review Waiting for review (Kanban column) labels Nov 19, 2018
@wjwwood wjwwood self-assigned this Nov 19, 2018
@serge-nikulin
Copy link
Contributor

Ack.

I will also join the chorus "do we really need rmw_init_options_t ?"

Please give me an example when a context is not enough.

@wjwwood
Copy link
Member Author

wjwwood commented Nov 19, 2018

rmw_init_options_t is given as input to init, and context is an output, so if you want to influence the initialization then you need the init options.

@serge-nikulin
Copy link
Contributor

OK, I like it.

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM, just some questions about pushing common implementation to shared functions.

rmw_connext_cpp/src/rmw_guard_condition.cpp Show resolved Hide resolved
rmw_connext_cpp/src/rmw_init.cpp Show resolved Hide resolved
context->implementation_identifier,
rti_connext_identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
RCUTILS_CHECK_ARGUMENT_FOR_NULL(context->impl, RMW_RET_INVALID_ARGUMENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like rmw_init() sets context->impl to nullptr, why is this expecting it to be non-null?

context->implementation_identifier,
rti_connext_dynamic_identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
RCUTILS_CHECK_ARGUMENT_FOR_NULL(context->impl, RMW_RET_INVALID_ARGUMENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment here, context->impl is set to nullptr in rmw_init()

Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood merged commit 6c22e0a into master Nov 30, 2018
@wjwwood wjwwood deleted the refactor_init branch November 30, 2018 05:33
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Nov 30, 2018
@wjwwood wjwwood mentioned this pull request Nov 30, 2018
@serge-nikulin
Copy link
Contributor

Connects to ros2/rmw#154

FYI this is an example of what the disruption to an existing rmw implementation will look like for the init options and context changes.

@wjwwood, do you have an example of a user-side customization?

@wjwwood
Copy link
Member Author

wjwwood commented Dec 17, 2018

@serge-nikulin no, but I think it would look something like this:

#include <rclcpp/rclcpp.hpp>
// note this next include breaks portability of ROS code
#include <rmw_connext_cpp/add_options_to_context.hpp>

auto context = std::make_shared<rclcpp::Context>()
context->init(...);

rmw_connext_cpp::add_option_to_context(context, "do_a_thing", true);

auto node = std::make_shared<rclcpp::Node>("my_node", "", context);

And the implementation of the add_option_to_context() function could looks something like this:

template<typename ContextT>
void
rmw_connext_cpp::add_option_to_context(
  std::shared_ptr<ContextT> context,
  const std::string & option_name,
  bool option_value)
{
  // see: http://docs.ros2.org/crystal/api/rclcpp/classrclcpp_1_1Context.html#a28596bd16aad20e057160f8a66d4697f
  auto rcl_context_shared_ptr = context->get_rcl_context();
  // some how assert that rcl_context_shared_ptr is not nullptr

  rcl_context_t * rcl_context = rcl_context_shared_ptr.get();
  // this function doesn't exist yet :'(
  rmw_context_t * rmw_context = rcl_context_get_rmw_context(rcl_context);
  // some how assert that rmw_context is not NULL

  // now you can cast the `void *` impl pointer to the connext cpp specific version
  // and then mutate it how ever you like
  rmw_context_impl_t * rmw_context_impl = (rmw_context_impl_t *)rmw_context->impl;
  // some how insert the option into the connext impl for later use
}

And so currently you cannot 🙃. I'll add the rcl_context_get_rmw_context function to the work related to passing the context to the wait set. Sorry about that.

@serge-nikulin
Copy link
Contributor

@wjwwood,

And so currently you cannot

Do you have an issue to address it?

@wjwwood
Copy link
Member Author

wjwwood commented Jan 10, 2019

@serge-nikulin see: ros2/rcl#372

dabonnie pushed a commit to aws-ros-dev/rmw_connext that referenced this pull request Apr 3, 2019
* refactor to support init options and context

Signed-off-by: William Woodall <william@osrfoundation.org>

* style

Signed-off-by: William Woodall <william@osrfoundation.org>

* add missing rmw_shutdown

* fix incorrect check for null

Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants