From 68863b614c9d5983e0aafa3bcbdb8a453e0cdf56 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Mon, 6 Jul 2020 18:53:01 -0700 Subject: [PATCH 1/7] perf: refactor from switch to map state transitions PL-629/state-string --- src/am_super/state_mediator.cpp | 102 +++++++------------------------- test/state_unit_tests.cpp | 21 ++----- 2 files changed, 28 insertions(+), 95 deletions(-) diff --git a/src/am_super/state_mediator.cpp b/src/am_super/state_mediator.cpp index 5bc5a677..4fdbde61 100644 --- a/src/am_super/state_mediator.cpp +++ b/src/am_super/state_mediator.cpp @@ -3,89 +3,31 @@ namespace am { - StateMediator::StateMediator(){ - } +const std::map> transitions_ = +{ + { SuperState::OFF, {SuperState::BOOTING} }, + { SuperState::BOOTING, {SuperState::READY} }, + { SuperState::READY, {SuperState::ARMING} }, + { SuperState::ARMING, {SuperState::ARMED} }, + { SuperState::ARMED, {SuperState::AUTO, SuperState::ABORT} }, + { SuperState::AUTO, {SuperState::READY,SuperState::SEMI_AUTO,SuperState::HOLD,SuperState::ABORT,SuperState::MANUAL} }, + { SuperState::SEMI_AUTO, {SuperState::AUTO,SuperState::HOLD,SuperState::ABORT,SuperState::MANUAL} }, + { SuperState::HOLD, {SuperState::ABORT,SuperState::MANUAL} }, + { SuperState::ABORT, {SuperState::READY,SuperState::MANUAL} }, + { SuperState::MANUAL, {SuperState::READY} }, + { SuperState::SHUTDOWN, {SuperState::OFF} }, +}; + +StateMediator::StateMediator(){ +} bool StateMediator::allowsTransition(SuperState from, SuperState to) { bool legal = false; - switch (from) - { - case SuperState::OFF: - if (to == SuperState::BOOTING) - { - legal = true; - } - break; - case SuperState::BOOTING: - if (to == SuperState::READY) - { - legal = true; - } - break; - case SuperState::READY: - if (to == SuperState::ARMING) - { - legal = true; - } - else if (to == SuperState::ARMING) - { - legal = true; - } - break; - case SuperState::ARMING: - if (to == SuperState::ARMED) - { - legal = true; - } - break; - case SuperState::ARMED: - // TODO: remove ABORT state here once we know how to deal with arming errors (should go back to READY). - if (to == SuperState::AUTO || to == SuperState::ABORT) - { - legal = true; - } - break; - case SuperState::AUTO: - if (to == SuperState::READY || to == SuperState::SEMI_AUTO || to == SuperState::HOLD || to == SuperState::ABORT || - to == SuperState::MANUAL) - { - legal = true; - } - break; - case SuperState::SEMI_AUTO: - if (to == SuperState::AUTO || to == SuperState::HOLD || to == SuperState::ABORT || to == SuperState::MANUAL) - { - legal = true; - } - break; - case SuperState::HOLD: - if (to == SuperState::ABORT || to == SuperState::MANUAL) - { - legal = true; - } - break; - case SuperState::ABORT: - if (to == SuperState::READY || to == SuperState::MANUAL) - { - legal = true; - } - break; - case SuperState::MANUAL: - if (to == SuperState::READY) - { - legal = true; - } - break; - case SuperState::SHUTDOWN: - if (to == SuperState::OFF) - { - legal = true; - } - break; - default: - { - throw std::invalid_argument("Unhandled state"); + if(transitions_.count(from) > 0){ + std::vector allowed = transitions_.at(from); + if(std::find(allowed.begin(),allowed.end(),to) != allowed.end()){ + legal = true; } } return legal; @@ -101,4 +43,6 @@ std::vector StateMediator::allSuperStates() } return all; } + + }; diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 43b8ea9a..6e2d0241 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -114,22 +114,11 @@ TEST(StateMediator, allowsTransition_ShutdownToOffAllowed) ASSERT_SINGLE_STATE_ALLOWED(SuperState::SHUTDOWN,SuperState::OFF); } -/** LAST_STATE exists to accomodate C++ limitations. If used, an - * expection is thrown since it is a coding error.*/ -TEST(StateMediator, allowsTransition_UnhandledThrowsException) -{ - try - { - int someBadNumber=999999; - mediator.allowsTransition((SuperState)someBadNumber,SuperState::OFF); - FAIL() << "Expected expection since last state not handled"; - - } - catch(const std::exception& e) - { - //expected. - } - +TEST(StateMediator, allowsTransition_InvalidStateNotAllowedNoThrow) +{ + int someBadNumber=999999; + ASSERT_TRANSITION_ALLOWED((SuperState)someBadNumber,SuperState::OFF,false); + ASSERT_TRANSITION_ALLOWED(SuperState::OFF,(SuperState)someBadNumber,false); } From 8e3c42838d35c6ccefbe6a57e94e6cc5455605ad Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Tue, 7 Jul 2020 08:28:04 -0700 Subject: [PATCH 2/7] perf: refactor string representation PL-629/state-string --- include/am_super/state_mediator.h | 11 +++++++++++ src/am_super/state_mediator.cpp | 23 +++++++++++++++++++++++ test/state_unit_tests.cpp | 11 +++++++++++ 3 files changed, 45 insertions(+) diff --git a/include/am_super/state_mediator.h b/include/am_super/state_mediator.h index d76af8f7..388e4fd0 100644 --- a/include/am_super/state_mediator.h +++ b/include/am_super/state_mediator.h @@ -19,9 +19,16 @@ namespace am */ class StateMediator { + public: StateMediator(); + /** An indicator that the string is not valid. + * Instead of throwing an exception which causes difficult to debug seg faults + * or returning an empty string creating a mystery of what why something is missing. + **/ + static constexpr std::string_view INVALID_STRING = "INVALID"; + /** See https://automodality.atlassian.net/wiki/spaces/AMROS/pages/929234949/AMROS+System+States * @return true if the new state is acceptable to follow the current. */ @@ -32,6 +39,10 @@ class StateMediator */ std::vector allSuperStates(); + /**String representation of the enumeration. + */ + std::string_view& stateToString(SuperState state); + private: }; } diff --git a/src/am_super/state_mediator.cpp b/src/am_super/state_mediator.cpp index 4fdbde61..312b6312 100644 --- a/src/am_super/state_mediator.cpp +++ b/src/am_super/state_mediator.cpp @@ -18,6 +18,21 @@ const std::map> transitions_ = { SuperState::SHUTDOWN, {SuperState::OFF} }, }; +const std::map state_strings_ = +{ + { SuperState::OFF, "OFF" }, + { SuperState::BOOTING, "BOOTING" }, + { SuperState::READY, "READY" }, + { SuperState::ARMING, "ARMING" }, + { SuperState::ARMED, "ARMED" }, + { SuperState::AUTO, "AUTO" }, + { SuperState::SEMI_AUTO, "SEMI_AUTO" }, + { SuperState::HOLD, "HOLD" }, + { SuperState::ABORT, "ABORT" }, + { SuperState::MANUAL, "MANUAL" }, + { SuperState::SHUTDOWN, "SHUTDOWN" }, +}; + StateMediator::StateMediator(){ } @@ -45,4 +60,12 @@ std::vector StateMediator::allSuperStates() } +std::string_view stateToString(SuperState state) +{ + if(state_strings_.count(state) > 0){ + return state_strings_.at(state); + }else{ + return StateMediator::INVALID_STRING; + } +} }; diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 6e2d0241..ffae31d8 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -135,6 +135,17 @@ TEST(StateMediator, allowsTransition_SemiAutoToManyAllowed) ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::SEMI_AUTO,allowed); } + +TEST(StateMediator, stateToString_AllStatesHaveString) +{ + for(SuperState state: mediator.allSuperStates()) + { + std::string_view str = mediator.stateToString(state); + ASSERT_NE(str,StateMediator::INVALID_STRING); + } +} + + /**Basic validation of super state enumeration */ TEST(StateMediator, allSuperStates_IncludesAll) { From c360a8f419a431688f4ab4f66b4fe29d31355f94 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Tue, 7 Jul 2020 08:41:36 -0700 Subject: [PATCH 3/7] matching signature definition PL-629/state-string --- include/am_super/state_mediator.h | 2 +- src/am_super/state_mediator.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/am_super/state_mediator.h b/include/am_super/state_mediator.h index 388e4fd0..d17d5ee0 100644 --- a/include/am_super/state_mediator.h +++ b/include/am_super/state_mediator.h @@ -41,7 +41,7 @@ class StateMediator /**String representation of the enumeration. */ - std::string_view& stateToString(SuperState state); + std::string_view stateToString(SuperState state); private: }; diff --git a/src/am_super/state_mediator.cpp b/src/am_super/state_mediator.cpp index 312b6312..bbccbad8 100644 --- a/src/am_super/state_mediator.cpp +++ b/src/am_super/state_mediator.cpp @@ -60,12 +60,12 @@ std::vector StateMediator::allSuperStates() } -std::string_view stateToString(SuperState state) +std::string_view StateMediator::stateToString(SuperState state) { if(state_strings_.count(state) > 0){ return state_strings_.at(state); }else{ - return StateMediator::INVALID_STRING; + return INVALID_STRING; } } }; From 63bba10c54b6c02e1616743124e1af077f54578d Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Tue, 7 Jul 2020 08:42:07 -0700 Subject: [PATCH 4/7] test: asserting invalid string returned PL-629/state-string --- test/state_unit_tests.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index ffae31d8..7f00ef8c 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -145,6 +145,12 @@ TEST(StateMediator, stateToString_AllStatesHaveString) } } +TEST(StateMediator, stateToString_InvalidStateReturnsInvalidString) +{ + std::string_view str = mediator.stateToString((SuperState)999999); + ASSERT_EQ(str,StateMediator::INVALID_STRING); +} + /**Basic validation of super state enumeration */ TEST(StateMediator, allSuperStates_IncludesAll) From b3ebd459f773d22f7ace38c47970f678f0fdf1f9 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Tue, 7 Jul 2020 09:00:44 -0700 Subject: [PATCH 5/7] ci: format after success PL-629/state-string --- .github/workflows/package.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/workflows/package.yml b/.github/workflows/package.yml index 0dffef0e..8214ea05 100644 --- a/.github/workflows/package.yml +++ b/.github/workflows/package.yml @@ -63,3 +63,12 @@ jobs: with: pr_draft: true github_token: ${{ secrets.GITHUB_TOKEN }} + code-format: + runs-on: ubuntu-18.04 + needs: package + steps: + - uses: actions/checkout@v2 + - name: Format Code + uses: AutoModality/action-ros-clang-format@master + env: + GITHUB_TOKEN: ${{ secrets.AMGITBOT_PAT }} \ No newline at end of file From 79ccde100fc5f9a290d14595b2cc67ce1af3043e Mon Sep 17 00:00:00 2001 From: Format Bot Date: Tue, 7 Jul 2020 16:07:19 +0000 Subject: [PATCH 6/7] style: Applied ROS C++ Style Guide --- include/am_super/state_mediator.h | 18 ++--- src/am_super/am_super.cpp | 23 +++--- src/am_super/state_mediator.cpp | 71 +++++++++--------- test/state_unit_tests.cpp | 118 ++++++++++++++---------------- 4 files changed, 111 insertions(+), 119 deletions(-) diff --git a/include/am_super/state_mediator.h b/include/am_super/state_mediator.h index d17d5ee0..e52c482b 100644 --- a/include/am_super/state_mediator.h +++ b/include/am_super/state_mediator.h @@ -7,25 +7,23 @@ namespace am { - /** - * Provides all logic about system State allowing the State objects - * to be simply data objects. Allows the ROS node to hold the state of the - * system, but delegates all logic to here improving readability, encapsulation + * Provides all logic about system State allowing the State objects + * to be simply data objects. Allows the ROS node to hold the state of the + * system, but delegates all logic to here improving readability, encapsulation * and ability to test. - * + * * This class is stateless, but non-static allowing extension to change behavior - * if different deployments warrant different behavior. + * if different deployments warrant different behavior. */ class StateMediator { - public: StateMediator(); - /** An indicator that the string is not valid. + /** An indicator that the string is not valid. * Instead of throwing an exception which causes difficult to debug seg faults - * or returning an empty string creating a mystery of what why something is missing. + * or returning an empty string creating a mystery of what why something is missing. **/ static constexpr std::string_view INVALID_STRING = "INVALID"; @@ -33,7 +31,7 @@ class StateMediator * @return true if the new state is acceptable to follow the current. */ bool allowsTransition(SuperState from, SuperState to); - + /** * @return a vector of all states in order of declaration, excluding LastState which is used for enum iteration */ diff --git a/src/am_super/am_super.cpp b/src/am_super/am_super.cpp index 9001aced..06f04dd4 100644 --- a/src/am_super/am_super.cpp +++ b/src/am_super/am_super.cpp @@ -764,28 +764,29 @@ class AMSuper { ROS_INFO_STREAM("request change system state from: " << stateToString(system_state_) << " to: " << stateToString(state)); - bool legal = state_mediator_.allowsTransition(system_state_,state); + bool legal = state_mediator_.allowsTransition(system_state_, state); if (!legal) { - ROS_ERROR_STREAM("illegal state transition from " - << stateToString(system_state_) << " to " << stateToString(state)); + ROS_ERROR_STREAM("illegal state transition from " << stateToString(system_state_) << " to " + << stateToString(state)); } else { - //send lifecycle updates for selected state transitions - switch(state){ + // send lifecycle updates for selected state transitions + switch (state) + { case SuperState::READY: - ROS_INFO_STREAM("sending CONFIGURE to all nodes"); - sendLifeCycleCommand(AMLifeCycle::BROADCAST_NODE_NAME, LifeCycleCommand::CONFIGURE); + ROS_INFO_STREAM("sending CONFIGURE to all nodes"); + sendLifeCycleCommand(AMLifeCycle::BROADCAST_NODE_NAME, LifeCycleCommand::CONFIGURE); break; case SuperState::ARMING: - ROS_INFO_STREAM("sending ACTIVATE to all nodes"); - sendLifeCycleCommand(AMLifeCycle::BROADCAST_NODE_NAME, LifeCycleCommand::ACTIVATE); + ROS_INFO_STREAM("sending ACTIVATE to all nodes"); + sendLifeCycleCommand(AMLifeCycle::BROADCAST_NODE_NAME, LifeCycleCommand::ACTIVATE); break; } - - //persist given state as the new current state + + // persist given state as the new current state system_state_ = state; reportSystemState(); diff --git a/src/am_super/state_mediator.cpp b/src/am_super/state_mediator.cpp index bbccbad8..9ab7cbfb 100644 --- a/src/am_super/state_mediator.cpp +++ b/src/am_super/state_mediator.cpp @@ -2,46 +2,47 @@ namespace am { - -const std::map> transitions_ = -{ - { SuperState::OFF, {SuperState::BOOTING} }, - { SuperState::BOOTING, {SuperState::READY} }, - { SuperState::READY, {SuperState::ARMING} }, - { SuperState::ARMING, {SuperState::ARMED} }, - { SuperState::ARMED, {SuperState::AUTO, SuperState::ABORT} }, - { SuperState::AUTO, {SuperState::READY,SuperState::SEMI_AUTO,SuperState::HOLD,SuperState::ABORT,SuperState::MANUAL} }, - { SuperState::SEMI_AUTO, {SuperState::AUTO,SuperState::HOLD,SuperState::ABORT,SuperState::MANUAL} }, - { SuperState::HOLD, {SuperState::ABORT,SuperState::MANUAL} }, - { SuperState::ABORT, {SuperState::READY,SuperState::MANUAL} }, - { SuperState::MANUAL, {SuperState::READY} }, - { SuperState::SHUTDOWN, {SuperState::OFF} }, +const std::map> transitions_ = { + { SuperState::OFF, { SuperState::BOOTING } }, + { SuperState::BOOTING, { SuperState::READY } }, + { SuperState::READY, { SuperState::ARMING } }, + { SuperState::ARMING, { SuperState::ARMED } }, + { SuperState::ARMED, { SuperState::AUTO, SuperState::ABORT } }, + { SuperState::AUTO, + { SuperState::READY, SuperState::SEMI_AUTO, SuperState::HOLD, SuperState::ABORT, SuperState::MANUAL } }, + { SuperState::SEMI_AUTO, { SuperState::AUTO, SuperState::HOLD, SuperState::ABORT, SuperState::MANUAL } }, + { SuperState::HOLD, { SuperState::ABORT, SuperState::MANUAL } }, + { SuperState::ABORT, { SuperState::READY, SuperState::MANUAL } }, + { SuperState::MANUAL, { SuperState::READY } }, + { SuperState::SHUTDOWN, { SuperState::OFF } }, }; -const std::map state_strings_ = -{ - { SuperState::OFF, "OFF" }, - { SuperState::BOOTING, "BOOTING" }, - { SuperState::READY, "READY" }, - { SuperState::ARMING, "ARMING" }, - { SuperState::ARMED, "ARMED" }, - { SuperState::AUTO, "AUTO" }, - { SuperState::SEMI_AUTO, "SEMI_AUTO" }, - { SuperState::HOLD, "HOLD" }, - { SuperState::ABORT, "ABORT" }, - { SuperState::MANUAL, "MANUAL" }, - { SuperState::SHUTDOWN, "SHUTDOWN" }, +const std::map state_strings_ = { + { SuperState::OFF, "OFF" }, + { SuperState::BOOTING, "BOOTING" }, + { SuperState::READY, "READY" }, + { SuperState::ARMING, "ARMING" }, + { SuperState::ARMED, "ARMED" }, + { SuperState::AUTO, "AUTO" }, + { SuperState::SEMI_AUTO, "SEMI_AUTO" }, + { SuperState::HOLD, "HOLD" }, + { SuperState::ABORT, "ABORT" }, + { SuperState::MANUAL, "MANUAL" }, + { SuperState::SHUTDOWN, "SHUTDOWN" }, }; -StateMediator::StateMediator(){ +StateMediator::StateMediator() +{ } bool StateMediator::allowsTransition(SuperState from, SuperState to) { bool legal = false; - if(transitions_.count(from) > 0){ + if (transitions_.count(from) > 0) + { std::vector allowed = transitions_.at(from); - if(std::find(allowed.begin(),allowed.end(),to) != allowed.end()){ + if (std::find(allowed.begin(), allowed.end(), to) != allowed.end()) + { legal = true; } } @@ -51,7 +52,7 @@ bool StateMediator::allowsTransition(SuperState from, SuperState to) std::vector StateMediator::allSuperStates() { std::vector all; - for ( int enumIndex = (int) SuperState::OFF; enumIndex <= (int) SuperState::LAST_STATE; enumIndex++ ) + for (int enumIndex = (int)SuperState::OFF; enumIndex <= (int)SuperState::LAST_STATE; enumIndex++) { SuperState state = static_cast(enumIndex); all.push_back(state); @@ -59,12 +60,14 @@ std::vector StateMediator::allSuperStates() return all; } - std::string_view StateMediator::stateToString(SuperState state) { - if(state_strings_.count(state) > 0){ + if (state_strings_.count(state) > 0) + { return state_strings_.at(state); - }else{ + } + else + { return INVALID_STRING; } } diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 7f00ef8c..e1c83768 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -9,164 +9,154 @@ using namespace am; StateMediator mediator; /**Re-usable test method for validating single states*/ -void -ASSERT_TRANSITION_ALLOWED(SuperState from, SuperState to, bool expected) +void ASSERT_TRANSITION_ALLOWED(SuperState from, SuperState to, bool expected) { bool allowed = mediator.allowsTransition(from, to); - EXPECT_EQ(expected,allowed) << "For state: " + std::to_string((int)to); + EXPECT_EQ(expected, allowed) << "For state: " + std::to_string((int)to); } /**Re-usable test method for validating multiple states behave as expected*/ -void -ASSERT_TRANSITIONS_ALLOWED(SuperState from, std::vector states, bool expected) +void ASSERT_TRANSITIONS_ALLOWED(SuperState from, std::vector states, bool expected) { - for(int i = 0; i < states.size(); i++) + for (int i = 0; i < states.size(); i++) { SuperState state = states.at(i); - ASSERT_TRANSITION_ALLOWED(from,state,expected); + ASSERT_TRANSITION_ALLOWED(from, state, expected); } } - /**Re-usable method for validating the states given are allowed and all other states * are not allowed */ -void -ASSERT_MULTIPLE_STATES_ALLOWED(SuperState from, std::vector allowedStates) +void ASSERT_MULTIPLE_STATES_ALLOWED(SuperState from, std::vector allowedStates) { - //verify allowed as expected - for(int i = 0; i < allowedStates.size(); i++) + // verify allowed as expected + for (int i = 0; i < allowedStates.size(); i++) { SuperState to = allowedStates.at(i); - ASSERT_TRANSITION_ALLOWED(from,to,true); + ASSERT_TRANSITION_ALLOWED(from, to, true); } - - //not allowed is all states minus those allowed + // not allowed is all states minus those allowed std::vector notAllowed; - for(SuperState state: mediator.allSuperStates()) + for (SuperState state : mediator.allSuperStates()) { - if (std::find(allowedStates.begin(), allowedStates.end(), state) == allowedStates.end()) - { - notAllowed.push_back(state); - } + if (std::find(allowedStates.begin(), allowedStates.end(), state) == allowedStates.end()) + { + notAllowed.push_back(state); + } } - ASSERT_TRANSITIONS_ALLOWED(from,notAllowed,false); + ASSERT_TRANSITIONS_ALLOWED(from, notAllowed, false); } /**Common situation to ensure a single state transition is allowed*/ -void -ASSERT_SINGLE_STATE_ALLOWED(SuperState from, SuperState to) +void ASSERT_SINGLE_STATE_ALLOWED(SuperState from, SuperState to) { - std::vector allowed{to}; - ASSERT_MULTIPLE_STATES_ALLOWED(from,allowed); + std::vector allowed{ to }; + ASSERT_MULTIPLE_STATES_ALLOWED(from, allowed); } TEST(StateMediator, allowsTransition_OnlyOffToBootingIAllowed) { - ASSERT_SINGLE_STATE_ALLOWED(SuperState::OFF,SuperState::BOOTING); + ASSERT_SINGLE_STATE_ALLOWED(SuperState::OFF, SuperState::BOOTING); } TEST(StateMediator, allowsTransition_OnlyBootingToReadyIsAllowed) { - ASSERT_SINGLE_STATE_ALLOWED(SuperState::BOOTING,SuperState::READY); + ASSERT_SINGLE_STATE_ALLOWED(SuperState::BOOTING, SuperState::READY); } TEST(StateMediator, allowsTransition_OnlyReadyToArmingIsAllowed) { - ASSERT_SINGLE_STATE_ALLOWED(SuperState::READY,SuperState::ARMING); + ASSERT_SINGLE_STATE_ALLOWED(SuperState::READY, SuperState::ARMING); } TEST(StateMediator, allowsTransition_OnlyArmingToArmedIsAllowed) { - ASSERT_SINGLE_STATE_ALLOWED(SuperState::ARMING,SuperState::ARMED); + ASSERT_SINGLE_STATE_ALLOWED(SuperState::ARMING, SuperState::ARMED); } TEST(StateMediator, allowsTransition_ArmedToAutoAndAbortIsAllowed) { - std::vector allowed{SuperState::ABORT, SuperState::AUTO}; - ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::ARMED,allowed); + std::vector allowed{ SuperState::ABORT, SuperState::AUTO }; + ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::ARMED, allowed); } - TEST(StateMediator, allowsTransition_HoldToExceptionStates) { - std::vector allowed{SuperState::ABORT, SuperState::MANUAL}; + std::vector allowed{ SuperState::ABORT, SuperState::MANUAL }; - ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::HOLD,allowed); + ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::HOLD, allowed); } TEST(StateMediator, allowsTransition_AbortToReadyAndManualAllowed) { - std::vector allowed{SuperState::READY,SuperState::MANUAL}; + std::vector allowed{ SuperState::READY, SuperState::MANUAL }; - ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::ABORT,allowed); + ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::ABORT, allowed); } TEST(StateMediator, allowsTransition_OnlyManualToReadyIsAllowed) { - ASSERT_SINGLE_STATE_ALLOWED(SuperState::MANUAL,SuperState::READY); + ASSERT_SINGLE_STATE_ALLOWED(SuperState::MANUAL, SuperState::READY); } TEST(StateMediator, allowsTransition_ShutdownToOffAllowed) { - ASSERT_SINGLE_STATE_ALLOWED(SuperState::SHUTDOWN,SuperState::OFF); + ASSERT_SINGLE_STATE_ALLOWED(SuperState::SHUTDOWN, SuperState::OFF); } TEST(StateMediator, allowsTransition_InvalidStateNotAllowedNoThrow) -{ - int someBadNumber=999999; - ASSERT_TRANSITION_ALLOWED((SuperState)someBadNumber,SuperState::OFF,false); - ASSERT_TRANSITION_ALLOWED(SuperState::OFF,(SuperState)someBadNumber,false); +{ + int someBadNumber = 999999; + ASSERT_TRANSITION_ALLOWED((SuperState)someBadNumber, SuperState::OFF, false); + ASSERT_TRANSITION_ALLOWED(SuperState::OFF, (SuperState)someBadNumber, false); } - TEST(StateMediator, allowsTransition_AutoToManyAllowed) { - std::vector allowed{SuperState::HOLD,SuperState::ABORT,SuperState::READY,SuperState::SEMI_AUTO, SuperState::MANUAL}; + std::vector allowed{ SuperState::HOLD, SuperState::ABORT, SuperState::READY, SuperState::SEMI_AUTO, + SuperState::MANUAL }; - ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::AUTO,allowed); + ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::AUTO, allowed); } TEST(StateMediator, allowsTransition_SemiAutoToManyAllowed) { - std::vector allowed{SuperState::AUTO, SuperState::HOLD, SuperState::ABORT, SuperState::MANUAL}; + std::vector allowed{ SuperState::AUTO, SuperState::HOLD, SuperState::ABORT, SuperState::MANUAL }; - ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::SEMI_AUTO,allowed); + ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::SEMI_AUTO, allowed); } - TEST(StateMediator, stateToString_AllStatesHaveString) { - for(SuperState state: mediator.allSuperStates()) + for (SuperState state : mediator.allSuperStates()) { std::string_view str = mediator.stateToString(state); - ASSERT_NE(str,StateMediator::INVALID_STRING); + ASSERT_NE(str, StateMediator::INVALID_STRING); } } TEST(StateMediator, stateToString_InvalidStateReturnsInvalidString) { std::string_view str = mediator.stateToString((SuperState)999999); - ASSERT_EQ(str,StateMediator::INVALID_STRING); + ASSERT_EQ(str, StateMediator::INVALID_STRING); } - /**Basic validation of super state enumeration */ TEST(StateMediator, allSuperStates_IncludesAll) { std::vector all = mediator.allSuperStates(); - int expectedNumberOfStates =11; + int expectedNumberOfStates = 11; int actualNumberOfStates = all.size(); - ASSERT_EQ(expectedNumberOfStates,actualNumberOfStates); - ASSERT_EQ(all.at((int) SuperState::OFF),SuperState::OFF); - ASSERT_EQ(all.at((int) SuperState::ABORT),SuperState::ABORT); - ASSERT_EQ(all.at((int) SuperState::ARMED),SuperState::ARMED); - ASSERT_EQ(all.at((int) SuperState::ARMING),SuperState::ARMING); - ASSERT_EQ(all.at((int) SuperState::SEMI_AUTO),SuperState::SEMI_AUTO); - ASSERT_EQ(all.at((int) SuperState::SHUTDOWN),SuperState::SHUTDOWN); - ASSERT_EQ(all.front(),SuperState::OFF); - ASSERT_EQ(all.back(),SuperState::SHUTDOWN); - ASSERT_EQ(all.back(),SuperState::LAST_STATE); - + ASSERT_EQ(expectedNumberOfStates, actualNumberOfStates); + ASSERT_EQ(all.at((int)SuperState::OFF), SuperState::OFF); + ASSERT_EQ(all.at((int)SuperState::ABORT), SuperState::ABORT); + ASSERT_EQ(all.at((int)SuperState::ARMED), SuperState::ARMED); + ASSERT_EQ(all.at((int)SuperState::ARMING), SuperState::ARMING); + ASSERT_EQ(all.at((int)SuperState::SEMI_AUTO), SuperState::SEMI_AUTO); + ASSERT_EQ(all.at((int)SuperState::SHUTDOWN), SuperState::SHUTDOWN); + ASSERT_EQ(all.front(), SuperState::OFF); + ASSERT_EQ(all.back(), SuperState::SHUTDOWN); + ASSERT_EQ(all.back(), SuperState::LAST_STATE); } From ffd247071af8b3c474979e4d7093749af9436833 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Tue, 7 Jul 2020 09:13:59 -0700 Subject: [PATCH 7/7] perf: calling tested string method PL-629/state-string --- src/am_super/am_super.cpp | 61 ++++++--------------------------------- 1 file changed, 9 insertions(+), 52 deletions(-) diff --git a/src/am_super/am_super.cpp b/src/am_super/am_super.cpp index 06f04dd4..c39de88f 100644 --- a/src/am_super/am_super.cpp +++ b/src/am_super/am_super.cpp @@ -66,18 +66,6 @@ enum SuperFltCtrlState class AMSuper { private: - static constexpr std::string_view STATE_OFF_STRING = "OFF"; - static constexpr std::string_view STATE_BOOTING_STRING = "BOOTING"; - static constexpr std::string_view STATE_READY_STRING = "READY"; - static constexpr std::string_view STATE_ARMING_STRING = "ARMING"; - static constexpr std::string_view STATE_ARMED_STRING = "ARMED"; - static constexpr std::string_view STATE_AUTO_STRING = "AUTO"; - static constexpr std::string_view STATE_SEMI_AUTO_STRING = "SEMI_AUTO"; - static constexpr std::string_view STATE_HOLD_STRING = "HOLD"; - static constexpr std::string_view STATE_ABORT_STRING = "ABORT"; - static constexpr std::string_view STATE_MANUAL_STRING = "MANUAL"; - static constexpr std::string_view STATE_SHUTDOWN_STRING = "SHUTDOWN"; - /** * heartbeat log output period */ @@ -275,37 +263,6 @@ class AMSuper BagLogger::instance()->stopLogging(); } - static const std::string_view& stateToString(SuperState state) - { - switch (state) - { - case SuperState::OFF: - return STATE_OFF_STRING; - case SuperState::BOOTING: - return STATE_BOOTING_STRING; - case SuperState::READY: - return STATE_READY_STRING; - case SuperState::ARMING: - return STATE_ARMING_STRING; - case SuperState::ARMED: - return STATE_ARMED_STRING; - case SuperState::AUTO: - return STATE_AUTO_STRING; - case SuperState::SEMI_AUTO: - return STATE_SEMI_AUTO_STRING; - case SuperState::HOLD: - return STATE_HOLD_STRING; - case SuperState::ABORT: - return STATE_ABORT_STRING; - case SuperState::MANUAL: - return STATE_MANUAL_STRING; - case SuperState::SHUTDOWN: - return STATE_SHUTDOWN_STRING; - default: - return AMLifeCycle::EMPTY_STRING; - } - } - private: /** * process LifeCycleState messages from nodes @@ -570,7 +527,7 @@ class AMSuper */ void genSystemState(std::stringstream& ss) { - ss << "state: " << stateToString(system_state_) << ", manifest: " << manifest_.size() + ss << "state: " << state_mediator_.stateToString(system_state_) << ", manifest: " << manifest_.size() << ", manifest online:" << num_manifest_nodes_online_ << ", total online:" << num_nodes_online_; } @@ -671,7 +628,7 @@ class AMSuper case SuperState::BOOTING: if (allManifestedNodesCheck(checkReadyForConfigureState)) { - ROS_INFO_STREAM(stateToString(system_state_) << ": changing to READY"); + ROS_INFO_STREAM(state_mediator_.stateToString(system_state_) << ": changing to READY"); setSystemState(SuperState::READY); } // else @@ -684,12 +641,12 @@ class AMSuper if (allManifestedNodesCheck(checkReadyForActivateState)) { // TODO: this should wait for operator to arm - ROS_INFO_STREAM(stateToString(SuperState::READY) << ": changing to ARMING"); + ROS_INFO_STREAM(state_mediator_.stateToString(SuperState::READY) << ": changing to ARMING"); setSystemState(SuperState::ARMING); } else { - ROS_INFO_STREAM(stateToString(system_state_) << ": sending CONFIGURE again"); + ROS_INFO_STREAM(state_mediator_.stateToString(system_state_) << ": sending CONFIGURE again"); sendLifeCycleCommand(AMLifeCycle::BROADCAST_NODE_NAME, LifeCycleCommand::CONFIGURE); } break; @@ -700,7 +657,7 @@ class AMSuper } else { - ROS_INFO_STREAM(stateToString(system_state_) << ": sending ACTIVATE again"); + ROS_INFO_STREAM(state_mediator_.stateToString(system_state_) << ": sending ACTIVATE again"); sendLifeCycleCommand(AMLifeCycle::BROADCAST_NODE_NAME, LifeCycleCommand::ACTIVATE); } break; @@ -762,14 +719,14 @@ class AMSuper */ void setSystemState(SuperState state) { - ROS_INFO_STREAM("request change system state from: " << stateToString(system_state_) - << " to: " << stateToString(state)); + ROS_INFO_STREAM("request change system state from: " << state_mediator_.stateToString(system_state_) + << " to: " << state_mediator_.stateToString(state)); bool legal = state_mediator_.allowsTransition(system_state_, state); if (!legal) { - ROS_ERROR_STREAM("illegal state transition from " << stateToString(system_state_) << " to " - << stateToString(state)); + ROS_ERROR_STREAM("illegal state transition from " << state_mediator_.stateToString(system_state_) << " to " + << state_mediator_.stateToString(state)); } else {