Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add few warning compiler options to error (backport #1181) #1817

Closed
wants to merge 2 commits into from
Closed

Add few warning compiler options to error (backport #1181) #1817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3abbcc8
Add few warning compiler options to error (#1181)
saikishor Nov 27, 2023
0e399b3
Fix backport #1308
christophfroehlich Oct 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion controller_interface/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.16)
project(controller_interface LANGUAGES CXX)

if(CMAKE_CXX_COMPILER_ID MATCHES "(GNU|Clang)")
add_compile_options(-Wall -Wextra -Wconversion)
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type -Werror=shadow)
endif()

set(THIS_PACKAGE_INCLUDE_DEPENDS
Expand Down
2 changes: 1 addition & 1 deletion controller_manager/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.16)
project(controller_manager LANGUAGES CXX)

if(CMAKE_CXX_COMPILER_ID MATCHES "(GNU|Clang)")
add_compile_options(-Wall -Wextra -Wconversion)
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type -Werror=shadow)
endif()

set(THIS_PACKAGE_INCLUDE_DEPENDS
Expand Down
46 changes: 21 additions & 25 deletions controller_manager/src/controller_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,8 +457,6 @@ void ControllerManager::init_resource_manager(const std::string & robot_descript
"Parameter 'activate_components_on_start' is deprecated. "
"Components are activated per default. Don't use this parameters in combination with the new "
"'hardware_components_initial_state' parameter structure.");
rclcpp_lifecycle::State active_state(
State::PRIMARY_STATE_ACTIVE, hardware_interface::lifecycle_state_names::ACTIVE);
for (const auto & component : activate_components_on_start)
{
resource_manager_->set_component_state(component, active_state);
Expand All @@ -470,8 +468,6 @@ void ControllerManager::init_resource_manager(const std::string & robot_descript
// activate all other components
for (const auto & [component, state] : components_to_activate)
{
rclcpp_lifecycle::State active_state(
State::PRIMARY_STATE_ACTIVE, hardware_interface::lifecycle_state_names::ACTIVE);
if (
resource_manager_->set_component_state(component, active_state) ==
hardware_interface::return_type::ERROR)
Expand Down Expand Up @@ -1008,7 +1004,7 @@ controller_interface::return_type ControllerManager::switch_controller(
auto controller_it = std::find_if(
controllers.begin(), controllers.end(),
std::bind(controller_name_compare, std::placeholders::_1, *ctrl_it));
controller_interface::return_type ret = controller_interface::return_type::OK;
controller_interface::return_type status = controller_interface::return_type::OK;

// if controller is not inactive then do not do any following-controllers checks
if (!is_controller_inactive(controller_it->c))
Expand All @@ -1018,14 +1014,14 @@ controller_interface::return_type ControllerManager::switch_controller(
"Controller with name '%s' is not inactive so its following "
"controllers do not have to be checked, because it cannot be activated.",
controller_it->info.name.c_str());
ret = controller_interface::return_type::ERROR;
status = controller_interface::return_type::ERROR;
}
else
{
ret = check_following_controllers_for_activate(controllers, strictness, controller_it);
status = check_following_controllers_for_activate(controllers, strictness, controller_it);
}

if (ret != controller_interface::return_type::OK)
if (status != controller_interface::return_type::OK)
{
RCLCPP_WARN(
get_logger(),
Expand Down Expand Up @@ -1059,22 +1055,22 @@ controller_interface::return_type ControllerManager::switch_controller(
auto controller_it = std::find_if(
controllers.begin(), controllers.end(),
std::bind(controller_name_compare, std::placeholders::_1, *ctrl_it));
controller_interface::return_type ret = controller_interface::return_type::OK;
controller_interface::return_type status = controller_interface::return_type::OK;

// if controller is not active then skip preceding-controllers checks
if (!is_controller_active(controller_it->c))
{
RCLCPP_WARN(
get_logger(), "Controller with name '%s' can not be deactivated since it is not active.",
controller_it->info.name.c_str());
ret = controller_interface::return_type::ERROR;
status = controller_interface::return_type::ERROR;
}
else
{
ret = check_preceeding_controllers_for_deactivate(controllers, strictness, controller_it);
status = check_preceeding_controllers_for_deactivate(controllers, strictness, controller_it);
}

if (ret != controller_interface::return_type::OK)
if (status != controller_interface::return_type::OK)
{
RCLCPP_WARN(
get_logger(),
Expand Down Expand Up @@ -1155,11 +1151,11 @@ controller_interface::return_type ControllerManager::switch_controller(
// check for double stop
if (!is_active && in_deactivate_list)
{
auto ret = handle_conflict(
auto conflict_status = handle_conflict(
"Could not deactivate controller '" + controller.info.name + "' since it is not active");
if (ret != controller_interface::return_type::OK)
if (conflict_status != controller_interface::return_type::OK)
{
return ret;
return conflict_status;
}
in_deactivate_list = false;
deactivate_request_.erase(deactivate_list_it);
Expand All @@ -1168,11 +1164,11 @@ controller_interface::return_type ControllerManager::switch_controller(
// check for doubled activation
if (is_active && !in_deactivate_list && in_activate_list)
{
auto ret = handle_conflict(
auto conflict_status = handle_conflict(
"Could not activate controller '" + controller.info.name + "' since it is already active");
if (ret != controller_interface::return_type::OK)
if (conflict_status != controller_interface::return_type::OK)
{
return ret;
return conflict_status;
}
in_activate_list = false;
activate_request_.erase(activate_list_it);
Expand All @@ -1181,21 +1177,21 @@ controller_interface::return_type ControllerManager::switch_controller(
// check for illegal activation of an unconfigured/finalized controller
if (!is_inactive && !in_deactivate_list && in_activate_list)
{
auto ret = handle_conflict(
auto conflict_status = handle_conflict(
"Could not activate controller '" + controller.info.name +
"' since it is not in inactive state");
if (ret != controller_interface::return_type::OK)
if (conflict_status != controller_interface::return_type::OK)
{
return ret;
return conflict_status;
}
in_activate_list = false;
activate_request_.erase(activate_list_it);
}

const auto extract_interfaces_for_controller =
[this](const ControllerSpec controller, std::vector<std::string> & request_interface_list)
[this](const ControllerSpec ctrl, std::vector<std::string> & request_interface_list)
{
auto command_interface_config = controller.c->command_interface_configuration();
auto command_interface_config = ctrl.c->command_interface_configuration();
std::vector<std::string> command_interface_names = {};
if (command_interface_config.type == controller_interface::interface_configuration_type::ALL)
{
Expand Down Expand Up @@ -1847,8 +1843,8 @@ void ControllerManager::reload_controller_libraries_service_cb(
loaded_controllers = get_controller_names();
{
// lock controllers
std::lock_guard<std::recursive_mutex> guard(rt_controllers_wrapper_.controllers_lock_);
for (const auto & controller : rt_controllers_wrapper_.get_updated_list(guard))
std::lock_guard<std::recursive_mutex> ctrl_guard(rt_controllers_wrapper_.controllers_lock_);
for (const auto & controller : rt_controllers_wrapper_.get_updated_list(ctrl_guard))
{
if (is_controller_active(*controller.c))
{
Expand Down
8 changes: 5 additions & 3 deletions controller_manager/test/test_controller_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,16 +446,18 @@ TEST_P(TestControllerUpdateRates, check_the_controller_update_rate)
time += rclcpp::Duration::from_seconds(0.01);
if (update_counter % cm_update_rate == 0)
{
const auto no_of_secs_passed = update_counter / cm_update_rate;
const double no_of_secs_passed = static_cast<double>(update_counter) / cm_update_rate;
// NOTE: here EXPECT_NEAR is used because it is observed that in the first iteration of whole
// cycle of cm_update_rate counts, there is one count missing, but in rest of the 9 cycles it
// is clearly tracking, so adding 1 here won't affect the final count.
// For instance, a controller with update rate 37 Hz, seems to have 36 in the first update
// cycle and then on accumulating 37 on every other update cycle so at the end of the 10
// cycles it will have 369 instead of 370.
EXPECT_NEAR(
EXPECT_THAT(
test_controller->internal_counter - initial_counter,
(controller_update_rate * no_of_secs_passed), 1);
testing::AnyOf(
testing::Eq(controller_update_rate * no_of_secs_passed),
testing::Eq((controller_update_rate * no_of_secs_passed) - 1)));
Comment on lines +459 to +460
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christophfroehlich I'm not sure if this should be with -1 or +1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied that from here, why do we expect a different value here? and yes, the tests fail..

}
}
}
Expand Down
2 changes: 1 addition & 1 deletion hardware_interface/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.16)
project(hardware_interface LANGUAGES CXX)

if(CMAKE_CXX_COMPILER_ID MATCHES "(GNU|Clang)")
add_compile_options(-Wall -Wextra -Wconversion)
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type -Werror=shadow)
endif()

set(THIS_PACKAGE_INCLUDE_DEPENDS
Expand Down
8 changes: 4 additions & 4 deletions hardware_interface/src/mock_components/generic_system.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,12 +596,12 @@ return_type GenericSystem::read(const rclcpp::Time & /*time*/, const rclcpp::Dur
}
else
{
for (size_t j = 0; j < joint_states_[POSITION_INTERFACE_INDEX].size(); ++j)
for (size_t k = 0; k < joint_states_[POSITION_INTERFACE_INDEX].size(); ++k)
{
if (!std::isnan(joint_commands_[POSITION_INTERFACE_INDEX][j]))
if (!std::isnan(joint_commands_[POSITION_INTERFACE_INDEX][k]))
{
joint_states_[POSITION_INTERFACE_INDEX][j] = // apply offset to positions only
joint_commands_[POSITION_INTERFACE_INDEX][j] +
joint_states_[POSITION_INTERFACE_INDEX][k] = // apply offset to positions only
joint_commands_[POSITION_INTERFACE_INDEX][k] +
(custom_interface_with_following_offset_.empty() ? position_state_following_offset_
: 0.0);
}
Expand Down
24 changes: 13 additions & 11 deletions hardware_interface_testing/test/test_resource_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,21 +233,23 @@ TEST_F(ResourceManagerTest, resource_claiming)
// Activate components to get all interfaces available
activate_components(rm);

const auto command_interface = "joint1/position";
EXPECT_TRUE(rm.command_interface_is_available(command_interface));
EXPECT_FALSE(rm.command_interface_is_claimed(command_interface));

{
auto position_command_interface = rm.claim_command_interface(command_interface);
EXPECT_TRUE(rm.command_interface_is_available(command_interface));
EXPECT_TRUE(rm.command_interface_is_claimed(command_interface));
const auto key = "joint1/position";
EXPECT_TRUE(rm.command_interface_is_available(key));
EXPECT_FALSE(rm.command_interface_is_claimed(key));

{
EXPECT_ANY_THROW(rm.claim_command_interface(command_interface));
EXPECT_TRUE(rm.command_interface_is_available(command_interface));
auto position_command_interface = rm.claim_command_interface(key);
EXPECT_TRUE(rm.command_interface_is_available(key));
EXPECT_TRUE(rm.command_interface_is_claimed(key));
{
EXPECT_ANY_THROW(rm.claim_command_interface(key));
EXPECT_TRUE(rm.command_interface_is_available(key));
}
}
EXPECT_TRUE(rm.command_interface_is_available(key));
EXPECT_FALSE(rm.command_interface_is_claimed(key));
}
EXPECT_TRUE(rm.command_interface_is_available(command_interface));
EXPECT_FALSE(rm.command_interface_is_claimed(command_interface));

// command interfaces can only be claimed once
for (const auto & interface_key :
Expand Down
2 changes: 1 addition & 1 deletion joint_limits/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.16)
project(joint_limits LANGUAGES CXX)

if(CMAKE_CXX_COMPILER_ID MATCHES "(GNU|Clang)")
add_compile_options(-Wall -Wextra -Wconversion)
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type -Werror=shadow)
endif()

set(THIS_PACKAGE_INCLUDE_DEPENDS
Expand Down
2 changes: 1 addition & 1 deletion transmission_interface/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.16)
project(transmission_interface LANGUAGES CXX)

if(CMAKE_CXX_COMPILER_ID MATCHES "(GNU|Clang)")
add_compile_options(-Wall -Wextra -Wpedantic -Wconversion)
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type -Werror=shadow)
endif()

set(THIS_PACKAGE_INCLUDE_DEPENDS
Expand Down
Loading