Fix controller starting with later load of robot description test #1624

saikishor · 2024-07-19T08:38:28Z

Fixes the failing tests in the ros2_control_ci jobs: ros-controls/ros2_control_ci#98 after the recent merge of #1358. The following tests is failing TestLoadControllerWithoutRobotDescription.controller_starting_with_later_load_of_robot_description. This PR tries to address this as well as some minor adjustments needed to clean the code

Fixes: ros-controls/ros2_control_ci#98
Fixes: #1620

…ization

codecov · 2024-07-19T08:48:43Z

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.60%. Comparing base (1f8ca08) to head (e92305b).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1624      +/-   ##
==========================================
+ Coverage   87.30%   87.60%   +0.29%     
==========================================
  Files         108      108              
  Lines        9866     9918      +52     
  Branches      890      889       -1     
==========================================
+ Hits         8614     8689      +75     
+ Misses        929      912      -17     
+ Partials      323      317       -6

Flag	Coverage Δ
unittests	`87.60% <95.45%> (+0.29%)`	⬆️

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

Files	Coverage Δ
controller_manager/test/test_spawner_unspawner.cpp	`99.20% <100.00%> (+0.19%)`	⬆️
controller_manager/src/controller_manager.cpp	`76.84% <80.00%> (+0.98%)`	⬆️

... and 1 file with indirect coverage changes

bmagyar · 2024-07-19T08:51:29Z

controller_manager/test/test_spawner_unspawner.cpp

@@ -327,6 +327,61 @@ TEST_F(TestLoadController, unload_on_kill)
  ASSERT_EQ(cm_->get_loaded_controllers().size(), 0ul);
 }

+TEST_F(TestLoadController, spawner_test_fallback_controllers)


Why was this moved?

I moved it for better log tracking as the newly added test is placed in between an original test.

ros2_control/controller_manager/test/test_spawner_unspawner.cpp

Lines 330 to 407 in 9d1aff9

class TestLoadControllerWithoutRobotDescription

: public ControllerManagerFixture<controller_manager::ControllerManager>

{

public:

TestLoadControllerWithoutRobotDescription()

: ControllerManagerFixture<controller_manager::ControllerManager>("")

{

}

void SetUp() override

{

ControllerManagerFixture::SetUp();

update_timer_ = cm_->create_wall_timer(

std::chrono::milliseconds(10),

[&]()

{

cm_->read(time_, PERIOD);

cm_->update(time_, PERIOD);

cm_->write(time_, PERIOD);

});

update_executor_ =

std::make_shared<rclcpp::executors::MultiThreadedExecutor>(rclcpp::ExecutorOptions(), 2);

update_executor_->add_node(cm_);

update_executor_spin_future_ =

std::async(std::launch::async, [this]() -> void { update_executor_->spin(); });

// This sleep is needed to prevent a too fast test from ending before the

// executor has began to spin, which causes it to hang

std::this_thread::sleep_for(50ms);

}

void TearDown() override { update_executor_->cancel(); }

rclcpp::TimerBase::SharedPtr robot_description_sending_timer_;

protected:

rclcpp::TimerBase::SharedPtr update_timer_;

// Using a MultiThreadedExecutor so we can call update on a separate thread from service callbacks

std::shared_ptr<rclcpp::Executor> update_executor_;

std::future<void> update_executor_spin_future_;

};

TEST_F(TestLoadControllerWithoutRobotDescription, when_no_robot_description_spawner_times_out)

{

cm_->set_parameter(rclcpp::Parameter("ctrl_1.type", test_controller::TEST_CONTROLLER_CLASS_NAME));

ControllerManagerRunner cm_runner(this);

EXPECT_EQ(call_spawner("ctrl_1 -c test_controller_manager"), 256)

<< "could not spawn controller because not robot description and not services for controller "

"manager are active";

}

TEST_F(

TestLoadControllerWithoutRobotDescription,

controller_starting_with_later_load_of_robot_description)

{

cm_->set_parameter(rclcpp::Parameter("ctrl_1.type", test_controller::TEST_CONTROLLER_CLASS_NAME));

// Delay sending robot description

robot_description_sending_timer_ = cm_->create_wall_timer(

std::chrono::milliseconds(2500), [&]() { pass_robot_description_to_cm_and_rm(); });

ControllerManagerRunner cm_runner(this);

EXPECT_EQ(call_spawner("ctrl_1 -c test_controller_manager"), 0)

<< "could not activate control because not robot description";

ASSERT_EQ(cm_->get_loaded_controllers().size(), 1ul);

{

auto ctrl_1 = cm_->get_loaded_controllers()[0];

ASSERT_EQ(ctrl_1.info.name, "ctrl_1");

ASSERT_EQ(ctrl_1.info.type, test_controller::TEST_CONTROLLER_CLASS_NAME);

ASSERT_EQ(ctrl_1.c->get_state().id(), lifecycle_msgs::msg::State::PRIMARY_STATE_ACTIVE);

}

}

bmagyar · 2024-07-19T08:52:17Z

controller_manager/src/controller_manager.cpp

-  diagnostics_updater_.setHardwareID("ros2_control");
-  diagnostics_updater_.add(
-    "Controllers Activity", this, &ControllerManager::controller_activity_diagnostic_callback);
+  init_controller_manager();


Did we have all this duplicated code here the whole time?

Well only from the #1358. So it is not so long

bmagyar · 2024-07-19T08:53:03Z

controller_manager/src/controller_manager.cpp

@@ -305,6 +295,10 @@ void ControllerManager::robot_description_callback(const std_msgs::msg::String &
  init_resource_manager(robot_description_);
  if (is_resource_manager_initialized())
  {
+    RCLCPP_INFO(


Isn't this a debug print?

yes! it is. I think it is very helpful to know somehow via log that it initialized properly and we are going to start the services. If you want, I can remove it as well. I'm fine with anything

bmagyar

A few small questions, sorry for the vagueness, can't do proper checks from the phone

saikishor · 2024-07-19T08:56:02Z

A few small questions, sorry for the vagueness, can't do proper checks from the phone

Done. Thanks for reviewing it quickly

christophfroehlich

Thanks for dealing with it. May also fix #1620

saikishor · 2024-07-19T12:45:19Z

Thanks for dealing with it. May also fix #1620

Yes! It does fix #1620

bmagyar

Thanks for the great work!

* [ResourceManager] Make destructor virtual for use in derived classes (#1607) * Fix typos in test_resource_manager.cpp (#1609) * [CM] Remove support for the description parameter and use only `robot_description` topic (#1358) --------- Co-authored-by: Felix Exner <exner@fzi.de> Co-authored-by: Christoph Froehlich <christoph.froehlich@ait.ac.at> * move critical variables to the private context (#1623) * Fix controller starting with later load of robot description test (#1624) * Fix the duplicated restart of the controller_manager services initialization * Scope the ControllerManagerRunner to avoid malloc and other test issues * reorder the tests for consistent log at the end * Remove noqa (#1626) * Unused header cleanup (#1627) * Create debugging.rst (#1625) --------- Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com> Co-authored-by: Christoph Froehlich <christoph.froehlich@ait.ac.at> * Update changelogs * 4.14.0 * add missing rclcpp logging include for Humble compatibility build (#1635) * [CM] Remove deprecated spawner args (#1639) * Add a pytest launch file to test ros2_control_node (#1636) * Fix rst markup (#1642) * Fix rqt_cm paragraph * Fix indent * CM: Add missing includes (#1641) * [RM] Add `get_hardware_info` method to the Hardware Components (#1643) * Fix the namespaced controller_manager spawner + added tests (#1640) * Bump version of pre-commit hooks (#1649) Co-authored-by: christophfroehlich <3367244+christophfroehlich@users.noreply.github.com> * Add missing include for executors (#1653) * Update changelogs * 4.15.0 * refactor SwitchParams to fix the incosistencies in the spawner tests (#1638) * Modify test with missing CM to have a timeout * Catch exception when CM services are not found And print the error and exit in the application * Exit with code 1 on unreached CM --------- Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com> Co-authored-by: Parker Drouillard <parker@pepcorp.ca> Co-authored-by: Dr. Denis <denis@stoglrobotics.de> Co-authored-by: Christoph Froehlich <christoph.froehlich@ait.ac.at> Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com> Co-authored-by: Henry Moore <henry.moore@picknik.ai> Co-authored-by: Lennart Nachtigall <firesurfer127@gmail.com> Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com> Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: christophfroehlich <3367244+christophfroehlich@users.noreply.github.com>

saikishor added 3 commits July 19, 2024 10:22

Fix the duplicated restart of the controller_manager services initial…

4b5ce9b

…ization

Scope the ControllerManagerRunner to avoid malloc and other test issues

b976964

reorder the tests for consistent log at the end

e92305b

saikishor changed the title ~~Fix/controller starting with later load of robot description/test~~ Fix controller starting with later load of robot description test Jul 19, 2024

github-actions bot requested review from aprotyas, bmagyar, christophfroehlich, destogl, fmauch, moriarty and olivier-stasse July 19, 2024 08:38

bmagyar reviewed Jul 19, 2024

View reviewed changes

bmagyar requested changes Jul 19, 2024

View reviewed changes

christophfroehlich approved these changes Jul 19, 2024

View reviewed changes

bmagyar approved these changes Jul 19, 2024

View reviewed changes

bmagyar merged commit 0792a35 into ros-controls:master Jul 19, 2024
18 of 20 checks passed

This was referenced Jul 19, 2024

CI build job rolling/testing failed! #1622

Closed

CI build job humble/testing failed! ros-controls/ros2_control_ci#99

Closed

saikishor deleted the fix/controller_starting_with_later_load_of_robot_description/test branch August 17, 2024 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix controller starting with later load of robot description test #1624

Fix controller starting with later load of robot description test #1624

saikishor commented Jul 19, 2024 •

edited

Loading

codecov bot commented Jul 19, 2024

bmagyar Jul 19, 2024

saikishor Jul 19, 2024

bmagyar Jul 19, 2024 •

edited

Loading

saikishor Jul 19, 2024

bmagyar Jul 19, 2024

saikishor Jul 19, 2024

bmagyar left a comment

saikishor commented Jul 19, 2024

christophfroehlich left a comment

saikishor commented Jul 19, 2024

bmagyar left a comment

	class TestLoadControllerWithoutRobotDescription
	: public ControllerManagerFixture<controller_manager::ControllerManager>
	{
	public:
	TestLoadControllerWithoutRobotDescription()
	: ControllerManagerFixture<controller_manager::ControllerManager>("")
	{
	}

	void SetUp() override
	{
	ControllerManagerFixture::SetUp();

	update_timer_ = cm_->create_wall_timer(
	std::chrono::milliseconds(10),
	[&]()
	{
	cm_->read(time_, PERIOD);
	cm_->update(time_, PERIOD);
	cm_->write(time_, PERIOD);
	});

	update_executor_ =
	std::make_shared<rclcpp::executors::MultiThreadedExecutor>(rclcpp::ExecutorOptions(), 2);

	update_executor_->add_node(cm_);
	update_executor_spin_future_ =
	std::async(std::launch::async, [this]() -> void { update_executor_->spin(); });

	// This sleep is needed to prevent a too fast test from ending before the
	// executor has began to spin, which causes it to hang
	std::this_thread::sleep_for(50ms);
	}

	void TearDown() override { update_executor_->cancel(); }

	rclcpp::TimerBase::SharedPtr robot_description_sending_timer_;

	protected:
	rclcpp::TimerBase::SharedPtr update_timer_;

	// Using a MultiThreadedExecutor so we can call update on a separate thread from service callbacks
	std::shared_ptr<rclcpp::Executor> update_executor_;
	std::future<void> update_executor_spin_future_;
	};

	TEST_F(TestLoadControllerWithoutRobotDescription, when_no_robot_description_spawner_times_out)
	{
	cm_->set_parameter(rclcpp::Parameter("ctrl_1.type", test_controller::TEST_CONTROLLER_CLASS_NAME));

	ControllerManagerRunner cm_runner(this);
	EXPECT_EQ(call_spawner("ctrl_1 -c test_controller_manager"), 256)
	<< "could not spawn controller because not robot description and not services for controller "
	"manager are active";
	}

	TEST_F(
	TestLoadControllerWithoutRobotDescription,
	controller_starting_with_later_load_of_robot_description)
	{
	cm_->set_parameter(rclcpp::Parameter("ctrl_1.type", test_controller::TEST_CONTROLLER_CLASS_NAME));

	// Delay sending robot description
	robot_description_sending_timer_ = cm_->create_wall_timer(
	std::chrono::milliseconds(2500), [&]() { pass_robot_description_to_cm_and_rm(); });

	ControllerManagerRunner cm_runner(this);
	EXPECT_EQ(call_spawner("ctrl_1 -c test_controller_manager"), 0)
	<< "could not activate control because not robot description";

	ASSERT_EQ(cm_->get_loaded_controllers().size(), 1ul);
	{
	auto ctrl_1 = cm_->get_loaded_controllers()[0];
	ASSERT_EQ(ctrl_1.info.name, "ctrl_1");
	ASSERT_EQ(ctrl_1.info.type, test_controller::TEST_CONTROLLER_CLASS_NAME);
	ASSERT_EQ(ctrl_1.c->get_state().id(), lifecycle_msgs::msg::State::PRIMARY_STATE_ACTIVE);
	}
	}

Fix controller starting with later load of robot description test #1624

Fix controller starting with later load of robot description test #1624

Conversation

saikishor commented Jul 19, 2024 • edited Loading

codecov bot commented Jul 19, 2024

Codecov Report

bmagyar Jul 19, 2024

Choose a reason for hiding this comment

saikishor Jul 19, 2024

Choose a reason for hiding this comment

bmagyar Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

saikishor Jul 19, 2024

Choose a reason for hiding this comment

bmagyar Jul 19, 2024

Choose a reason for hiding this comment

saikishor Jul 19, 2024

Choose a reason for hiding this comment

bmagyar left a comment

Choose a reason for hiding this comment

saikishor commented Jul 19, 2024

christophfroehlich left a comment

Choose a reason for hiding this comment

saikishor commented Jul 19, 2024

bmagyar left a comment

Choose a reason for hiding this comment

saikishor commented Jul 19, 2024 •

edited

Loading

bmagyar Jul 19, 2024 •

edited

Loading