joint state controller with resource manager #109

Karsten1987 · 2020-11-13T08:34:08Z

The way to run it:

terminal1:

ros2 run controller_manager ros2_control_node --ros-args -p update_rate:=10 -p "robot_description:=`cat install/test_robot_hardware/share/test_robot_hardware/test_robot_hardware.urdf`" -p "joint_state_controller.type:=joint_state_controller/JointStateController"

terminal2:

ros2 control load joint_state_controller

joint_state_controller/src/joint_state_controller.cpp

destogl

Looks good. Just some changes regarding standard constants and shared test data.

destogl · 2020-11-25T13:05:58Z

joint_state_controller/include/joint_state_controller/joint_state_controller.hpp

@@ -68,6 +65,7 @@ class JointStateController : public controller_interface::ControllerInterface

 protected:
  std::vector<std::string> joint_names_;
+  std::unordered_map<std::string, std::unordered_map<std::string, double>> name_if_value_mapping_;


Can you a short comment about this map?

Added comments, I've also grouped the joint-state-related stuff and the dynamic joint state-reltaed stuff together

destogl · 2020-11-25T13:09:05Z

joint_state_controller/src/joint_state_controller.cpp

+const auto kPositionName = "position";
+const auto kVelocityName = "velocity";
+const auto kEffortName = "effort";


I propose to use names from: hardware_interface/include/hardware_interface/types/hardware_interface_type_values.hpp

Suggested change

const auto kPositionName = "position";

const auto kVelocityName = "velocity";

const auto kEffortName = "effort";

at the time of first doing this, these values were not available. Replaced with this constants now

destogl · 2020-11-25T13:09:51Z

joint_state_controller/src/joint_state_controller.cpp

 #include <vector>

-#include "hardware_interface/joint_handle.hpp"
-#include "hardware_interface/robot_hardware.hpp"
 #include "hardware_interface/types/hardware_interface_return_values.hpp"


Header for standardized names...

Suggested change

#include "hardware_interface/types/hardware_interface_return_values.hpp"

#include "hardware_interface/types/hardware_interface_return_values.hpp"

#include "hardware_interface/types/hardware_interface_type_values.hpp"

thanks, added

destogl · 2020-11-25T13:14:51Z

joint_state_controller/src/joint_state_controller.cpp

+  // the rest will be ignored for this message
+  for (const auto name_ifv : name_if_value_mapping_) {
+    const auto & interfaces_and_values = name_ifv.second;
+    if (has_any_key(interfaces_and_values, {kPositionName, kVelocityName, kEffortName})) {


Suggested change

if (has_any_key(interfaces_and_values, {kPositionName, kVelocityName, kEffortName})) {

if (has_any_key(interfaces_and_values, {hardware_interface::HW_IF_POSITION,

hardware_interface::HW_IF_VELOCITY,

hardware_interface::HW_IF_EFFORT})) {

Maybe not the best formatting possible...

when working around linters and code formatters, there's only so much you can discuss about code style :D

removing the hardware_interface namespace by a couple of usings at the top of the file kept this a one-liner

Yeah, I mean my proposal was not the best formating possible.

destogl · 2020-11-25T13:22:55Z

joint_state_controller/src/joint_state_controller.cpp

+      get_value(name_if_value_mapping_, joint_names_[i], kPositionName);
+    joint_state_msg_.velocity[i] =
+      get_value(name_if_value_mapping_, joint_names_[i], kVelocityName);
+    joint_state_msg_.effort[i] = get_value(name_if_value_mapping_, joint_names_[i], kEffortName);


Suggested change

get_value(name_if_value_mapping_, joint_names_[i], kPositionName);

joint_state_msg_.velocity[i] =

get_value(name_if_value_mapping_, joint_names_[i], kVelocityName);

joint_state_msg_.effort[i] = get_value(name_if_value_mapping_, joint_names_[i], kEffortName);

get_value(name_if_value_mapping_, joint_names_[i], hardware_interface::HW_IF_POSITION);

joint_state_msg_.velocity[i] =

get_value(name_if_value_mapping_, joint_names_[i], hardware_interface::HW_IF_VELOCITY);

joint_state_msg_.effort[i] = get_value(name_if_value_mapping_, joint_names_[i], hardware_interface::HW_IF_EFFORT);

destogl · 2020-11-25T13:30:48Z

joint_state_controller/test/test_common.hpp

+
+namespace joint_state_controller_testing
+{
+constexpr auto urdf =


I would propose to use descriptions.hpp header for this as proposed in #99 and ros-controls/ros2_control#247.
We should just move the file in test_robot_hardware as @Karsten1987 proposed.

#113 created follow-up issue

destogl · 2020-11-25T13:36:11Z

joint_state_controller/test/test_joint_state_controller.cpp

-  ASSERT_EQ(dynamic_joint_state_msg.interface_values[0].values[0], 1.1);
-  ASSERT_EQ(dynamic_joint_state_msg.interface_values[1].values[0], 2.1);
-  ASSERT_EQ(dynamic_joint_state_msg.interface_values[2].values[0], 3.1);
+  const auto INTERFACE_NAMES = {"position", "velocity", "effort"};


Suggested change

const auto INTERFACE_NAMES = {"position", "velocity", "effort"};

const auto INTERFACE_NAMES = {hardware_interface::HW_IF_POSITION,

hardware_interface::HW_IF_VELOCITY,

hardware_interface::HW_IF_EFFORT};

there was another one of these, done that too

destogl · 2020-11-25T13:37:25Z

joint_state_controller/test/test_joint_state_controller.cpp


+#include "hardware_interface/loaned_state_interface.hpp"
 #include "hardware_interface/types/hardware_interface_return_values.hpp"


Suggested change

#include "hardware_interface/types/hardware_interface_return_values.hpp"

#include "hardware_interface/types/hardware_interface_return_values.hpp"

#include "hardware_interface/types/hardware_interface_type_values.hpp"

v-lopez · 2020-11-25T17:19:38Z

Looks good to me after @destogl suggestions are addressed.

bmagyar · 2020-11-30T07:56:04Z

ok guys, I've addressed all your comments, please verify/resolve discussions (I'm not going to resolve your discussion points)

v-lopez · 2020-11-30T08:00:33Z

I agree with how you addressed @destogl review, could be merged by me.

bmagyar

This wasn't my PR originally so I can still "review it"! After doing some major changes to it, the controller works and testing is mostly appropriate.

Patting myself on the shoulder in approval

destogl

Looks good!

EDIT: My approvals are not counted anyway :)

* Forward command controller almost using resource manager * Make forward command controller fully work and restore testing * Use node namespace for topics * Review fixes and linters * Move CallbackReturn using to header Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>

* Use gmock * Add Bence as maintainer * Test invalid handles * Add handle validation to RobotHardware * Test proper handles and double registering handles * Test name and blanket handle getters * Test named handle getters * Refactor registering second handles in tests, add check for getting second name * Check that returned pointers are not null * Update ros2ci workflow versions

Karsten1987 mentioned this pull request Nov 13, 2020

Review how to provide controller parameters ros-controls/ros2_control#168

Closed

destogl reviewed Nov 15, 2020

View reviewed changes

joint_state_controller/src/joint_state_controller.cpp Outdated Show resolved Hide resolved

This was referenced Nov 16, 2020

Can we rename joint_state_controller #110

Closed

Use resource manager ros-controls/ros2_control#236

Merged

bmagyar marked this pull request as ready for review November 25, 2020 12:09

destogl requested changes Nov 25, 2020

View reviewed changes

bmagyar approved these changes Nov 30, 2020

View reviewed changes

destogl approved these changes Nov 30, 2020

View reviewed changes

bmagyar force-pushed the use_resource_manager branch 2 times, most recently from e600d36 to 6197903 Compare December 1, 2020 21:58

joint_state_controller using resource manager

2ec6f9c

bmagyar force-pushed the use_resource_manager branch from 6197903 to 2ec6f9c Compare December 1, 2020 22:06

bmagyar and others added 3 commits December 1, 2020 22:15

Touch up CI version for ros-action

5da4994

only joint state ctrl

9d78fe0

bmagyar merged commit 30612b1 into master Dec 1, 2020

bmagyar deleted the use_resource_manager branch December 1, 2020 22:51

This was referenced Dec 1, 2020

Move position/velocity/effort_controllers to CM #120

Closed

Move diff_drive_controller to ResourceManager #121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

joint state controller with resource manager #109

joint state controller with resource manager #109

Karsten1987 commented Nov 13, 2020

destogl left a comment

destogl Nov 25, 2020

bmagyar Nov 30, 2020

destogl Nov 25, 2020

bmagyar Nov 30, 2020

destogl Nov 25, 2020

bmagyar Nov 30, 2020

destogl Nov 25, 2020

bmagyar Nov 30, 2020

destogl Nov 30, 2020

destogl Nov 25, 2020

bmagyar Nov 30, 2020

destogl Nov 25, 2020

bmagyar Nov 30, 2020

destogl Nov 25, 2020

bmagyar Nov 30, 2020

bmagyar Nov 30, 2020

destogl Nov 25, 2020

bmagyar Nov 30, 2020

v-lopez commented Nov 25, 2020

bmagyar commented Nov 30, 2020

v-lopez commented Nov 30, 2020

bmagyar left a comment

destogl left a comment •

edited

	const auto kPositionName = "position";
	const auto kVelocityName = "velocity";
	const auto kEffortName = "effort";

	#include "hardware_interface/types/hardware_interface_return_values.hpp"
	#include "hardware_interface/types/hardware_interface_return_values.hpp"
	#include "hardware_interface/types/hardware_interface_type_values.hpp"

-    if (has_any_key(interfaces_and_values, {kPositionName, kVelocityName, kEffortName})) {
+    if (has_any_key(interfaces_and_values, {hardware_interface::HW_IF_POSITION,
+                                                                     hardware_interface::HW_IF_VELOCITY,
+                                                                     hardware_interface::HW_IF_EFFORT})) {


		#include "hardware_interface/loaned_state_interface.hpp"
		#include "hardware_interface/types/hardware_interface_return_values.hpp"

joint state controller with resource manager #109

joint state controller with resource manager #109

Conversation

Karsten1987 commented Nov 13, 2020

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

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

Choose a reason for hiding this comment

v-lopez commented Nov 25, 2020

bmagyar commented Nov 30, 2020

v-lopez commented Nov 30, 2020

bmagyar left a comment

Choose a reason for hiding this comment

destogl left a comment • edited

Choose a reason for hiding this comment

destogl left a comment •

edited