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

Add namespaced controllers #1074

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

hellantos
Copy link

@hellantos hellantos commented Jul 6, 2023

Enabling namespacing for controllers.

Currently, it is not possible to set a controller specific namespace. Controllers simply add the namespace of the controller manager if it has one. Having a bit more flexibility can be pretty handy in larger systems.

This PR enables setting the namespace when loading a controller by name, by the following naming conventions.
It should not have any effect on existing systems.

controller name starts with '/'

  • The controller name passed in when loading is taken as fully qualified name. This means relative to the root namespace.
  • The controller type parameter for the manager is the qualified name with '/' replaced by '.' and without leading '.'

controller name starts without '/'

  • The controller name is taken as relative to the controller_managers namespace.
  • The controller type parameter for the manager is the passed in controller name with '/' replaced by '.'

If the passed in name starts with a '/' it is assumed to be relative to the root namespace. In this case the parameter
is the full name with the initial '/' removed and all other '/' replaced with '.'.

To send us a pull request, please:

  • Fork the repository.
  • Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
  • Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
    • colcon test 100%
    • precommit fails for master branch as well...
  • Commit to your fork using clear commit messages.
  • Send a pull request, answering any default questions in the pull request interface.
  • Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

@saikishor
Copy link
Member

Hello @ipa-cmh,

Can you explain a bit on your current usecase? I would like to understand better which limitations you are planning to address.
Adding this feature might have some affects with the current approach. Can you also please add some tests?.

Thank you,

@hellantos
Copy link
Author

We are working on systems that will have 10 - 100 devices to control. From simple IO devices and single servos to robot systems (the general machines you find in automation) and there will be mutiple devices of the same kind in one machine. We will also have machines with different variations of devices. Conseuqently, we want to standardise the interface to the different devices via namespaces. Currently, we have to options to do that.

a) one controller manager per device and setting a namespace for the controller manager
b) naming conventions for the controllers
c) enable namespacing for controllers in the controller manager

We want to avoid a) because it would lead to hundreds of unnecessary threads and have negatie performance impact.
We want to avoid b) because we would need naming conventions that do not leverage the given namespacing options of the ROS 2 ecosystem and also lead to very large and difficult to understand configuration files. It would also clutter the root namespace.

Therefore, c) seems to be a good solution. We can give controllers namespaces independent of the controller manager. We are able to freely define how many controller managers we run to manage hardware and controllers. To me, it simply makes sense, that the namespace of the controller should be independent of the namespace of the controller manager. I usually would choose the number of control loops (controller managers) depending on the available RT cores. However my application should not need to know on which controller manager the currently used controller runs, but rather which device it belongs to.

This is what an example system would look like:

/controller_manager_1 # runs on core 1 in RT 
/controller_manager_2 # runs on core 2 in RT
/cylinder1/operation_mode_manager
/cylinder1/forward_position_controller # loaded by controller manager 1
/cylinder2/operation_mode_manager
/cylinder2/forward_position_controller # loaded by controller manager 1
/robot1/operation_mode_manager
/robot1/forward_position_controller # loaded by controller manager 2
/robot1/joint_trajectory_controller # loaded by controller manager 2
/robot2/operation_mode_manager
/robot2/forward_position_controller # loaded by controller manager 2
/robot2/joint_trajectory_controller # loaded by controller manager 2
/system_operation_mode_manager
/joint_state_publisher
/robot_state_publisher
...

The operation_mode_manager is basically a more intelligent spawner that spawns and enables controllers for a specific device and also does some monitoring. The system_operation_mode_manager takes care of operation mode of the system (i.e. the mode of the different devices). For multiple devices otf the same type, we can just feed a namespace and the same configuration file to the operation mode manager (which will load the necessary parameters into the controller manager).

It is certainly possible to implement this without namespacing support, but it would just be harder. Having namespacing available just gives everyone more freedom in designing their ROS 2 driver/controller interfaces.

@hellantos
Copy link
Author

I'll add some tests in the next days.

@hellantos
Copy link
Author

Okay, I have taken existing namespace test as base and created the following tests:

  • Normal behavior (controller manager and controller in same namespace)
  • Relative namespace (controller in namespace relative to controller manager), controller name used to load the controller does not have a leading '/' but contains a '/'
  • Absolute namespace (controller in namespace relative to '/'), controller name has a leading '/'.

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.

Generally, I find the idea quite nice, but we have to clean up and clarify some more details in this PR.

@@ -68,6 +68,64 @@ bool controller_name_compare(const controller_manager::ControllerSpec & a, const
return a.info.name == name;
}

// Pass in full_name and the namespace of the manager node, receive
Copy link
Member

Choose a reason for hiding this comment

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

Is this sentence complete?

controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
controller_manager/src/controller_manager.cpp Outdated Show resolved Hide resolved
}

RCLCPP_INFO(
rclcpp::get_logger("split_namespace_and_name"),
Copy link
Member

Choose a reason for hiding this comment

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

Why not use controller manager's logger

Suggested change
rclcpp::get_logger("split_namespace_and_name"),
get_logger(),

@saikishor
Copy link
Member

Therefore, c) seems to be a good solution. We can give controllers namespaces independent of the controller manager. We are able to freely define how many controller managers we run to manage hardware and controllers. To me, it simply makes sense, that the namespace of the controller should be independent of the namespace of the controller manager. I usually would choose the number of control loops (controller managers) depending on the available RT cores. However my application should not need to know on which controller manager the currently used controller runs, but rather which device it belongs to.

Hello @ipa-cmh,

Thank you for such a nice explanation. I think your use case is valid, and I like the idea of the namespacing to be able to solve this. It's so nice to see tests.

std::string controller_namespace_, controller_name_, controller_param_name_;
calculate_controller_naming(
controller.info.name, get_namespace(), controller_namespace_, controller_name_,
controller_param_name_);

However, I see a possible issue that the controller.info.name is always the original name of the controller with namespace, this might create problems for the controller chaining case here:

auto interface_prefix = interface_name.substr(0, split_pos);
following_controller_it = std::find_if(
controllers.begin(), controllers.end(),
std::bind(controller_name_compare, std::placeholders::_1, interface_prefix));

In the above case, the interfaces will have the controller name without the namespace, and then when comparing it will be compared with the total name. This might probably affect the name-spaced controller chaining.

I think it would be nice to add a field called namespace in the ControllerInfo and then be able to set it separately as a parameter as how we do with the type of the controller. It would be much clear I believe.

struct ControllerInfo
{
/// Controller name.
std::string name;
/// Controller type.
std::string type;
/// List of claimed interfaces by the controller.
std::vector<std::string> claimed_interfaces;
};

What do you think?

Best Regards,
Sai

@hellantos
Copy link
Author

Sorry for the long radio silence some family things came in between.

@saikishor
Good that you point that out. I do not really have a full overview over the code. Adding a field in ControllerInfo should be feasible.

@destogl
I will include your changes in the PR as soon as possible.

hellantos and others added 8 commits August 11, 2023 09:55
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Copy link
Contributor

mergify bot commented Nov 14, 2023

This pull request is in conflict. Could you fix it @ipa-cmh?

Copy link
Contributor

mergify bot commented Jan 29, 2024

This pull request is in conflict. Could you fix it @hellantos?

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.

3 participants