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

Expose node options to controller manager #942

Merged
merged 2 commits into from
Feb 20, 2023

Conversation

methylDragon
Copy link
Contributor

@methylDragon methylDragon commented Feb 16, 2023

Description and Scope

This PR exposes an options argument to the ControllerManager constructor to allow for users to configure NodeOptions to be given to the class.

It also exposes the get_cm_node_options() function for downstream use.

The default behavior doesn't change, this just adds a way for users to configure the manager. It shouldn't affect anyone downstream, but enables more configuration, and opens the path to fixing node name duplications down the line.

Context

The ControllerManager as its being used in this package doesn't need this functionality, but since it is a Node, downstream users (or more saliently, myself 😬) might want to instantiate it independently in a separate process.

This might lead to a case where there is more than one Node in the process, which can cause global node name remaps to forcibly change the ControllerManager's node name, leading to duplicate node names.

node_remaps-Page-1 drawio (2)

Design

The fix for this situation is to either:

  • Globally in the CLI/launch, target the node name for remaps;
    node_remaps-Page-2 drawio (2)

Or:

  • Specify another node name remap rule in the NodeOptions passed to a node (causing it to ignore the global rule)

The former case requires users to be made aware of the constraint when writing launchfiles/starting the node up in CLI.
Whereas the latter case avoids that downside, but requires a NodeOptions to get passed to the node.

This change I'm proposing not only benefits my use case (fixing the node name remap issue), but also allows users to further configure the ControllerManager's node options if needed.

The intended use of this change would be for users to either ignore the NodeOptions arg, (i.e. nothing changes)

  auto cm = std::make_shared<controller_manager::ControllerManager>(executor, manager_node_name);

Or create a NodeOptions object using get_cm_node_options() and then configure it.

  auto options = controller_manager::get_cm_node_options();
  options.arguments({
    "--ros-args",
    "--remap", "_target_node_name:__node:=dst_node_name",
    // "--remap", "__node:=dst_node_name",  // (This should also work)
    "--log-level", "info"});
  
  auto cm = std::make_shared<controller_manager::ControllerManager>(executor, "_target_node_name", "some_optional_namespace", options);

There aren't any alternative solutions for this other than telling users downstream to change the way they use CLI/launch, and that's kinda fragile. So it would be good to get this in.

@methylDragon
Copy link
Contributor Author

I'd also like this backported to humble when it's ready!

Signed-off-by: methylDragon <methylDragon@gmail.com>
destogl
destogl previously approved these changes Feb 17, 2023
Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

I agree! Thanks for this extension!

and.... WOW! this is a great explanation! Thanks for this. Would you mind adding at least parts of the explanation into documentation describing your use case of adding controller manager into other nodes. In doc folder are documentation files generated for control.ros.org

@destogl
Copy link
Member

destogl commented Feb 17, 2023

I'd also like this backported to humble when it's ready!

@bmagyar this is a API braking change... How many users would actually feel it?

@methylDragon
Copy link
Contributor Author

methylDragon commented Feb 18, 2023

I agree! Thanks for this extension!

and.... WOW! this is a great explanation! Thanks for this. Would you mind adding at least parts of the explanation into documentation describing your use case of adding controller manager into other nodes. In doc folder are documentation files generated for control.ros.org

I've updated the docs!
I'm not sure if I did it right though...

@methylDragon methylDragon requested review from destogl and removed request for rosterloh, progtologist, bmagyar, livanov93 and bijoua29 February 18, 2023 01:29
Signed-off-by: methylDragon <methylDragon@gmail.com>
Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

Thanks!

@bmagyar
Copy link
Member

bmagyar commented Feb 20, 2023

I don't think many would notice the API change since it's handled w default values for parameters.

The ABI change may pose a small issue but also it's mostly within the same package so I wouldn't worry too much. I think this PR is good for a backport

@destogl destogl enabled auto-merge (squash) February 20, 2023 17:24
@destogl destogl merged commit 3a09fb3 into ros-controls:master Feb 20, 2023
@destogl
Copy link
Member

destogl commented Feb 20, 2023

@Mergifyio backport humble

@mergify
Copy link
Contributor

mergify bot commented Feb 20, 2023

backport humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Feb 20, 2023
Signed-off-by: methylDragon <methylDragon@gmail.com>
(cherry picked from commit 3a09fb3)
@methylDragon methylDragon deleted the ch3/cm-options branch February 20, 2023 19:54
bmagyar pushed a commit that referenced this pull request Feb 21, 2023
Signed-off-by: methylDragon <methylDragon@gmail.com>
(cherry picked from commit 3a09fb3)

Co-authored-by: methylDragon <methylDragon@gmail.com>
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.

None yet

3 participants