Conversation
destogl
requested review from
Karsten1987 and
bmagyar
and removed request for
bmagyar
|
Check out the follow-up issues |
controller_manager/include/controller_manager/controller_manager.hpp
Outdated
Show resolved
Hide resolved
controller_manager/include/controller_manager/controller_manager.hpp
Outdated
Show resolved
Hide resolved
| auto cm = std::make_shared<controller_manager::ControllerManager>( | ||
| // TODO(anyone): remove robot_hw when ResourceManager is added | ||
| // since RobotHW is not a plugin we had to take some type of robot | ||
| std::make_shared<test_robot_hardware::TestRobotHardware>(), | ||
| executor, | ||
| manager_node_name); |
There was a problem hiding this comment.
what is that? Why would you hardcode TestRobotHardware here?
There was a problem hiding this comment.
Because we need to have something here otherwise it will not work. As soon is there the ResourceManager there is no issue any more.
The RobotHardware should be loaded dynamically to work properly.
| timer = cm->create_wall_timer( | ||
| std::chrono::milliseconds(update_time_ms), | ||
| std::bind(&controller_manager::ControllerManager::update, cm.get()), | ||
| cm->deterministic_callback_group_); |
There was a problem hiding this comment.
this looks a bit like a dependency inversion to me. If the executor is handled outside of the controller manager in its own node, then the associated callback groups should be handled externally as well.
With this approach - essentially having the controller manager's callback groups exposed publicly - every node can temper with the execution model of the controller manager and provoke a undesired behavior given the assumptions within the CM.
There was a problem hiding this comment.
Realistically, I see two executors being used here. The first one spawns the controller and handles the update (as well as the read and write) loop with a given frequency. That executor can then be assigned a higher realtime priority by the operating system to guarantee a deterministic behavior.
We have to introduce a second node which handles the async callbacks to low prio calls such as list controllers etc.
That workaround is needed as we're targeting foxy and we can't leverage the changes introduced in here: ros2/rclcpp#1218
There was a problem hiding this comment.
Please submit a PR with a suggested alternative
There was a problem hiding this comment.
@Karsten1987 I agree with you. I added here are @v-lopez and @gavanderhoorn commented that they want to temper with the execution of update function.
Or should they then just call specialize ControllerManager in a new class?
Codecov Report
@@ Coverage Diff @@
## master #216 +/- ##
==========================================
- Coverage 31.41% 31.23% -0.19%
==========================================
Files 38 39 +1
Lines 2489 2513 +24
Branches 1638 1646 +8
==========================================
+ Hits 782 785 +3
- Misses 251 265 +14
- Partials 1456 1463 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Karsten1987
force-pushed
the
ros2_control_node
branch
from
baaf393 to
62a4d7c
Compare
Signed-off-by: Karsten Knese <Karsten1987@users.noreply.github.com>
|
I tried your PR locally and the node without having any controllers loaded nor doing any work eats up a complete core with more than 100% CPU. |
|
@Karsten1987: I can confirm this. There is a lot of resource usage... I have between 60% and 95% CPU usage Why is that? update calls? |
|
@Karsten1987. it seems you merged some of the code here already in #236. Should I close this now? |
|
Added into #236. closing... |
…ces on specific joints (ros-controls#216) * Joint state broadcaster support for specific joint_names and interface_names. * Extend documentation. Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
6 participants
rework of #147.
Changes:
controller_manager- callback groups for services and update loopcontroller_interface- enable undeclared parameters for nowros2_control_node- default node - currently is usingTestRobotHardwareto initialize something untilResourceManageris compleated