Wrong place to insert the lock?

[yashi]

The PR #1599 which backports #1451 failed to insert the lock in the right place.

ros2_control/hardware_interface/src/resource_manager.cpp

Lines 687 to 691 in 6a8053e

	const auto hardware_info = hardware_interface::parse_control_resources_from_urdf(urdf);
	if (load_and_initialize_components)
	std::lock_guard<std::recursive_mutex> resource_guard(resources_lock_);
	{
	for (const auto & individual_hardware_info : hardware_info)

This means that when load_and_initialize_components is true the lock is taken, but the rest of the block will be executed regardless.
Or, is it intended fix?

@destogl Please confirm.

[christophfroehlich]

@saikishor ?

[saikishor]

This means that when load_and_initialize_components is true the lock is taken, but the rest of the block will be executed regardless.
Or, is it intended fix?

You are totally right!

[yashi]

@saikishor Just curious, what exactly are you protecting with resources_lock_? I mean, what does resource in the name of the lock refering to? At first I thought read_write_status.failed_hardware_names but it's not only this but others as well?

[saikishor]

Hello!

For Humble, we are trying to protect the read and write methods to access the object before it is constructed completely or configured.

ros2_control/hardware_interface/src/resource_manager.cpp

Lines 1234 to 1280 in 6a8053e

	HardwareReadWriteStatus ResourceManager::read(
	const rclcpp::Time & time, const rclcpp::Duration & period)
	{
	std::lock_guard<std::recursive_mutex> guard(resources_lock_);
	read_write_status.ok = true;
	read_write_status.failed_hardware_names.clear();
	auto read_components = [&](auto & components)
	{
	for (auto & component : components)
	{
	if (component.read(time, period) != return_type::OK)
	{
	read_write_status.ok = false;
	read_write_status.failed_hardware_names.push_back(component.get_name());
	resource_storage_->remove_all_hardware_interfaces_from_available_list(component.get_name());
	}
	}
	};
	read_components(resource_storage_->actuators_);
	read_components(resource_storage_->sensors_);
	read_components(resource_storage_->systems_);
	return read_write_status;
	}
	// CM API: Called in "update"-thread
	HardwareReadWriteStatus ResourceManager::write(
	const rclcpp::Time & time, const rclcpp::Duration & period)
	{
	std::lock_guard<std::recursive_mutex> guard(resources_lock_);
	read_write_status.ok = true;
	read_write_status.failed_hardware_names.clear();
	auto write_components = [&](auto & components)
	{
	for (auto & component : components)
	{
	if (component.write(time, period) != return_type::OK)
	{
	read_write_status.ok = false;
	read_write_status.failed_hardware_names.push_back(component.get_name());
	resource_storage_->remove_all_hardware_interfaces_from_available_list(component.get_name());
	}
	}
	};

That is the reason, we would need this mutex here. For rolling on, it might not be needed as it is handled individually in each component

[christophfroehlich]

fixed with #1960

[saikishor]

The fix in #1960 is merged. I will go ahead and close this