From 3b70d0f03524ad8ffd38aad5475dc0a545ae88d2 Mon Sep 17 00:00:00 2001 From: nibanovic Date: Tue, 26 Aug 2025 10:53:26 +0200 Subject: [PATCH 1/4] proposal --- design_drafts/refactor_controller_manager.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 design_drafts/refactor_controller_manager.md diff --git a/design_drafts/refactor_controller_manager.md b/design_drafts/refactor_controller_manager.md new file mode 100644 index 0000000..61ccda7 --- /dev/null +++ b/design_drafts/refactor_controller_manager.md @@ -0,0 +1,14 @@ +extract ros2 pub/sub/services into a `ros2_communication` helper files. + +## Motivation: +In a project, we're wrapping `controller_manager` in a custom scheduler task, essentially re-writing the `ros2_control_node` + +- **Conceptual ambiguity**: `ros2_control_node` is not a node, but an executable specific to the OS scheduler responsible for instantiating and running the `controller_manager`, which IS the node. +- **Code Maintainability**: `controller_manager.cpp` file is hitting 4k LoC, mixes core logic and ROS2 communicaiton (services, publishers, subscribers) + +## Proposed changes +We propose a simple refactoring with two main objectives: + +1. **Extract all ROS 2 interfaces** and their callback implementations into a helper file `ros2_communication.cpp/hpp` which current `controller_manager.hpp` would include. + +2. **Rename the current `ros2_control_node`** to `ros2_control_executable` to accurately reflect its function as a system entry point rather than a Node \ No newline at end of file From e9e2b06453caf12dd854b60ba48ac485001bc2a1 Mon Sep 17 00:00:00 2001 From: nibanovic Date: Tue, 26 Aug 2025 10:56:41 +0200 Subject: [PATCH 2/4] food for thought --- design_drafts/refactor_controller_manager.md | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/design_drafts/refactor_controller_manager.md b/design_drafts/refactor_controller_manager.md index 61ccda7..9932250 100644 --- a/design_drafts/refactor_controller_manager.md +++ b/design_drafts/refactor_controller_manager.md @@ -11,4 +11,18 @@ We propose a simple refactoring with two main objectives: 1. **Extract all ROS 2 interfaces** and their callback implementations into a helper file `ros2_communication.cpp/hpp` which current `controller_manager.hpp` would include. -2. **Rename the current `ros2_control_node`** to `ros2_control_executable` to accurately reflect its function as a system entry point rather than a Node \ No newline at end of file +2. **Rename the current `ros2_control_node`** to `ros2_control_executable` to accurately reflect its function as a system entry point rather than a Node + +## Notes: +naming is are for discussion, of course + +#### Food for thought: +Outside of scope for this suggestion, but we could push it a step further and make `ControllerManager` not inherit from `Node` at all, and place all ROS-related stuff into `ControllerManagerNode` class. + +Then, as we do with `ResourceManager`, we'd pass in the necessary node interfaces to it: +- executor (already passed) +- parameters +- logger +- clock + +Then the `ControlNode` would be responsible for ros2 communication, controller manager for controllers and resource manager for hardware. \ No newline at end of file From fc3d8fd3c2fec578b709b7d8ce16a87b195a56db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nikola=20Banovi=C4=87?= Date: Thu, 28 Aug 2025 08:53:43 +0200 Subject: [PATCH 3/4] helper file rename Co-authored-by: Bence Magyar --- design_drafts/refactor_controller_manager.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/design_drafts/refactor_controller_manager.md b/design_drafts/refactor_controller_manager.md index 9932250..4df7d3a 100644 --- a/design_drafts/refactor_controller_manager.md +++ b/design_drafts/refactor_controller_manager.md @@ -9,7 +9,7 @@ In a project, we're wrapping `controller_manager` in a custom scheduler task, es ## Proposed changes We propose a simple refactoring with two main objectives: -1. **Extract all ROS 2 interfaces** and their callback implementations into a helper file `ros2_communication.cpp/hpp` which current `controller_manager.hpp` would include. +1. **Extract all ROS 2 interfaces** and their callback implementations into a helper file `controller_manager_ros.cpp/hpp` which current `controller_manager.hpp` would include. 2. **Rename the current `ros2_control_node`** to `ros2_control_executable` to accurately reflect its function as a system entry point rather than a Node From ced196bb2edd561a3e86415bf5b5f37924a9cab7 Mon Sep 17 00:00:00 2001 From: "Dr. Denis" Date: Wed, 10 Sep 2025 19:54:29 +0200 Subject: [PATCH 4/4] Update design_drafts/refactor_controller_manager.md Co-authored-by: Bence Magyar --- design_drafts/refactor_controller_manager.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/design_drafts/refactor_controller_manager.md b/design_drafts/refactor_controller_manager.md index 4df7d3a..d8e8cc8 100644 --- a/design_drafts/refactor_controller_manager.md +++ b/design_drafts/refactor_controller_manager.md @@ -14,7 +14,7 @@ We propose a simple refactoring with two main objectives: 2. **Rename the current `ros2_control_node`** to `ros2_control_executable` to accurately reflect its function as a system entry point rather than a Node ## Notes: -naming is are for discussion, of course +naming is up for discussion, of course #### Food for thought: Outside of scope for this suggestion, but we could push it a step further and make `ControllerManager` not inherit from `Node` at all, and place all ROS-related stuff into `ControllerManagerNode` class.