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 #1181

Merged
merged 5 commits into from
Nov 27, 2023
Merged

Add few warning compiler options to error #1181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55d0617
add conversion, unused-but-set-variable, and return-type warnings to …
saikishor Nov 24, 2023
0c50c97
add shadow variables to error and their fixes for code compilation
saikishor Nov 24, 2023
c3b25a8
apply the same flags to controller interface package
saikishor Nov 24, 2023
b14a628
apply the same flags and their fixes to hardware_interface package
saikishor Nov 24, 2023
c5c9ec2
apply the same compiler options to the rest of the packages
saikishor Nov 25, 2023
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 @@ -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);
Expand All @@ -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);
}
}
Expand Down Expand Up @@ -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))
Expand All @@ -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(),
Expand Down Expand Up @@ -991,22 +987,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 @@ -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);
Expand All @@ -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);
Expand All @@ -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<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();
bmagyar marked this conversation as resolved.
Show resolved Hide resolved
std::vector<std::string> command_interface_names = {};
if (command_interface_config.type == controller_interface::interface_configuration_type::ALL)
{
Expand Down Expand Up @@ -1767,8 +1763,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
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 @@ -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);
}
Expand Down
20 changes: 11 additions & 9 deletions hardware_interface/test/test_resource_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 :
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