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.

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

Dynamically remap topic names #1072

Open
fmrico opened this issue May 23, 2023 · 8 comments
Open

Dynamically remap topic names #1072

fmrico opened this issue May 23, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@fmrico
Copy link

fmrico commented May 23, 2023

Feature request

Dynamically remap topic names.

Feature description

Currently, topic remapping can only be done when a node is started, and, as far as I understand, it takes effect when the publishers/subscribers are created. Later, it is not possible to change the topic name.

Remapping topics once the publisher/subscriber is created would be useful. It would even be interesting if each node had a service to do this so that there could be requests for external remapping.

This functionality makes it possible to analyze a running computing graph and fix disconnections between topics without stopping and restarting the nodes with new remappings.

I am interested in working on this, but I need guidance.

Implementation considerations

Doing this at the client library level (rclcpp, for example) could be relatively simple, but in rcl it would be more general.

A service interface should be created in rcl_interfaces.

@fujitatomoya
Copy link
Collaborator

IMO this is good feature so that user can reconnect the topic to the different resource at runtime.

@fujitatomoya fujitatomoya added the enhancement New feature or request label May 24, 2023
@clalancette
Copy link
Contributor

IMO this is good feature so that user can reconnect the topic to the different resource at runtime.

We do have a small section about dynamic remapping in the design document at http://design.ros2.org/articles/static_remapping.html#static-versus-dynamic-remapping . I can't exactly remember now, but I think at the time we didn't implement it to keep the scope small. @sloretz maybe you can comment here?

Regardless, I do think it is a fine feature to have. If we are going to do it, I'd much prefer to have it at the rcl layer, so that both rclcpp and rclpy can more easily get support for it.

I'm not totally convinced that we want to expose a service to allow this remapping to happen. A service is just one way that a user might want to make this change, and services are expensive in terms of discovery. I could imagine a user changing this via a parameter, a service, a topic, or even in reaction to an external event. So my suggestion here is to start off just by defining the API, and worrying about a service or something like that as a totally separate step.

@fmrico
Copy link
Author

fmrico commented May 28, 2023

Currently, remapping is established either through global arguments (from argc/argv) or through local arguments (node options) and takes value when creating the resource. In rcl_publisher_init, for example, a call to rcl_node_resolve_name calls to rcl_resolve_name with local and global arguments.

Let's center on publishers in this conversation but take into account other resources. One approach could be as follows: The interface to create a publisher could be very similar to rcl_publisher_init, but taking a pre-existing publisher and the new topic name (we can get the current topic name and options from the existing publisher) :

rcl_ret_t
rcl_publisher_remap(
  rcl_publisher_t * publisher,
  const rcl_node_t * node,
  const char * remapped_topic_name,
)

In this function, to avoid changes in rmw, we could replace the current one by one with the remapped name:

  1. Finish current publisher via rcl_publisher_fini
  2. Add a dynamic remap to the node (see below)
  3. Call rcl_publisher_init

What do you think?

Something that worries me is what happens with a subscriber who has received messages that still need to be processed or a client who is waiting for a response. If we destroy the subscriber or the client, messages and responses may be lost.

Add a dynamic remap to the node

To extend rcl_resolve_name with a new parameter dynamic_args:

static
rcl_ret_t
rcl_resolve_name(
  const rcl_arguments_t * local_args,
  const rcl_arguments_t * global_args,
  const rcl_arguments_t * dynamic_args,
  const char * input_topic_name,
  const char * node_name,
[...]

rcl_node_resolve_name would get the dynamic arguments from the node, extending rcl_node_impl_s:

struct rcl_node_impl_s
{
  rcl_node_options_t options;
  rcl_arguments_t dynamic_arguments;
  rmw_node_t * rmw_node_handle;
  rcl_guard_condition_t * graph_guard_condition;
  const char * logger_name;
  const char * fq_name;
};

Many functions that takes local_arguments and global_arguments to resolve a name, need to add a dynamic_arguments, virtually all functions in remap.h: rcl_remap_topic_name, rcl_remap_service_name, rcl_remap_node_name, andrcl_remap_node_namespace. Maybe we also need to add functions to add create rcl_remap_t from C strings, because now they are not parsed from argc/argv.

A good aspect is that it opens the option of having more dynamic arguments, beyond remapping resources.

@fujitatomoya
Copy link
Collaborator

@fmrico thanks for sharing idea.

In this function, to avoid changes in rmw, we could replace the current one by one with the remapped name

just checking, so once the topic is remapped, the existing publisher will no longer exist? that will be replaced with new topic name? right?
i think this is what remap means, if existing publisher will stay alive, that sounds more like copy.

Something that worries me is what happens with a subscriber who has received messages that still need to be processed or a client who is waiting for a response.

if we want it to be processed or continue, this feature remap is more like copy the topic? (not finilizing the original publisher.)
I think that is useful too, because the use case for changing topic name at runtime is to connect the topic with different endpoints after they are already launched.
but the above is not remap in ROS? i think original topic should not exist if it is remapped to the another topic name, that aligns to the current static remapping option.

user application should be able to know that there is no publishers anymore since it is remapped?
i think we can detect that matched event that there is no publisher (publisher count is zero) anymore on the topic, but not sure if it can know that is because of remapping.

So my suggestion here is to start off just by defining the API, and worrying about a service or something like that as a totally separate step.

totally agree, currently node has each dedicated service or parameter to manage the specific configuration or options.
this is or will not be cost effective especially for discovery, but it tends to land in this way since service, parameter interfaces are good for user experience.
i believe that it is worth to reconsider those options, external services or parameters provided by node could be optimized.
one way could be, we can have common node service interface to manage options or configuration via single entity with capabilities.

@jrutgeer
Copy link

jrutgeer commented Nov 1, 2023

Just as a FYI:

Someone asked for this functionality on the Robotics stack exchange.

I could imagine a user changing this via a parameter,

I proposed as a solution to install a post_set_parameter_callback that resets the current subscriber and creates a new one for the changed topic name.

auto post_set_parameter_callback =
  [this](const std::vector<rclcpp::Parameter> & parameters) {
    for (const auto & param : parameters) {
      if (param.get_name() == "topic_name") {
        std::string new_topic = this->get_parameter("topic_name").as_string();
        // Delete current subscriber
        // Create new subscriber
      }
    }
  }

post_set_parameters_callback_handle_ = this->add_post_set_parameters_callback(post_set_parameter_callback);

I did not test this, but I assume it works.

@jrutgeer
Copy link

jrutgeer commented Nov 2, 2023

This issue is probably relevant as well:

ros2/rclcpp#2146

@fujitatomoya
Copy link
Collaborator

@jrutgeer

I proposed as a solution to install a post_set_parameter_callback that resets the current subscriber and creates a new one for the changed topic name.

it should work but i would use ParameterEventHandler::add_parameter_event_callback instead. with add_post_set_parameters_callback publisher needs to be responsible to set the parameters on the subscription node each, this could be problem if there are many subscriptions. instead, publisher remaps the topic name and set the specific parameter that indicates that topic name has been changed, then taking advantage of /parameter_event topic and ParameterEventHandler::add_parameter_event_callback will deliver the event to the all concerned subscriptions. i believe that this would be better and scalable for distributed application.

@jrutgeer
Copy link

jrutgeer commented Nov 3, 2023

@fujitatomoya

I was rather thinking about the use case where you'd remap either the publisher, or the subscriber, but not both. E.g. robot moves from 'warehouse_1' to 'warehouse_2' and hence needs to remap its listeners to the publishers of 'warehouse_2' instead of those of 'warehouse_1'. But indeed, add_parameter_event_callback is also a possibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants