Skip to content

Commit

Permalink
Add few warning compiler options to error (backport #1181) (#1816)
Browse files Browse the repository at this point in the history 
* Add few warning compiler options to error (#1181)

* add conversion, unused-but-set-variable, and return-type warnings to error
* add shadow variables to error and their fixes for code compilation
* apply the same flags to controller interface package
* apply the same flags and their fixes to hardware_interface package
* apply the same compiler options to the rest of the packages

* Deactivate compiler warning for class_loader

---------

Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
Co-authored-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>
  • Loading branch information
@mergify @saikishor @christophfroehlich
3 people authored Oct 27, 2024
1 parent 24fcfe7 commit 03bf04b
Show file tree
Hide file tree
Showing 8 changed files with 50 additions and 44 deletions.
2 changes: 1 addition & 1 deletion controller_interface/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.5)
project(controller_interface)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "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()

find_package(ament_cmake REQUIRED)
Expand Down
2 changes: 1 addition & 1 deletion controller_manager/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.5)
project(controller_manager)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "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
48 changes: 23 additions & 25 deletions controller_manager/src/controller_manager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -444,15 +444,15 @@ void ControllerManager::init_resource_manager(const std::string & robot_descript
// BEGIN: Keep old functionality on for backwards compatibility
std::vector<std::string> activate_components_on_start = std::vector<std::string>({});
get_parameter("activate_components_on_start", activate_components_on_start);
rclcpp_lifecycle::State active_state(
State::PRIMARY_STATE_ACTIVE, hardware_interface::lifecycle_state_names::ACTIVE);
if (!activate_components_on_start.empty())
{
RCLCPP_WARN(
get_logger(),
"[Deprecated]: 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 @@ -464,8 +464,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 @@ -988,7 +986,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 @@ -998,14 +996,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 @@ -1039,22 +1037,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 @@ -1135,11 +1133,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 @@ -1148,11 +1146,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 @@ -1161,21 +1159,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 @@ -1775,8 +1773,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
2 changes: 1 addition & 1 deletion hardware_interface/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.5)
project(hardware_interface)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "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
Original file line number Diff line number Diff line change
Expand Up @@ -594,12 +594,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
Original file line number Diff line number Diff line change
Expand Up @@ -232,21 +232,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
3 changes: 3 additions & 0 deletions joint_limits/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ project(joint_limits)
if(NOT CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 14)
endif()
if(CMAKE_CXX_COMPILER_ID MATCHES "(GNU|Clang)")
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type -Werror=shadow)
endif()

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra -Wconversion)
Expand Down
5 changes: 4 additions & 1 deletion transmission_interface/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ cmake_minimum_required(VERSION 3.5)
project(transmission_interface)

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
add_compile_options(-Wall -Wextra -Wpedantic -Wconversion)
add_compile_options(-Wall -Wextra -Werror=conversion -Werror=unused-but-set-variable -Werror=return-type
# deactivating this, because of issue in upstream class_loader_imp.hpp
# -Werror=shadow
)
endif()

set(THIS_PACKAGE_INCLUDE_DEPENDS
Expand Down

0 comments on commit 03bf04b

Please sign in to comment.