From f511834439097e1dc92e50a9ba22a11545246f5d Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Fri, 9 Nov 2018 16:04:13 -0800 Subject: [PATCH 1/3] [rcl_action] Add function for checking if goal can be transitioned to CANCELING Add unit tests for the new function rcl_action_goal_handle_is_cancelable(), as well as rcl_action_goal_handle_is_active(). --- rcl_action/include/rcl_action/goal_handle.h | 24 +++++- rcl_action/src/rcl_action/goal_handle.c | 12 +++ .../test/rcl_action/test_goal_handle.cpp | 79 ++++++++++--------- 3 files changed, 76 insertions(+), 39 deletions(-) diff --git a/rcl_action/include/rcl_action/goal_handle.h b/rcl_action/include/rcl_action/goal_handle.h index f5032d1d1..d991c27dd 100644 --- a/rcl_action/include/rcl_action/goal_handle.h +++ b/rcl_action/include/rcl_action/goal_handle.h @@ -202,7 +202,7 @@ rcl_action_goal_handle_get_status( * Lock-Free | Yes * * \param[in] goal_handle struct containing the goal and metadata - * \return `true` if a goal is in one of the following states: ACCEPTED, EXECUTING, or CANCELING, or + * \return `true` if the goal is in one of the following states: ACCEPTED, EXECUTING, or CANCELING, or * \return `false` otherwise, also * \return `false` if the goal handle pointer is invalid */ @@ -211,6 +211,28 @@ RCL_WARN_UNUSED bool rcl_action_goal_handle_is_active(const rcl_action_goal_handle_t * goal_handle); +/// Check if a goal can be transitioned to CANCELING in its current state. +/** + * This is a non-blocking call. + * + *
+ * Attribute | Adherence + * ------------------ | ------------- + * Allocates Memory | No + * Thread-Safe | No + * Uses Atomics | No + * Lock-Free | Yes + * + * \param[in] goal_handle struct containing the goal and metadata + * \return `true` if the goal can be transitioned to CANCELING from its current state, or + * \return `false` otherwise, also + * \return `false` if the goal handle pointer is invalid +*/ +RCL_ACTION_PUBLIC +RCL_WARN_UNUSED +bool +rcl_action_goal_handle_is_cancelable(const rcl_action_goal_handle_t * goal_handle); + /// Check if a rcl_action_goal_handle_t is valid. /** * This is a non-blocking call. diff --git a/rcl_action/src/rcl_action/goal_handle.c b/rcl_action/src/rcl_action/goal_handle.c index dce06dc36..1048b1def 100644 --- a/rcl_action/src/rcl_action/goal_handle.c +++ b/rcl_action/src/rcl_action/goal_handle.c @@ -137,6 +137,18 @@ rcl_action_goal_handle_is_active(const rcl_action_goal_handle_t * goal_handle) } } +bool +rcl_action_goal_handle_is_cancelable(const rcl_action_goal_handle_t * goal_handle) +{ + if (!rcl_action_goal_handle_is_valid(goal_handle)) { + return false; // error message is set + } + // Check if the state machine reports a cancel event is valid + rcl_action_goal_state_t state = rcl_action_transition_goal_state( + goal_handle->impl->state, GOAL_EVENT_CANCEL); + return GOAL_STATE_CANCELING == state; +} + bool rcl_action_goal_handle_is_valid(const rcl_action_goal_handle_t * goal_handle) { diff --git a/rcl_action/test/rcl_action/test_goal_handle.cpp b/rcl_action/test/rcl_action/test_goal_handle.cpp index 6700eaa54..7481a099b 100644 --- a/rcl_action/test/rcl_action/test_goal_handle.cpp +++ b/rcl_action/test/rcl_action/test_goal_handle.cpp @@ -15,7 +15,7 @@ #include #include -#include +#include #include #include "rcl_action/goal_handle.h" @@ -156,8 +156,9 @@ TEST(TestGoalHandle, test_goal_handle_update_state_invalid) rcl_reset_error(); } -using EventStatePair = std::pair; -using StateTransitionSequence = std::vector; +using EventStateActiveCancelableTuple = + std::tuple; +using StateTransitionSequence = std::vector; const std::vector event_strs = { "EXECUTE", "CANCEL", "SET_SUCCEEDED", "SET_ABORTED", "SET_CANCELED"}; @@ -169,8 +170,8 @@ class TestGoalHandleStateTransitionSequence const testing::TestParamInfo & info) { std::stringstream result; - for (const EventStatePair & event_state : info.param) { - result << "_" << event_strs[event_state.first]; + for (const EventStateActiveCancelableTuple & event_state : info.param) { + result << "_" << event_strs[std::get<0>(event_state)]; } return result.str(); } @@ -216,15 +217,17 @@ TEST_P(TestGoalHandleStateTransitionSequence, test_goal_handle_state_transitions // Walk through state transitions rcl_ret_t ret; - for (const EventStatePair & event_state : this->test_sequence) { - ret = rcl_action_update_goal_state(&this->goal_handle, event_state.first); - const rcl_action_goal_state_t & expected_state = event_state.second; + for (const EventStateActiveCancelableTuple & event_state : this->test_sequence) { + ret = rcl_action_update_goal_state(&this->goal_handle, std::get<0>(event_state)); + const rcl_action_goal_state_t & expected_state = std::get<1>(event_state); if (GOAL_STATE_UNKNOWN == expected_state) { EXPECT_EQ(ret, RCL_RET_ACTION_GOAL_EVENT_INVALID); continue; } EXPECT_EQ(ret, RCL_RET_OK); expect_state_eq(expected_state); + EXPECT_EQ(std::get<2>(event_state), rcl_action_goal_handle_is_active(&this->goal_handle)); + EXPECT_EQ(std::get<3>(event_state), rcl_action_goal_handle_is_cancelable(&this->goal_handle)); } } @@ -232,40 +235,40 @@ TEST_P(TestGoalHandleStateTransitionSequence, test_goal_handle_state_transitions // Note, each sequence starts in the ACCEPTED state const StateTransitionSequence valid_state_transition_sequences[] = { { - {GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING}, - {GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING}, - {GOAL_EVENT_SET_CANCELED, GOAL_STATE_CANCELED}, + std::make_tuple(GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING, true, true), + std::make_tuple(GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING, true, false), + std::make_tuple(GOAL_EVENT_SET_CANCELED, GOAL_STATE_CANCELED, false, false), }, { - {GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING}, - {GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING}, - {GOAL_EVENT_SET_SUCCEEDED, GOAL_STATE_SUCCEEDED}, + std::make_tuple(GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING, true, true), + std::make_tuple(GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING, true, false), + std::make_tuple(GOAL_EVENT_SET_SUCCEEDED, GOAL_STATE_SUCCEEDED, false, false), }, { - {GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING}, - {GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING}, - {GOAL_EVENT_SET_ABORTED, GOAL_STATE_ABORTED}, + std::make_tuple(GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING, true, true), + std::make_tuple(GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING, true, false), + std::make_tuple(GOAL_EVENT_SET_ABORTED, GOAL_STATE_ABORTED, false, false), }, { - {GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING}, - {GOAL_EVENT_SET_SUCCEEDED, GOAL_STATE_SUCCEEDED}, + std::make_tuple(GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING, true, true), + std::make_tuple(GOAL_EVENT_SET_SUCCEEDED, GOAL_STATE_SUCCEEDED, false, false), }, { - {GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING}, - {GOAL_EVENT_SET_ABORTED, GOAL_STATE_ABORTED}, + std::make_tuple(GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING, true, true), + std::make_tuple(GOAL_EVENT_SET_ABORTED, GOAL_STATE_ABORTED, false, false), }, { - {GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING}, - {GOAL_EVENT_SET_CANCELED, GOAL_STATE_CANCELED}, + std::make_tuple(GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING, true, false), + std::make_tuple(GOAL_EVENT_SET_CANCELED, GOAL_STATE_CANCELED, false, false), }, { - {GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING}, - {GOAL_EVENT_SET_ABORTED, GOAL_STATE_ABORTED}, + std::make_tuple(GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING, true, false), + std::make_tuple(GOAL_EVENT_SET_ABORTED, GOAL_STATE_ABORTED, false, false), }, // This is an odd case, but valid nonetheless { - {GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING}, - {GOAL_EVENT_SET_SUCCEEDED, GOAL_STATE_SUCCEEDED}, + std::make_tuple(GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING, true, false), + std::make_tuple(GOAL_EVENT_SET_SUCCEEDED, GOAL_STATE_SUCCEEDED, false, false), }, }; @@ -277,27 +280,27 @@ INSTANTIATE_TEST_CASE_P( const StateTransitionSequence invalid_state_transition_sequences[] = { { - {GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING}, - {GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING}, - {GOAL_EVENT_EXECUTE, GOAL_STATE_UNKNOWN}, + std::make_tuple(GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING, true, true), + std::make_tuple(GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING, true, false), + std::make_tuple(GOAL_EVENT_EXECUTE, GOAL_STATE_UNKNOWN, false, false), }, { - {GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING}, - {GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING}, - {GOAL_EVENT_CANCEL, GOAL_STATE_UNKNOWN}, + std::make_tuple(GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING, true, true), + std::make_tuple(GOAL_EVENT_CANCEL, GOAL_STATE_CANCELING, true, false), + std::make_tuple(GOAL_EVENT_CANCEL, GOAL_STATE_UNKNOWN, false, false), }, { - {GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING}, - {GOAL_EVENT_EXECUTE, GOAL_STATE_UNKNOWN}, + std::make_tuple(GOAL_EVENT_EXECUTE, GOAL_STATE_EXECUTING, true, true), + std::make_tuple(GOAL_EVENT_EXECUTE, GOAL_STATE_UNKNOWN, false, false), }, { - {GOAL_EVENT_SET_CANCELED, GOAL_STATE_UNKNOWN}, + std::make_tuple(GOAL_EVENT_SET_CANCELED, GOAL_STATE_UNKNOWN, false, false), }, { - {GOAL_EVENT_SET_SUCCEEDED, GOAL_STATE_UNKNOWN}, + std::make_tuple(GOAL_EVENT_SET_SUCCEEDED, GOAL_STATE_UNKNOWN, false, false), }, { - {GOAL_EVENT_SET_ABORTED, GOAL_STATE_UNKNOWN}, + std::make_tuple(GOAL_EVENT_SET_ABORTED, GOAL_STATE_UNKNOWN, false, false), }, }; From 3a9be732a6fe97a546aa01ff36bcb75e2336be94 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Mon, 12 Nov 2018 09:57:05 -0800 Subject: [PATCH 2/3] Address review --- rcl_action/include/rcl_action/goal_handle.h | 13 +++++++------ rcl_action/test/rcl_action/test_goal_handle.cpp | 17 +++++++++++------ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/rcl_action/include/rcl_action/goal_handle.h b/rcl_action/include/rcl_action/goal_handle.h index d991c27dd..1295ef0a3 100644 --- a/rcl_action/include/rcl_action/goal_handle.h +++ b/rcl_action/include/rcl_action/goal_handle.h @@ -203,8 +203,8 @@ rcl_action_goal_handle_get_status( * * \param[in] goal_handle struct containing the goal and metadata * \return `true` if the goal is in one of the following states: ACCEPTED, EXECUTING, or CANCELING, or - * \return `false` otherwise, also - * \return `false` if the goal handle pointer is invalid + * \return `false` if the goal handle pointer is invalid, or + * \return `false` otherwise */ RCL_ACTION_PUBLIC RCL_WARN_UNUSED @@ -225,8 +225,8 @@ rcl_action_goal_handle_is_active(const rcl_action_goal_handle_t * goal_handle); * * \param[in] goal_handle struct containing the goal and metadata * \return `true` if the goal can be transitioned to CANCELING from its current state, or - * \return `false` otherwise, also - * \return `false` if the goal handle pointer is invalid + * \return `false` if the goal handle pointer is invalid, or + * \return `false` otherwise */ RCL_ACTION_PUBLIC RCL_WARN_UNUSED @@ -251,8 +251,9 @@ rcl_action_goal_handle_is_cancelable(const rcl_action_goal_handle_t * goal_handl * Lock-Free | Yes * * \param[in] goal_handle struct to evaluate as valid or not - * \return `true` if the goal handle is valid, `false` otherwise, also - * \return `false` if the goal handle pointer is null + * \return `true` if the goal handle is valid, or + * \return `false` if the goal handle pointer is null, + * \return `false` otherwise */ RCL_ACTION_PUBLIC RCL_WARN_UNUSED diff --git a/rcl_action/test/rcl_action/test_goal_handle.cpp b/rcl_action/test/rcl_action/test_goal_handle.cpp index 7481a099b..e5b0b59d9 100644 --- a/rcl_action/test/rcl_action/test_goal_handle.cpp +++ b/rcl_action/test/rcl_action/test_goal_handle.cpp @@ -218,16 +218,21 @@ TEST_P(TestGoalHandleStateTransitionSequence, test_goal_handle_state_transitions // Walk through state transitions rcl_ret_t ret; for (const EventStateActiveCancelableTuple & event_state : this->test_sequence) { - ret = rcl_action_update_goal_state(&this->goal_handle, std::get<0>(event_state)); - const rcl_action_goal_state_t & expected_state = std::get<1>(event_state); - if (GOAL_STATE_UNKNOWN == expected_state) { + rcl_action_goal_event_t goal_event; + rcl_action_goal_state_t expected_goal_state; + bool expected_is_active; + bool expected_is_cancelable; + std::tie(goal_event, expected_goal_state, expected_is_active, expected_is_cancelable) = + event_state; + ret = rcl_action_update_goal_state(&this->goal_handle, goal_event); + if (GOAL_STATE_UNKNOWN == expected_goal_state) { EXPECT_EQ(ret, RCL_RET_ACTION_GOAL_EVENT_INVALID); continue; } EXPECT_EQ(ret, RCL_RET_OK); - expect_state_eq(expected_state); - EXPECT_EQ(std::get<2>(event_state), rcl_action_goal_handle_is_active(&this->goal_handle)); - EXPECT_EQ(std::get<3>(event_state), rcl_action_goal_handle_is_cancelable(&this->goal_handle)); + expect_state_eq(expected_goal_state); + EXPECT_EQ(expected_is_active, rcl_action_goal_handle_is_active(&this->goal_handle)); + EXPECT_EQ(expected_is_cancelable, rcl_action_goal_handle_is_cancelable(&this->goal_handle)); } } From 2529005d504b1511b09b26ae1a2cbc8cb1b4fb52 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Tue, 13 Nov 2018 10:52:03 -0800 Subject: [PATCH 3/3] Fix typo --- rcl_action/include/rcl_action/goal_handle.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl_action/include/rcl_action/goal_handle.h b/rcl_action/include/rcl_action/goal_handle.h index 1295ef0a3..ecd903401 100644 --- a/rcl_action/include/rcl_action/goal_handle.h +++ b/rcl_action/include/rcl_action/goal_handle.h @@ -252,7 +252,7 @@ rcl_action_goal_handle_is_cancelable(const rcl_action_goal_handle_t * goal_handl * * \param[in] goal_handle struct to evaluate as valid or not * \return `true` if the goal handle is valid, or - * \return `false` if the goal handle pointer is null, + * \return `false` if the goal handle pointer is null, or * \return `false` otherwise */ RCL_ACTION_PUBLIC