From 55d0617470445a1bcc3fac31a73456fe832a7b55 Mon Sep 17 00:00:00 2001 From: Sai Kishor Kothakota Date: Sat, 25 Nov 2023 00:38:51 +0100 Subject: [PATCH 1/5] add conversion, unused-but-set-variable, and return-type warnings to error --- controller_manager/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller_manager/CMakeLists.txt b/controller_manager/CMakeLists.txt index 77ba823e1f..b775e29905 100644 --- a/controller_manager/CMakeLists.txt +++ b/controller_manager/CMakeLists.txt @@ -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) endif() set(THIS_PACKAGE_INCLUDE_DEPENDS From 0c50c974cb8eb09e1bc7cec217544f675e67df53 Mon Sep 17 00:00:00 2001 From: Sai Kishor Kothakota Date: Sat, 25 Nov 2023 00:49:31 +0100 Subject: [PATCH 2/5] add shadow variables to error and their fixes for code compilation --- controller_manager/CMakeLists.txt | 2 +- controller_manager/src/controller_manager.cpp | 46 +++++++++---------- 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/controller_manager/CMakeLists.txt b/controller_manager/CMakeLists.txt index b775e29905..41a6b18306 100644 --- a/controller_manager/CMakeLists.txt +++ b/controller_manager/CMakeLists.txt @@ -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 -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type) + add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type -Werror=shadow) endif() set(THIS_PACKAGE_INCLUDE_DEPENDS diff --git a/controller_manager/src/controller_manager.cpp b/controller_manager/src/controller_manager.cpp index 5ef8062593..4ffaef24aa 100644 --- a/controller_manager/src/controller_manager.cpp +++ b/controller_manager/src/controller_manager.cpp @@ -442,8 +442,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); @@ -455,8 +453,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); resource_manager_->set_component_state(component, active_state); } } @@ -940,7 +936,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)) @@ -950,14 +946,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(), @@ -991,7 +987,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 active then skip preceding-controllers checks if (!is_controller_active(controller_it->c)) @@ -999,14 +995,14 @@ controller_interface::return_type ControllerManager::switch_controller( 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(), @@ -1087,11 +1083,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); @@ -1100,11 +1096,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); @@ -1113,21 +1109,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 & request_interface_list) + [this](const ControllerSpec ctrl, std::vector & request_interface_list) { - auto command_interface_config = controller.c->command_interface_configuration(); + auto command_interface_config = ctrl.c->command_interface_configuration(); std::vector command_interface_names = {}; if (command_interface_config.type == controller_interface::interface_configuration_type::ALL) { @@ -1767,8 +1763,8 @@ void ControllerManager::reload_controller_libraries_service_cb( loaded_controllers = get_controller_names(); { // lock controllers - std::lock_guard guard(rt_controllers_wrapper_.controllers_lock_); - for (const auto & controller : rt_controllers_wrapper_.get_updated_list(guard)) + std::lock_guard 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)) { From c3b25a82db703a2b661e41f82474feedfb069fcc Mon Sep 17 00:00:00 2001 From: Sai Kishor Kothakota Date: Sat, 25 Nov 2023 00:54:00 +0100 Subject: [PATCH 3/5] apply the same flags to controller interface package --- controller_interface/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller_interface/CMakeLists.txt b/controller_interface/CMakeLists.txt index 88a54d5c7a..d02c422451 100644 --- a/controller_interface/CMakeLists.txt +++ b/controller_interface/CMakeLists.txt @@ -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 From b14a628f52a14da6c25695bbf3af6bd95f729897 Mon Sep 17 00:00:00 2001 From: Sai Kishor Kothakota Date: Sat, 25 Nov 2023 00:54:30 +0100 Subject: [PATCH 4/5] apply the same flags and their fixes to hardware_interface package --- hardware_interface/CMakeLists.txt | 2 +- .../src/mock_components/generic_system.cpp | 8 ++++---- .../test/test_resource_manager.cpp | 20 ++++++++++--------- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/hardware_interface/CMakeLists.txt b/hardware_interface/CMakeLists.txt index 2108bab17c..bcd03a0a16 100644 --- a/hardware_interface/CMakeLists.txt +++ b/hardware_interface/CMakeLists.txt @@ -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 diff --git a/hardware_interface/src/mock_components/generic_system.cpp b/hardware_interface/src/mock_components/generic_system.cpp index 7b62af105b..f4aee6c8a6 100644 --- a/hardware_interface/src/mock_components/generic_system.cpp +++ b/hardware_interface/src/mock_components/generic_system.cpp @@ -580,12 +580,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); } diff --git a/hardware_interface/test/test_resource_manager.cpp b/hardware_interface/test/test_resource_manager.cpp index c80cdbb5bf..8246cc207d 100644 --- a/hardware_interface/test/test_resource_manager.cpp +++ b/hardware_interface/test/test_resource_manager.cpp @@ -247,21 +247,23 @@ TEST_F(ResourceManagerTest, resource_claiming) // Activate components to get all interfaces available activate_components(rm); - const auto key = "joint1/position"; - EXPECT_TRUE(rm.command_interface_is_available(key)); - EXPECT_FALSE(rm.command_interface_is_claimed(key)); - { - auto position_command_interface = rm.claim_command_interface(key); + const auto key = "joint1/position"; EXPECT_TRUE(rm.command_interface_is_available(key)); - EXPECT_TRUE(rm.command_interface_is_claimed(key)); + EXPECT_FALSE(rm.command_interface_is_claimed(key)); + { - EXPECT_ANY_THROW(rm.claim_command_interface(key)); + 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(key)); - EXPECT_FALSE(rm.command_interface_is_claimed(key)); // command interfaces can only be claimed once for (const auto & key : From c5c9ec2ab1ece92cdc190891dd201565d9367a52 Mon Sep 17 00:00:00 2001 From: Sai Kishor Kothakota Date: Sat, 25 Nov 2023 01:00:45 +0100 Subject: [PATCH 5/5] apply the same compiler options to the rest of the packages --- joint_limits/CMakeLists.txt | 2 +- transmission_interface/CMakeLists.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/joint_limits/CMakeLists.txt b/joint_limits/CMakeLists.txt index 82467514a3..a788b6de4f 100644 --- a/joint_limits/CMakeLists.txt +++ b/joint_limits/CMakeLists.txt @@ -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 diff --git a/transmission_interface/CMakeLists.txt b/transmission_interface/CMakeLists.txt index d4674366e9..efd8db1652 100644 --- a/transmission_interface/CMakeLists.txt +++ b/transmission_interface/CMakeLists.txt @@ -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