Add class for thread management of async hw interfaces #981

VX792 · 2023-04-02T19:11:05Z

This PR is about making the async components fit into the framework structurally. I didn't have the opportunity to test the lifecycle changes extensively. I think it should be safe enough, since the only change is that we're destroying the map elements on shutdown, plus storing the components in a separate vector so we don't have to check for is_async everytime in the control loop.

The threads are created and started at the end of the init_resource_manager class, but I'll put it into the activate function to be consistent with the async controllers' solution. Also, now the resource manager owns all async components and their thread wrapper classes

hardware_interface/include/hardware_interface/async_components.hpp

hardware_interface/include/hardware_interface/resource_manager.hpp

hardware_interface/src/resource_manager.cpp

joint_limits_interface/test/joint_limits_urdf_test.cpp

codecov-commenter · 2023-04-05T18:06:22Z

Codecov Report

Merging #981 (38b4ef6) into master (925f5f3) will decrease coverage by 2.14%.
The diff coverage is 35.26%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #981      +/-   ##
==========================================
- Coverage   34.61%   32.48%   -2.14%     
==========================================
  Files          52       92      +40     
  Lines        2981     9647    +6666     
  Branches     1855     6484    +4629     
==========================================
+ Hits         1032     3134    +2102     
- Misses        310      766     +456     
- Partials     1639     5747    +4108

Flag	Coverage Δ
unittests	`32.48% <35.26%> (-2.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
controller_manager/src/controller_manager.cpp	`38.03% <ø> (-1.68%)`	⬇️
controller_manager/src/ros2_control_node.cpp	`0.00% <0.00%> (ø)`
..._interface/include/hardware_interface/actuator.hpp	`100.00% <ø> (ø)`
...ce/include/hardware_interface/async_components.hpp	`0.00% <0.00%> (ø)`
...re_interface/include/hardware_interface/sensor.hpp	`100.00% <ø> (ø)`
...re_interface/include/hardware_interface/system.hpp	`100.00% <ø> (ø)`
hardware_interface/src/resource_manager.cpp	`47.16% <ø> (-6.47%)`	⬇️
hardware_interface/src/sensor.cpp	`50.52% <ø> (ø)`
hardware_interface/src/system.cpp	`55.45% <ø> (ø)`
...rface/test/mock_components/test_generic_system.cpp	`9.02% <ø> (ø)`
... and 68 more

... and 20 files with indirect coverage changes

destogl

Generally fine, few small comments to make simpler interfaces and proposal for the better code-reuse in the follow-up.

destogl · 2023-04-08T19:39:46Z

hardware_interface/include/hardware_interface/async_components.hpp

+    std::is_same<hardware_interface::Actuator, HardwareT>::value ||
+      std::is_same<hardware_interface::System, HardwareT>::value ||
+      std::is_same<hardware_interface::Sensor, HardwareT>::value,


Does this means if someone extends a component type, they will not be able to use this async implementation?

Can we enable this to all types that have mentioned types as base class?

Actually that wouldn't work since all component type classes are final. Maybe I don't understand the use case here.

hardware_interface/include/hardware_interface/async_components.hpp

destogl · 2023-05-17T16:49:55Z

hardware_interface/include/hardware_interface/async_components.hpp

+  explicit AsyncComponentThread(
+    Actuator * component, unsigned int update_rate,
+    rclcpp::node_interfaces::NodeClockInterface::SharedPtr clock_interface)
+  : hardware_component_(component), cm_update_rate_(update_rate), clock_interface_(clock_interface)
+  {
+  }
+
+  explicit AsyncComponentThread(
+    System * component, unsigned int update_rate,
+    rclcpp::node_interfaces::NodeClockInterface::SharedPtr clock_interface)
+  : hardware_component_(component), cm_update_rate_(update_rate), clock_interface_(clock_interface)
+  {
+  }
+
+  explicit AsyncComponentThread(
+    Sensor * component, unsigned int update_rate,
+    rclcpp::node_interfaces::NodeClockInterface::SharedPtr clock_interface)
+  : hardware_component_(component), cm_update_rate_(update_rate), clock_interface_(clock_interface)
+  {
+  }


Can we template this?

the constructor sadly no, but I think I can provide a templated setup function or something similar instead

destogl · 2023-05-17T16:51:09Z

hardware_interface/include/hardware_interface/resource_manager.hpp

@@ -46,7 +49,9 @@ class HARDWARE_INTERFACE_PUBLIC ResourceManager
 {
 public:
  /// Default constructor for the Resource Manager.
-  ResourceManager();
+  ResourceManager(
+    unsigned int update_rate = 100,


Why default value? I would avoid it

Because this is the default update rate of the ControllerManager too, and this way you don't have to provide one when creating a ResourceManager object. Also, this is passed to the ResourceStorage and only used for async components. But it might be misleading for the reader of the code though.

destogl · 2023-05-17T16:55:46Z

hardware_interface/src/resource_manager.cpp

@@ -73,10 +76,16 @@ class ResourceStorage
  static constexpr const char * system_interface_name = "hardware_interface::SystemInterface";

 public:
-  ResourceStorage()
+  // TODO(VX792): Change this when HW ifs get their own update rate,


How do you want to do this? I don't understand. Resource Storage will easier know the update rate then component. Or do you mean that for async HW has this in parameters?

Yes, the async HW itself. Since when HW ifs get their own update rate, then this could be parsed from the config file directly to the HardwareInfo struct of the component.

async components

c0c7dc5

github-actions bot requested review from bmagyar, destogl, erickisos, livanov93, malapatiravi, mcbed and peterdavidfagan April 2, 2023 19:11

VX792 added 2 commits April 2, 2023 21:26

start -> activate

35e3249

fix lifecycle issue when reactivating

b8e0e29

bmagyar requested changes Apr 4, 2023

View reviewed changes

mark external component as async in test

1280079

VX792 force-pushed the async_hwif_lifecycle branch from ff34923 to 1280079 Compare April 5, 2023 17:50

make formatter happy #2

2555c9a

destogl requested changes Apr 10, 2023

View reviewed changes

bmagyar added the iron label Apr 14, 2023

VX792 and others added 5 commits April 15, 2023 22:18

variant instead of template

14895cc

add write fn to sensor

3fb8ccf

format

a7588fa

make cpplint happy too

636810a

fix copyright

7e12436

bmagyar previously approved these changes May 3, 2023

View reviewed changes

bmagyar added 3 commits May 3, 2023 18:34

Merge branch 'master' into async_hwif_lifecycle

1e6555a

Merge branch 'master' into async_hwif_lifecycle

38b4ef6

remove changes to unrelated

2022ec0

bmagyar dismissed their stale review via 2022ec0 May 17, 2023 13:21

Merge branch 'master' into async_hwif_lifecycle

2b1db84

bmagyar approved these changes May 17, 2023

View reviewed changes

bmagyar merged commit 51e6aa9 into ros-controls:master May 17, 2023
1 check passed

destogl reviewed May 22, 2023

View reviewed changes

destogl mentioned this pull request Jul 17, 2023

Good practice to handle multiple controller_managers #1073

Closed

bmagyar mentioned this pull request Jul 27, 2023

Add documentation for async mode of controllers and hardware components #1090

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add class for thread management of async hw interfaces #981

Add class for thread management of async hw interfaces #981

VX792 commented Apr 2, 2023

codecov-commenter commented Apr 5, 2023 •

edited

Loading

destogl left a comment

destogl Apr 8, 2023

VX792 Apr 18, 2023

destogl May 17, 2023

VX792 May 25, 2023

destogl May 17, 2023

VX792 May 25, 2023

destogl May 17, 2023

VX792 May 25, 2023

Add class for thread management of async hw interfaces #981

Add class for thread management of async hw interfaces #981

Conversation

VX792 commented Apr 2, 2023

codecov-commenter commented Apr 5, 2023 • edited Loading

Codecov Report

destogl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 5, 2023 •

edited

Loading