Add reference timeout parameter to controllers templates by EXPX3 · Pull Request #111 · b-robotized/ros_team_workspace

EXPX3 · 2023-03-14T15:56:22Z

shall include all changes related to reference timeout

EXPX3 · 2023-03-23T09:41:37Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

  return controller_interface::CallbackReturn::SUCCESS;
 }

+void DummyClassName::reference_callback(const std::shared_ptr<ControllerReferenceMsg> msg)


saw that ref_callback is called in different order in standard and chainable controller, hence adjusting it to be consistent with standard controller

in the chainable controller,
this callback is not used at all. I mean, you can call it, but nothing will happen.

sorry for the confusion i should have written "ref_callback is defined in different order in standard and chain-able controller classes, hence adjusting it to be consistent with standard controller"

its trivial change, just changed the order

EXPX3 · 2023-03-23T09:45:06Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

  // Reference Subscriber
  ref_subscriber_ = get_node()->create_subscription<ControllerReferenceMsg>(
-    "~/reference", subscribers_qos,
+    "~/commands", subscribers_qos,


saw that usage of "/reference" is inconsistent in the current master version changed to "/reference" completely in a different PR
#106

EXPX3 · 2023-03-23T10:00:41Z

templates/ros2_control/controller/test_dummy_chainable_controller.cpp

  EXPECT_EQ(*(controller_->control_mode_.readFromRT()), control_mode_type::FAST);
  EXPECT_EQ(joint_command_values_[STATE_MY_ITFS], TEST_DISPLACEMENT);
-  EXPECT_TRUE(std::isnan((*(controller_->input_ref_.readFromRT()))->displacements[0]));
+  EXPECT_EQ((*(controller_->input_ref_.readFromRT()))->displacements[0], TEST_DISPLACEMENT);  


i just adjusted this to work here, but wanted to get confirmation from you. I see that in mecanum we set reference msg to nan when ref_timeout = 0 and for too old msg (age_of_last_command > ref_timeout_)

in this particular test we have testing update fast logic where age_of_last_command <= ref_timeout_

and more over this test will be over written in the PR #106
with a different name with similar logic test

EXPX3 · 2023-03-23T10:05:00Z

templates/ros2_control/controller/test_dummy_chainable_controller.cpp

  ASSERT_EQ(msg.set_point, 0.45);
 }

+// when too old msg is sent expect reference msg reset


added ref_timeout related tests with agreed naming convention

destogl · 2023-03-27T09:44:57Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

  // Reference Subscriber
  ref_subscriber_ = get_node()->create_subscription<ControllerReferenceMsg>(
-    "~/reference", subscribers_qos,
+    "~/commands", subscribers_qos,


Suggested change

"~/commands", subscribers_qos,

"~/reference", subscribers_qos,

ok change ing it to /reference everywhere

destogl · 2023-03-27T09:46:10Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

  return controller_interface::CallbackReturn::SUCCESS;
 }

+void DummyClassName::reference_callback(const std::shared_ptr<ControllerReferenceMsg> msg)


in the chainable controller,
this callback is not used at all. I mean, you can call it, but nothing will happen.

destogl

Please use reference instead of command.
One test is not fully implemented when testing only a single call.
Why is there repetition of the logic in both the update methods? Only method that gets things from callback should have the logic.

destogl · 2023-03-27T09:49:21Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

+  const auto age_of_last_command = get_node()->now() - msg->header.stamp;
+  if (msg->joint_names.size() == params_.joints.size()) {
+    if (ref_timeout_ == rclcpp::Duration::from_seconds(0) || age_of_last_command <= ref_timeout_) {
+      input_ref_.writeFromNonRT(msg);


Should we not have here default value of the header is timestamp is "0"?

you are referring to this handling of case if header.stamp ==0 right?

this is included in the improvements from mecanum PR, you can see the line in the below link, should i also implement this in this ref_timeout PR?

https://github.com/StoglRobotics/ros_team_workspace/pull/106/files#diff-6e6a1012850b4b6040c5339b00ffad68f776d9edb72dbbe3e1d84482d52c4081R189-R197

destogl · 2023-03-27T10:22:06Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

+  auto current_ref = input_ref_.readFromRT();
+  const auto age_of_last_command = get_node()->now() - (*current_ref)->header.stamp;


why is this here? This should be in the update reference from subscribers only. Please adjust

destogl · 2023-03-27T10:22:28Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

+    // send message only if there is no timeout
+    if (age_of_last_command <= ref_timeout_ || ref_timeout_ == rclcpp::Duration::from_seconds(0)) {
+      if (!std::isnan(reference_interfaces_[i])) {
+        if (*(control_mode_.readFromRT()) == control_mode_type::SLOW) {
+          reference_interfaces_[i] /= 2;
+        }
+        command_interfaces_[i].set_value(reference_interfaces_[i]);
+        if (ref_timeout_ == rclcpp::Duration::from_seconds(0)) {
+          reference_interfaces_[i] = std::numeric_limits<double>::quiet_NaN();
+        }
      }
-      command_interfaces_[i].set_value(reference_interfaces_[i]);
-
-      reference_interfaces_[i] = std::numeric_limits<double>::quiet_NaN();
+    } else {
+      command_interfaces_[i].set_value(0.0);
+      (*current_ref)->displacements[i] = std::numeric_limits<double>::quiet_NaN();


Why is the code here repeated?

destogl · 2023-03-27T10:22:46Z

templates/ros2_control/controller/dummy_controller.cpp

  // Reference Subscriber
  ref_subscriber_ = get_node()->create_subscription<ControllerReferenceMsg>(
-    "~/reference", subscribers_qos,
+    "~/commands", subscribers_qos,


Suggested change

"~/commands", subscribers_qos,

"~/reference", subscribers_qos,

destogl · 2023-03-27T10:23:53Z

templates/ros2_control/controller/dummy_controller.cpp

-      command_interfaces_[i].set_value((*current_ref)->displacements[i]);
-
+    } else {
+      command_interfaces_[i].set_value(0.0);


here under is missing resetting of refrence interfaces...

this is standard controller and in my understanding we dont have reference_interfaces in standard controller. Please correct me if i am wrong

destogl · 2023-03-27T10:31:08Z

templates/ros2_control/controller/test_dummy_chainable_controller.cpp

+  EXPECT_FALSE(std::isnan((*(controller_->input_ref_.readFromNonRT()))->duration));
+  EXPECT_EQ((*(controller_->input_ref_.readFromNonRT()))->displacements[0], 0.45);
+  EXPECT_EQ((*(controller_->input_ref_.readFromNonRT()))->velocities[0], 0.0);
+  EXPECT_EQ((*(controller_->input_ref_.readFromNonRT()))->duration, 1.25);


you should call this also a second time to be sure that this works only once - not you can not confirm that.

extended the test here like this but have question,
it works when setting the ref_msg to nan like done here

but instead of individually setting them like that i wanted to just call
reset_controller_reference_msg(*(input_ref_.readFromRT()), state_joints_, get_node());

but this is not working, also made sure that the fun is being called but the msg is not being reset. is the way i am inputing the ref_msg wrong? like this "*(input_ref_.readFromRT())".

…lback_expect_reference_msg_being_used_only_once and extending its relavant code in controllers

mergify · 2023-05-12T07:06:52Z

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

mergify · 2023-08-11T12:34:26Z

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

mergify · 2023-10-09T19:09:38Z

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

mergify · 2023-12-12T09:51:07Z

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

mergify · 2024-02-01T16:40:49Z

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

mergify · 2025-02-07T21:16:15Z

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

GiridharBukka added 9 commits March 14, 2023 12:25

added ref_timeout a parameter

df36359

added ref_timeout related

0cef88d

both ref_time related and improvements from mecanum controller

62ec6f3

correction

8b5c1c9

added tests (new names) onl ref_timeout specific

77e1b53

chainable added missing

27014c9

regular controller added missing

64c8f1d

regular controller added missing tests

e2cc41e

corrections

3f27a6e

destogl requested review from destogl and muritane March 21, 2023 16:43

GiridharBukka added 3 commits March 23, 2023 08:12

all tests pass except load_controller_test

524e334

all tests pass except for load_controller in chaiable controller

508ce0e

correction

40aef82

EXPX3 commented Mar 23, 2023

View reviewed changes

GiridharBukka added 2 commits March 23, 2023 11:08

corrections

72605dc

ran pre.commit

c02dd31

destogl reviewed Mar 27, 2023

View reviewed changes

destogl requested changes Mar 27, 2023

View reviewed changes

GiridharBukka added 3 commits March 29, 2023 18:41

/commands to /reference

0fb82f6

removed duplicate code for age_of_last_command

74b4d11

finishing the incomplete test when_ref_timeout_zero_for_reference_cal…

9692b17

…lback_expect_reference_msg_being_used_only_once and extending its relavant code in controllers

destogl force-pushed the master branch from 4745fce to 2b80753 Compare February 9, 2025 15:21

	"~/commands", subscribers_qos,
	"~/reference", subscribers_qos,

		auto current_ref = input_ref_.readFromRT();
		const auto age_of_last_command = get_node()->now() - (*current_ref)->header.stamp;

Conversation

EXPX3 commented Mar 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

destogl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented May 12, 2023

Uh oh!

mergify bot commented Aug 11, 2023

Uh oh!

mergify bot commented Oct 9, 2023

Uh oh!

mergify bot commented Dec 12, 2023

Uh oh!

mergify bot commented Feb 1, 2024

Uh oh!

mergify bot commented Feb 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

destogl left a comment •

edited

Loading