From 708903e5dff17aa9deee95472a54a58b0954a8a8 Mon Sep 17 00:00:00 2001 From: Karsten Knese Date: Thu, 25 May 2017 19:52:50 -0700 Subject: [PATCH] Warn unused (#327) * comply with unused warnings * fix flakiness and add test for transitions * mark flaky test * duplicate const char * in State constructor * linters * correct year in license * mark flaky test --- rclcpp_lifecycle/CMakeLists.txt | 9 +++ .../src/lifecycle_node_interface_impl.hpp | 28 ++++--- rclcpp_lifecycle/src/state.cpp | 12 ++- rclcpp_lifecycle/src/transition.cpp | 8 +- rclcpp_lifecycle/test/test_state_wrapper.cpp | 77 +++++++++++++++++++ 5 files changed, 118 insertions(+), 16 deletions(-) create mode 100644 rclcpp_lifecycle/test/test_state_wrapper.cpp diff --git a/rclcpp_lifecycle/CMakeLists.txt b/rclcpp_lifecycle/CMakeLists.txt index 07fa076..9f22fa5 100644 --- a/rclcpp_lifecycle/CMakeLists.txt +++ b/rclcpp_lifecycle/CMakeLists.txt @@ -79,6 +79,15 @@ if(BUILD_TESTING) ) target_link_libraries(test_callback_exceptions ${PROJECT_NAME}) endif() + ament_add_gtest(test_state_wrapper test/test_state_wrapper.cpp) + if(TARGET test_state_wrapper) + target_include_directories(test_state_wrapper PUBLIC + ${rcl_lifecycle_INCLUDE_DIRS} + ${rclcpp_INCLUDE_DIRS} + ${rclcpp_lifecycle_INCLUDE_DIRS} + ) + target_link_libraries(test_state_wrapper ${PROJECT_NAME}) + endif() endif() ament_export_dependencies(rclcpp) diff --git a/rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp b/rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp index 4daac48..34b172a 100644 --- a/rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp +++ b/rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp @@ -63,9 +63,15 @@ public: if (rcl_lifecycle_state_machine_is_initialized(&state_machine_) != RCL_RET_OK) { fprintf(stderr, "%s:%u, FATAL: rcl_state_machine got destroyed externally.\n", __FILE__, __LINE__); + // TODO(karsten1987): Remove else. It should always be possible to call + // fini on state machine, also when not initialized } else { - rcl_lifecycle_state_machine_fini(&state_machine_, - node_base_interface_->get_rcl_node_handle()); + if (rcl_lifecycle_state_machine_fini(&state_machine_, + node_base_interface_->get_rcl_node_handle()) != RCL_RET_OK) + { + fprintf(stderr, "%s:%u, FATAL: failed to destroy rcl_state_machine.\n", + __FILE__, __LINE__); + } } } @@ -304,16 +310,20 @@ public: rcl_lifecycle_ret_t error_resolved = execute_callback(state_machine_.current_state->id, initial_state); if (error_resolved == RCL_RET_OK) { - fprintf(stderr, "Exception handling was successful\n"); + // fprintf(stderr, "Exception handling was successful\n"); // We call cleanup on the error state - rcl_lifecycle_trigger_transition( - &state_machine_, error_resolved, true); - fprintf(stderr, "current state after error callback%s\n", - state_machine_.current_state->label); + if (rcl_lifecycle_trigger_transition(&state_machine_, error_resolved, true) != RCL_RET_OK) { + fprintf(stderr, "Failed to call cleanup on error state\n"); + return false; + } + // fprintf(stderr, "current state after error callback%s\n", + // state_machine_.current_state->label); } else { // We call shutdown on the error state - rcl_lifecycle_trigger_transition( - &state_machine_, error_resolved, true); + if (rcl_lifecycle_trigger_transition(&state_machine_, error_resolved, true) != RCL_RET_OK) { + fprintf(stderr, "Failed to call cleanup on error state\n"); + return false; + } } } // This true holds in both cases where the actual callback diff --git a/rclcpp_lifecycle/src/state.cpp b/rclcpp_lifecycle/src/state.cpp index fae2856..98a9105 100644 --- a/rclcpp_lifecycle/src/state.cpp +++ b/rclcpp_lifecycle/src/state.cpp @@ -15,6 +15,9 @@ #include "rclcpp_lifecycle/state.hpp" #include + +#include +#include #include #include @@ -35,7 +38,8 @@ State::State(uint8_t id, const std::string & label) auto state_handle = new rcl_lifecycle_state_t; state_handle->id = id; - state_handle->label = label.c_str(); + state_handle->label = + rcutils_strndup(label.c_str(), label.size(), rcutils_get_default_allocator()); state_handle_ = state_handle; } @@ -56,12 +60,18 @@ State::~State() uint8_t State::id() const { + if (!state_handle_) { + throw std::runtime_error("Error in state! Internal state_handle is NULL."); + } return state_handle_->id; } std::string State::label() const { + if (!state_handle_) { + throw std::runtime_error("Error in state! Internal state_handle is NULL."); + } return state_handle_->label; } diff --git a/rclcpp_lifecycle/src/transition.cpp b/rclcpp_lifecycle/src/transition.cpp index 7fff732..8ca0333 100644 --- a/rclcpp_lifecycle/src/transition.cpp +++ b/rclcpp_lifecycle/src/transition.cpp @@ -90,17 +90,13 @@ Transition::label() const State Transition::start_state() const { - return State( - transition_handle_->start->id, - transition_handle_->start->label); + return State(transition_handle_->start); } State Transition::goal_state() const { - return State( - transition_handle_->goal->id, - transition_handle_->goal->label); + return State(transition_handle_->goal); } } // namespace rclcpp_lifecycle diff --git a/rclcpp_lifecycle/test/test_state_wrapper.cpp b/rclcpp_lifecycle/test/test_state_wrapper.cpp new file mode 100644 index 0000000..6d0e490 --- /dev/null +++ b/rclcpp_lifecycle/test/test_state_wrapper.cpp @@ -0,0 +1,77 @@ +// Copyright 2017 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include +#include +#include +#include + +#include "rclcpp_lifecycle/lifecycle_node.hpp" + +class TestStateWrapper : public ::testing::Test +{ +protected: + static void SetUpTestCase() + { + } +}; + +TEST_F(TestStateWrapper, wrapper) { + { + rclcpp_lifecycle::State state(1, "my_state"); + EXPECT_EQ(1, state.id()); + EXPECT_FALSE(state.label().empty()); + EXPECT_STREQ("my_state", state.label().c_str()); + } + + { + rcl_lifecycle_state_t lc_state = {"my_c_state", 2, NULL, NULL, 0}; + rclcpp_lifecycle::State c_state(lc_state.id, lc_state.label); + EXPECT_EQ(2, c_state.id()); + EXPECT_FALSE(c_state.label().empty()); + EXPECT_STREQ("my_c_state", c_state.label().c_str()); + } + + { + rcl_lifecycle_state_t lc_state = {"my_c_state", 2, NULL, NULL, 0}; + rclcpp_lifecycle::State c_state(&lc_state); + EXPECT_EQ(2, c_state.id()); + EXPECT_FALSE(c_state.label().empty()); + EXPECT_STREQ("my_c_state", c_state.label().c_str()); + } + + { + rcl_lifecycle_state_t * lc_state = + new rcl_lifecycle_state_t {"my_c_state", 3, NULL, NULL, 0}; + rclcpp_lifecycle::State c_state(lc_state->id, lc_state->label); + EXPECT_EQ(3, c_state.id()); + EXPECT_FALSE(c_state.label().empty()); + EXPECT_STREQ("my_c_state", c_state.label().c_str()); + } + + + // introduces flakiness + // unsupported behavior! + // fails when compiled with memory sanitizer + // { + // rcl_lifecycle_state_t * lc_state + // = new rcl_lifecycle_state_t {"my_c_state", 3, NULL, NULL, 0}; + // rclcpp_lifecycle::State c_state(lc_state); + // delete lc_state; + // lc_state = NULL; + // EXPECT_EQ(3, c_state.id()); + // EXPECT_STREQ("my_c_state", c_state.label().c_str()); + // } +}