From 1db89a228fc8493a2af250262981e248b7a797b5 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Tue, 9 Jun 2020 07:37:56 -0700 Subject: [PATCH 01/36] test: added multiple test files PL-629/state-tests --- CMakeLists.txt | 6 ++++- test/README.md | 57 ++++++++++++++++++++++++++++++++++++++++++- test/all_tests.cpp | 8 ++++++ test/sample_tests.cpp | 6 ----- test/state_tests.cpp | 13 ++++++++++ 5 files changed, 82 insertions(+), 8 deletions(-) create mode 100644 test/all_tests.cpp create mode 100644 test/state_tests.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 6a6c4de7..95f2a1e7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -167,8 +167,12 @@ install(DIRECTORY include/super_lib ## Testing ## ############# +file(GLOB TEST_FILES + test/*_tests.cpp +) + ## Add gtest based cpp test target and link libraries -catkin_add_gtest(${PROJECT_NAME}_test test/sample_tests.cpp) +catkin_add_gtest(${PROJECT_NAME}_test ${TEST_FILES}) if(TARGET ${PROJECT_NAME}_test) target_link_libraries(${PROJECT_NAME}_test ${catkin_LIBRARIES}) endif() diff --git a/test/README.md b/test/README.md index 816841a4..4b1b0305 100644 --- a/test/README.md +++ b/test/README.md @@ -1 +1,56 @@ -* tests go here +# Tests + +all_tests.cpp will run all the tests + +## Using catkin_make + +``` +catkin_make run_tests +``` + +should give the detailed output + +``` +[==========] Running 2 tests from 1 test case. +[----------] Global test environment set-up. +[----------] 2 tests from StrCompare +[ RUN ] StrCompare.CStrEqual +[ OK ] StrCompare.CStrEqual (0 ms) +[ RUN ] StrCompare.CStrEqual2 +[ OK ] StrCompare.CStrEqual2 (0 ms) +[----------] 2 tests from StrCompare (1 ms total) + +[----------] Global test environment tear-down +[==========] 2 tests from 1 test case ran. (1 ms total) +[ PASSED ] 2 tests. +``` + + +## Using Catkin Tools + +It is better to use `catkin_make` during development. See above. + +Catkin Tools is useful during builds. + +``` +catkin run_tests +``` + +Provides no valuable feedback so you must then run: + +``` +catkin_test_results +``` + +which provides a limited summary and exit code + +``` +Summary: 2 tests, 0 errors, 0 failures, 0 skipped +``` + +So look in the test results folder for more details from the XML report: + +``` +catkin_ws/build/am_super/test_results/am_super +``` + diff --git a/test/all_tests.cpp b/test/all_tests.cpp new file mode 100644 index 00000000..bfc5696f --- /dev/null +++ b/test/all_tests.cpp @@ -0,0 +1,8 @@ +#include // googletest header file + + +int main(int argc, char** argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} \ No newline at end of file diff --git a/test/sample_tests.cpp b/test/sample_tests.cpp index 67965064..4f8deacd 100644 --- a/test/sample_tests.cpp +++ b/test/sample_tests.cpp @@ -11,9 +11,3 @@ TEST(StrCompare, CStrEqual) { EXPECT_STREQ(expectVal, actualValTrue); } - -int main(int argc, char** argv) -{ - ::testing::InitGoogleTest(&argc, argv); - return RUN_ALL_TESTS(); -} \ No newline at end of file diff --git a/test/state_tests.cpp b/test/state_tests.cpp new file mode 100644 index 00000000..d1fbc6cd --- /dev/null +++ b/test/state_tests.cpp @@ -0,0 +1,13 @@ +#include // googletest header file + +#include +using std::string; + +const char* actualVal2True = "hello gtest"; +const char* actualVal2False = "hello world"; +const char* expectVal2 = "hello gtest"; + +TEST(StrCompare, CStrEqual2) +{ + EXPECT_STREQ(expectVal2, actualVal2True); +} From 2cc68396585350a6128ad07ebba1a7dd38369d6f Mon Sep 17 00:00:00 2001 From: Format Bot Date: Tue, 9 Jun 2020 14:44:40 +0000 Subject: [PATCH 02/36] style: Applied ROS C++ Style Guide --- test/all_tests.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/test/all_tests.cpp b/test/all_tests.cpp index bfc5696f..f574220f 100644 --- a/test/all_tests.cpp +++ b/test/all_tests.cpp @@ -1,6 +1,5 @@ #include // googletest header file - int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); From b3f3f5e6e05f731e0267d8e48c3aa55cdacfebb3 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Wed, 24 Jun 2020 05:21:30 -0700 Subject: [PATCH 03/36] test: interfaces defined PL-629/super-header --- README.md | 8 ++++++ include/am_super/state_mediator.h | 28 +++++++++++++++++++ .../{state_tests.cpp => state_unit_tests.cpp} | 3 ++ 3 files changed, 39 insertions(+) create mode 100644 include/am_super/state_mediator.h rename test/{state_tests.cpp => state_unit_tests.cpp} (88%) diff --git a/README.md b/README.md index 3cdfe997..ba558fc5 100644 --- a/README.md +++ b/README.md @@ -5,3 +5,11 @@ Watches the health of all nodes to determine if the flight shall continue.. [![Release](https://github.com/AutoModality/am_super/workflows/Release/badge.svg)](https://github.com/AutoModality/am_super/workflows/Release/badge.svg) [![Latest Version @ Cloudsmith](https://api-prd.cloudsmith.io/badges/version/automodality/release/deb/ros-melodic-am-super/latest/d=ubuntu%252Fbionic;t=1/?render=true&badge_token=gAAAAABetY4e0kXP_ZlIdblJEZG8GiEIYJRkjvt9-nVmp3U4QiqyH-2mOfwi_B7meqOAh3rgt-lbVvFTiAmsysp4iMNx79oZfuVCEac-Lqz-dXxW4W7AbYU%3D)](https://cloudsmith.io/~automodality/repos/release/packages/detail/deb/ros-melodic-am-super/latest/d=ubuntu%252Fbionic;t=1/) + +# Run Tests + +``` +catkin_make run_tests +``` + +See [test](test) for more. \ No newline at end of file diff --git a/include/am_super/state_mediator.h b/include/am_super/state_mediator.h new file mode 100644 index 00000000..61865562 --- /dev/null +++ b/include/am_super/state_mediator.h @@ -0,0 +1,28 @@ + + +#ifndef AM_SUPER_INCLUDE_AM_SUPER_SERVICE_H_ +#define AM_SUPER_INCLUDE_AM_SUPER_SERVICE_H_ + +#include + +namespace am +{ + +/** Stateless class providing logic to determine + * state transition. + */ +class StateMediator +{ +public: + StateMediator(); + /** + * @return true if the new state is acceptable to follow the current. + */ + bool allowsTransition(SuperState from, SuperState to); + +private: + +}; +} + +#endif /* AM_SUPER_INCLUDE_AM_SUPER_SERVICE_H_ */ diff --git a/test/state_tests.cpp b/test/state_unit_tests.cpp similarity index 88% rename from test/state_tests.cpp rename to test/state_unit_tests.cpp index d1fbc6cd..1882e982 100644 --- a/test/state_tests.cpp +++ b/test/state_unit_tests.cpp @@ -1,4 +1,6 @@ #include // googletest header file +#include + #include using std::string; @@ -9,5 +11,6 @@ const char* expectVal2 = "hello gtest"; TEST(StrCompare, CStrEqual2) { + EXPECT_STREQ(expectVal2, actualVal2True); } From 882b3f3262df6cc302f7d59c54d24b230693903f Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Wed, 24 Jun 2020 09:48:41 -0700 Subject: [PATCH 04/36] refactor: extracting state logic to mediator PL-629/state-mediator --- include/am_super/state_mediator.h | 4 +- src/am_super/state_mediator.cpp | 104 ++++++++++++++++++++++++++++++ test/state_unit_tests.cpp | 11 ++-- 3 files changed, 111 insertions(+), 8 deletions(-) create mode 100644 src/am_super/state_mediator.cpp diff --git a/include/am_super/state_mediator.h b/include/am_super/state_mediator.h index 61865562..8ce992b3 100644 --- a/include/am_super/state_mediator.h +++ b/include/am_super/state_mediator.h @@ -8,13 +8,13 @@ namespace am { -/** Stateless class providing logic to determine - * state transition. +/** Stateless class providing logic relating to SuperState rules. */ class StateMediator { public: StateMediator(); + /** * @return true if the new state is acceptable to follow the current. */ diff --git a/src/am_super/state_mediator.cpp b/src/am_super/state_mediator.cpp new file mode 100644 index 00000000..e62ce34c --- /dev/null +++ b/src/am_super/state_mediator.cpp @@ -0,0 +1,104 @@ +#include + +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 + * and ability to test. + * + * This class is stateless, but non-static allowing extension to change behavior + * if different deployments warrant different behavior. + */ + 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 "s + from); + } + } + return legal; + } + +}; diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 1882e982..8b4fd5d7 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -1,16 +1,15 @@ #include // googletest header file #include - +#include #include using std::string; -const char* actualVal2True = "hello gtest"; -const char* actualVal2False = "hello world"; -const char* expectVal2 = "hello gtest"; -TEST(StrCompare, CStrEqual2) +TEST(StateMediator, allowsTransition_OffToBootingIsAllowed) { - EXPECT_STREQ(expectVal2, actualVal2True); + am::StateMediator mediator; + bool allowed = mediator.allowsTransition(SuperState::OFF,SuperState::BOOTING); + EXPECT_TRUE(allowed); } From c706fc093cc8aa95eddccb17d03e5deb92f4a56a Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Wed, 24 Jun 2020 10:35:00 -0700 Subject: [PATCH 05/36] test: added first mediator test PL-629/state-mediator --- CMakeLists.txt | 6 +++--- src/am_super/state_mediator.cpp | 2 +- test/state_unit_tests.cpp | 5 ++--- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 95f2a1e7..e2735e8e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -117,10 +117,10 @@ target_link_libraries(super_lib ${catkin_LIBRARIES}) add_dependencies(super_lib ${catkin_EXPORTED_TARGETS}) if(CUDA_FOUND) - add_executable(am_super src/am_super/am_super.cpp src/cuda/cuda_utility_class.cpp) + add_executable(am_super src/am_super/am_super.cpp src/cuda/cuda_utility_class.cpp src/am_super/state_mediator.cpp) target_link_libraries(am_super ${catkin_LIBRARIES} super_lib cuda_utility) else() - add_executable(am_super src/am_super/am_super.cpp) + add_executable(am_super src/am_super/am_super.cpp src/am_super/state_mediator.cpp) target_link_libraries(am_super ${catkin_LIBRARIES} super_lib) endif(CUDA_FOUND) add_dependencies(am_super ${catkin_EXPORTED_TARGETS}) @@ -172,7 +172,7 @@ file(GLOB TEST_FILES ) ## Add gtest based cpp test target and link libraries -catkin_add_gtest(${PROJECT_NAME}_test ${TEST_FILES}) +catkin_add_gtest(${PROJECT_NAME}_test ${TEST_FILES} src/am_super/state_mediator.cpp) if(TARGET ${PROJECT_NAME}_test) target_link_libraries(${PROJECT_NAME}_test ${catkin_LIBRARIES}) endif() diff --git a/src/am_super/state_mediator.cpp b/src/am_super/state_mediator.cpp index e62ce34c..04ee4ec9 100644 --- a/src/am_super/state_mediator.cpp +++ b/src/am_super/state_mediator.cpp @@ -95,7 +95,7 @@ namespace am break; default: { - throw std::invalid_argument("Unhandled state "s + from); + throw std::invalid_argument("Unhandled state "); } } return legal; diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 8b4fd5d7..503a2231 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -4,12 +4,11 @@ #include using std::string; - +using namespace am; TEST(StateMediator, allowsTransition_OffToBootingIsAllowed) { - - am::StateMediator mediator; + StateMediator mediator; bool allowed = mediator.allowsTransition(SuperState::OFF,SuperState::BOOTING); EXPECT_TRUE(allowed); } From 6bdf8c761d03debced486fc04a1445e0b091997c Mon Sep 17 00:00:00 2001 From: Format Bot Date: Wed, 24 Jun 2020 17:41:06 +0000 Subject: [PATCH 06/36] style: Applied ROS C++ Style Guide --- include/am_super/state_mediator.h | 14 +-- src/am_super/state_mediator.cpp | 190 +++++++++++++++--------------- test/state_unit_tests.cpp | 2 +- 3 files changed, 101 insertions(+), 105 deletions(-) diff --git a/include/am_super/state_mediator.h b/include/am_super/state_mediator.h index 8ce992b3..6a261118 100644 --- a/include/am_super/state_mediator.h +++ b/include/am_super/state_mediator.h @@ -7,21 +7,19 @@ namespace am { - /** Stateless class providing logic relating to SuperState rules. */ class StateMediator { public: - StateMediator(); + StateMediator(); - /** - * @return true if the new state is acceptable to follow the current. - */ - bool allowsTransition(SuperState from, SuperState to); - -private: + /** + * @return true if the new state is acceptable to follow the current. + */ + bool allowsTransition(SuperState from, SuperState to); +private: }; } diff --git a/src/am_super/state_mediator.cpp b/src/am_super/state_mediator.cpp index 04ee4ec9..0beca4c0 100644 --- a/src/am_super/state_mediator.cpp +++ b/src/am_super/state_mediator.cpp @@ -2,103 +2,101 @@ 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 - * and ability to test. - * - * This class is stateless, but non-static allowing extension to change behavior - * if different deployments warrant different behavior. - */ - StateMediator::StateMediator(){ - } +/** + * 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. + */ +StateMediator::StateMediator() +{ +} - bool - StateMediator::allowsTransition(SuperState from, SuperState to) +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: { - 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 "); - } - } - return legal; + throw std::invalid_argument("Unhandled state "); } - + } + return legal; +} }; diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 503a2231..e3641964 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -9,6 +9,6 @@ using namespace am; TEST(StateMediator, allowsTransition_OffToBootingIsAllowed) { StateMediator mediator; - bool allowed = mediator.allowsTransition(SuperState::OFF,SuperState::BOOTING); + bool allowed = mediator.allowsTransition(SuperState::OFF, SuperState::BOOTING); EXPECT_TRUE(allowed); } From 6a22f1914ef8660ac568dcfd7fd012918d996203 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Thu, 25 Jun 2020 06:13:48 -0700 Subject: [PATCH 07/36] docs: relocated comments PL-629/state-mediator --- README.md | 8 ++++++++ debian/control | 2 +- include/am_super/state_mediator.h | 9 ++++++++- src/am_super/state_mediator.cpp | 10 +--------- 4 files changed, 18 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index ba558fc5..ee5c18d0 100644 --- a/README.md +++ b/README.md @@ -6,6 +6,14 @@ Watches the health of all nodes to determine if the flight shall continue.. [![Latest Version @ Cloudsmith](https://api-prd.cloudsmith.io/badges/version/automodality/release/deb/ros-melodic-am-super/latest/d=ubuntu%252Fbionic;t=1/?render=true&badge_token=gAAAAABetY4e0kXP_ZlIdblJEZG8GiEIYJRkjvt9-nVmp3U4QiqyH-2mOfwi_B7meqOAh3rgt-lbVvFTiAmsysp4iMNx79oZfuVCEac-Lqz-dXxW4W7AbYU%3D)](https://cloudsmith.io/~automodality/repos/release/packages/detail/deb/ros-melodic-am-super/latest/d=ubuntu%252Fbionic;t=1/) +# ROS Node + +[AMSuper](include/am_super/am_super.h) is the ROS Node watching all other nodes for system health and reliability. + +# Library + +Other nodes must communicate with the Supervisor and should do so using the [library interface](include/super_lib/). + # Run Tests ``` diff --git a/debian/control b/debian/control index c7ae1031..abd6ef44 100644 --- a/debian/control +++ b/debian/control @@ -9,7 +9,7 @@ Build-Depends: debhelper (>= 9.0.0), ros-melodic-rosbag, ros-melodic-roscpp, ros-melodic-std-msgs, - ros-melodic-std-srvs, + ros-melodic-std-srvs, Homepage: https://github.com/AutoModality/am-super Standards-Version: 3.9.2 diff --git a/include/am_super/state_mediator.h b/include/am_super/state_mediator.h index 8ce992b3..295caa9d 100644 --- a/include/am_super/state_mediator.h +++ b/include/am_super/state_mediator.h @@ -8,7 +8,14 @@ namespace am { -/** Stateless class providing logic relating to SuperState rules. +/** + * 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. */ class StateMediator { diff --git a/src/am_super/state_mediator.cpp b/src/am_super/state_mediator.cpp index 04ee4ec9..a9258bed 100644 --- a/src/am_super/state_mediator.cpp +++ b/src/am_super/state_mediator.cpp @@ -2,15 +2,7 @@ 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 - * and ability to test. - * - * This class is stateless, but non-static allowing extension to change behavior - * if different deployments warrant different behavior. - */ + StateMediator::StateMediator(){ } From 0d6829240f845be55f37ad7fc6a91dcccc164578 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Thu, 25 Jun 2020 06:18:52 -0700 Subject: [PATCH 08/36] build: finding all lib cpp files PL-629/state-mediator --- CMakeLists.txt | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e2735e8e..53ad0fc9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -109,9 +109,14 @@ include_directories( ${catkin_INCLUDE_DIRS} ) + +file(GLOB super_lib_cpp_files + src/super_lib/*.cpp +) + ## Declare a C++ library add_library(super_lib - src/super_lib/am_life_cycle.cpp + ${super_lib_cpp_files} ) target_link_libraries(super_lib ${catkin_LIBRARIES}) add_dependencies(super_lib ${catkin_EXPORTED_TARGETS}) From 368742310ba432da1dd7a855e7043afd3dd606a8 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Thu, 25 Jun 2020 06:23:11 -0700 Subject: [PATCH 09/36] build: finding all cuda files PL-629/state-mediator --- CMakeLists.txt | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 53ad0fc9..ca2dea09 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -114,6 +114,11 @@ file(GLOB super_lib_cpp_files src/super_lib/*.cpp ) +file(GLOB cuda_cpp_files + src/cuda/*.cpp +) + + ## Declare a C++ library add_library(super_lib ${super_lib_cpp_files} @@ -122,7 +127,7 @@ target_link_libraries(super_lib ${catkin_LIBRARIES}) add_dependencies(super_lib ${catkin_EXPORTED_TARGETS}) if(CUDA_FOUND) - add_executable(am_super src/am_super/am_super.cpp src/cuda/cuda_utility_class.cpp src/am_super/state_mediator.cpp) + add_executable(am_super src/am_super/am_super.cpp ${cuda_cpp_files} src/am_super/state_mediator.cpp) target_link_libraries(am_super ${catkin_LIBRARIES} super_lib cuda_utility) else() add_executable(am_super src/am_super/am_super.cpp src/am_super/state_mediator.cpp) From 7c38c5315aad1150cfc99cd063d27fd19d8facf5 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Thu, 25 Jun 2020 06:32:01 -0700 Subject: [PATCH 10/36] ci: delay code format until ready for review PL-629/state-mediator --- .github/workflows/format.yml | 14 ++++++++++++++ .github/workflows/package.yml | 9 --------- 2 files changed, 14 insertions(+), 9 deletions(-) create mode 100644 .github/workflows/format.yml diff --git a/.github/workflows/format.yml b/.github/workflows/format.yml new file mode 100644 index 00000000..013f4f72 --- /dev/null +++ b/.github/workflows/format.yml @@ -0,0 +1,14 @@ +name: Code Format +on: + pull_request: + types: + - ready_for_review +jobs: + code-format: + runs-on: ubuntu-18.04 + steps: + - uses: actions/checkout@v2 + - name: Format Code + uses: AutoModality/action-ros-clang-format@master + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/package.yml b/.github/workflows/package.yml index a849f138..91a8cba9 100644 --- a/.github/workflows/package.yml +++ b/.github/workflows/package.yml @@ -60,12 +60,3 @@ 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.GITHUB_TOKEN }} \ No newline at end of file From cb63f9d6d13aaf09b146b45a0c925069998725e6 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Thu, 25 Jun 2020 06:55:47 -0700 Subject: [PATCH 11/36] ci: using dispatch to format code on PR PL-629/state-mediator --- .github/workflows/format.yml | 7 +++---- .github/workflows/ready.yml | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/ready.yml diff --git a/.github/workflows/format.yml b/.github/workflows/format.yml index 013f4f72..0df38e2f 100644 --- a/.github/workflows/format.yml +++ b/.github/workflows/format.yml @@ -1,8 +1,7 @@ name: Code Format -on: - pull_request: - types: - - ready_for_review +on: + repository_dispatch: + types: [ready-for-review] jobs: code-format: runs-on: ubuntu-18.04 diff --git a/.github/workflows/ready.yml b/.github/workflows/ready.yml new file mode 100644 index 00000000..2ef568b6 --- /dev/null +++ b/.github/workflows/ready.yml @@ -0,0 +1,15 @@ +name: Ready for Review +on: + pull_request: + types: + - ready_for_review +jobs: + notify-ready: + runs-on: ubuntu-18.04 + steps: + - name: Notify Code Format + uses: peter-evans/repository-dispatch@v1 + with: + token: ${{ secrets.AMGITBOT_PAT }} + repository: ${{ github.repository }} + event-type: ready-for-release \ No newline at end of file From a59e040a7ff5fdc09a22aeb2420de4ee13005b0d Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Thu, 25 Jun 2020 07:49:10 -0700 Subject: [PATCH 12/36] build: removing node from test code to avoid multiple main PL-629/state-mediator --- CMakeLists.txt | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index ca2dea09..c51bb246 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -118,6 +118,10 @@ file(GLOB cuda_cpp_files src/cuda/*.cpp ) +file(GLOB am_super_cpp_files + src/am_super/*.cpp +) + ## Declare a C++ library add_library(super_lib @@ -127,10 +131,10 @@ target_link_libraries(super_lib ${catkin_LIBRARIES}) add_dependencies(super_lib ${catkin_EXPORTED_TARGETS}) if(CUDA_FOUND) - add_executable(am_super src/am_super/am_super.cpp ${cuda_cpp_files} src/am_super/state_mediator.cpp) + add_executable(am_super ${am_super_cpp_files} ${cuda_cpp_files} ) target_link_libraries(am_super ${catkin_LIBRARIES} super_lib cuda_utility) else() - add_executable(am_super src/am_super/am_super.cpp src/am_super/state_mediator.cpp) + add_executable(am_super ${am_super_cpp_files}) target_link_libraries(am_super ${catkin_LIBRARIES} super_lib) endif(CUDA_FOUND) add_dependencies(am_super ${catkin_EXPORTED_TARGETS}) @@ -182,6 +186,7 @@ file(GLOB TEST_FILES ) ## Add gtest based cpp test target and link libraries +list(REMOVE_ITEM am_super_cpp_files src/am_super/am_super.cpp ) catkin_add_gtest(${PROJECT_NAME}_test ${TEST_FILES} src/am_super/state_mediator.cpp) if(TARGET ${PROJECT_NAME}_test) target_link_libraries(${PROJECT_NAME}_test ${catkin_LIBRARIES}) From 8d65f525a67ac4acb9c0fa61488059d00b294783 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Thu, 25 Jun 2020 09:50:27 -0700 Subject: [PATCH 13/36] test: confirming not allowed PL-629/state-mediator --- include/am_super/state_mediator.h | 5 ++++ src/am_super/state_mediator.cpp | 11 +++++++ test/state_unit_tests.cpp | 50 +++++++++++++++++++++++++++++-- 3 files changed, 63 insertions(+), 3 deletions(-) diff --git a/include/am_super/state_mediator.h b/include/am_super/state_mediator.h index 720eb376..2fec062e 100644 --- a/include/am_super/state_mediator.h +++ b/include/am_super/state_mediator.h @@ -26,6 +26,11 @@ 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 + */ + std::vector allSuperStates(); private: }; diff --git a/src/am_super/state_mediator.cpp b/src/am_super/state_mediator.cpp index 7f6dbf57..acd32c0f 100644 --- a/src/am_super/state_mediator.cpp +++ b/src/am_super/state_mediator.cpp @@ -90,4 +90,15 @@ bool StateMediator::allowsTransition(SuperState from, SuperState to) } return legal; } + +std::vector StateMediator::allSuperStates() +{ + std::vector all; + for ( int enumIndex = (int) SuperState::OFF; enumIndex <= (int) SuperState::LAST_STATE; enumIndex++ ) + { + SuperState state = static_cast(enumIndex); + all.push_back(state); + } + return all; +} }; diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index e3641964..284c149d 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -6,9 +6,53 @@ using std::string; using namespace am; +StateMediator mediator; + +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); +} + +void +ASSERT_TRANSITIONS_ALLOWED(SuperState from, std::vector states, bool expected) +{ + for(int i = 0; i < states.size(); i++) + { + SuperState state = states.at(i); + ASSERT_TRANSITION_ALLOWED(from,state,expected); + } +} + + TEST(StateMediator, allowsTransition_OffToBootingIsAllowed) { - StateMediator mediator; - bool allowed = mediator.allowsTransition(SuperState::OFF, SuperState::BOOTING); - EXPECT_TRUE(allowed); + ASSERT_TRANSITION_ALLOWED(SuperState::OFF,SuperState::BOOTING,true); +} + + +TEST(StateMediator, allowsTransition_OffToOthersIsNotAllowed) +{ + ASSERT_TRANSITION_ALLOWED(SuperState::OFF,SuperState::ABORT,false); + std::vector notAllowed = mediator.allSuperStates(); + notAllowed.erase(notAllowed.begin() + (int) SuperState::BOOTING) ; + ASSERT_TRANSITIONS_ALLOWED(SuperState::OFF,notAllowed,false); +} + + +TEST(StateMediator, allSuperStates_IncludesAll) +{ + std::vector all = mediator.allSuperStates(); + 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); } From 876c7659cce2b3ab0f13e8823b13fd4a283dfe9b Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Thu, 25 Jun 2020 11:02:56 -0700 Subject: [PATCH 14/36] test: added booting to ready test PL-629/state-mediator --- test/sample_tests.cpp | 13 ------------- test/state_unit_tests.cpp | 13 ++++++++----- 2 files changed, 8 insertions(+), 18 deletions(-) delete mode 100644 test/sample_tests.cpp diff --git a/test/sample_tests.cpp b/test/sample_tests.cpp deleted file mode 100644 index 4f8deacd..00000000 --- a/test/sample_tests.cpp +++ /dev/null @@ -1,13 +0,0 @@ -#include // googletest header file - -#include -using std::string; - -const char* actualValTrue = "hello gtest"; -const char* actualValFalse = "hello world"; -const char* expectVal = "hello gtest"; - -TEST(StrCompare, CStrEqual) -{ - EXPECT_STREQ(expectVal, actualValTrue); -} diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 284c149d..97fa253d 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -26,18 +26,21 @@ ASSERT_TRANSITIONS_ALLOWED(SuperState from, std::vector states, bool } -TEST(StateMediator, allowsTransition_OffToBootingIsAllowed) +TEST(StateMediator, allowsTransition_OnlyOffToBootingIAllowed) { ASSERT_TRANSITION_ALLOWED(SuperState::OFF,SuperState::BOOTING,true); + std::vector notAllowed = mediator.allSuperStates(); + notAllowed.erase(notAllowed.begin() + (int) SuperState::BOOTING) ; + ASSERT_TRANSITIONS_ALLOWED(SuperState::OFF,notAllowed,false); } -TEST(StateMediator, allowsTransition_OffToOthersIsNotAllowed) +TEST(StateMediator, allowsTransition_OnlyBootingToReadyIsAllowed) { - ASSERT_TRANSITION_ALLOWED(SuperState::OFF,SuperState::ABORT,false); + ASSERT_TRANSITION_ALLOWED(SuperState::BOOTING,SuperState::READY,true); std::vector notAllowed = mediator.allSuperStates(); - notAllowed.erase(notAllowed.begin() + (int) SuperState::BOOTING) ; - ASSERT_TRANSITIONS_ALLOWED(SuperState::OFF,notAllowed,false); + notAllowed.erase(notAllowed.begin() + (int) SuperState::READY); + ASSERT_TRANSITIONS_ALLOWED(SuperState::BOOTING,notAllowed,false); } From f62d09564f75778a55d9992e278708d0794bae7e Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Thu, 25 Jun 2020 17:14:38 -0700 Subject: [PATCH 15/36] test: added multiple state tetss PL-629/state-mediator --- test/state_unit_tests.cpp | 55 +++++++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 8 deletions(-) diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 97fa253d..9fbf3bbd 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -26,23 +26,62 @@ ASSERT_TRANSITIONS_ALLOWED(SuperState from, std::vector states, bool } -TEST(StateMediator, allowsTransition_OnlyOffToBootingIAllowed) + +void +ASSERT_MULTIPLE_STATES_ALLOWED(SuperState from, std::vector allowedStates) { - ASSERT_TRANSITION_ALLOWED(SuperState::OFF,SuperState::BOOTING,true); std::vector notAllowed = mediator.allSuperStates(); - notAllowed.erase(notAllowed.begin() + (int) SuperState::BOOTING) ; - ASSERT_TRANSITIONS_ALLOWED(SuperState::OFF,notAllowed,false); + for(int i = 0; i < allowedStates.size(); i++) + { + SuperState to = allowedStates.at(i); + ASSERT_TRANSITION_ALLOWED(from,to,true); + notAllowed.erase(notAllowed.begin() + (int) to); + } + 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) +{ + std::vector allowed{to}; + ASSERT_MULTIPLE_STATES_ALLOWED(from,allowed); } +TEST(StateMediator, allowsTransition_OnlyOffToBootingIAllowed) +{ + ASSERT_SINGLE_STATE_ALLOWED(SuperState::OFF,SuperState::BOOTING); +} TEST(StateMediator, allowsTransition_OnlyBootingToReadyIsAllowed) { - ASSERT_TRANSITION_ALLOWED(SuperState::BOOTING,SuperState::READY,true); - std::vector notAllowed = mediator.allSuperStates(); - notAllowed.erase(notAllowed.begin() + (int) SuperState::READY); - ASSERT_TRANSITIONS_ALLOWED(SuperState::BOOTING,notAllowed,false); + ASSERT_SINGLE_STATE_ALLOWED(SuperState::BOOTING,SuperState::READY); } +TEST(StateMediator, allowsTransition_OnlyReadyToArmingIsAllowed) +{ + ASSERT_SINGLE_STATE_ALLOWED(SuperState::READY,SuperState::ARMING); +} + +TEST(StateMediator, allowsTransition_OnlyArmingToArmedIsAllowed) +{ + 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); +} + +TEST(StateMediator, allowsTransition_ArmedToAutoAndAbortIsAllowed) +{ + std::vector allowed{SuperState::ABORT, SuperState::AUTO}; + ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::ARMED,allowed); +} + + + TEST(StateMediator, allSuperStates_IncludesAll) { From ff82b95463c9effc7c230ac4547a6fc78ae301f2 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Thu, 25 Jun 2020 18:22:30 -0700 Subject: [PATCH 16/36] test: remove duplicate PL-629/state-mediator --- test/state_unit_tests.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 9fbf3bbd..b9ba76c2 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -74,11 +74,6 @@ TEST(StateMediator, allowsTransition_ArmedToAutoAndAbortIsAllowed) ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::ARMED,allowed); } -TEST(StateMediator, allowsTransition_ArmedToAutoAndAbortIsAllowed) -{ - std::vector allowed{SuperState::ABORT, SuperState::AUTO}; - ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::ARMED,allowed); -} From 798db7e5b44e93b7fe6c58146a46310a1e6493c5 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Thu, 25 Jun 2020 18:52:56 -0700 Subject: [PATCH 17/36] docs: added link to state definition on wiki PL-629/state-mediator --- include/am_super/state_mediator.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/am_super/state_mediator.h b/include/am_super/state_mediator.h index 2fec062e..d76af8f7 100644 --- a/include/am_super/state_mediator.h +++ b/include/am_super/state_mediator.h @@ -22,7 +22,7 @@ class StateMediator public: StateMediator(); - /** + /** 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. */ bool allowsTransition(SuperState from, SuperState to); From 82324747e5a2274041cc16e4c4fe883a7e798ecd Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Fri, 26 Jun 2020 08:09:33 -0700 Subject: [PATCH 18/36] test: added more tests. Seg fault issues locally with initializing multiple values in vector PL-629/state-mediator --- src/am_super/state_mediator.cpp | 2 +- test/state_unit_tests.cpp | 51 +++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/am_super/state_mediator.cpp b/src/am_super/state_mediator.cpp index acd32c0f..5bc5a677 100644 --- a/src/am_super/state_mediator.cpp +++ b/src/am_super/state_mediator.cpp @@ -85,7 +85,7 @@ bool StateMediator::allowsTransition(SuperState from, SuperState to) break; default: { - throw std::invalid_argument("Unhandled state "); + throw std::invalid_argument("Unhandled state"); } } return legal; diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index b9ba76c2..8d3cd7ee 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -77,6 +77,55 @@ TEST(StateMediator, allowsTransition_ArmedToAutoAndAbortIsAllowed) +TEST(StateMediator, allowsTransition_HoldToExceptionStates) +{ + std::vector allowed{SuperState::ABORT, SuperState::MANUAL}; + + ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::HOLD,allowed); +} + +TEST(StateMediator, allowsTransition_AbortToReadyAndManualAllowed) +{ + std::vector allowed{SuperState::READY,SuperState::MANUAL}; + + ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::ABORT,allowed); +} + +TEST(StateMediator, allowsTransition_OnlyManualToReadyIsAllowed) +{ + ASSERT_SINGLE_STATE_ALLOWED(SuperState::MANUAL,SuperState::READY); +} + +TEST(StateMediator, allowsTransition_ShutdownToOffAllowed) +{ + ASSERT_SINGLE_STATE_ALLOWED(SuperState::SHUTDOWN,SuperState::OFF); +} + +TEST(StateMediator, allowsTransition_UnhandledThrowsException) +{ + FAIL() << "JUst testing"; +} + + +TEST(StateMediator, allowsTransition_AutoToManyAllowed) +{ + std::vector allowed{SuperState::READY, SuperState::SEMI_AUTO, + SuperState::HOLD, SuperState::ABORT, + SuperState::MANUAL}; + + ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::AUTO,allowed); +} +TEST(StateMediator, allowsTransition_SemiAutoToManyAllowed) +{ + std::vector allowed{SuperState::AUTO, SuperState::HOLD, SuperState::ABORT, + SuperState::MANUAL}; + + ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::SEMI_AUTO,allowed); +} + + + + TEST(StateMediator, allSuperStates_IncludesAll) { @@ -92,4 +141,6 @@ TEST(StateMediator, allSuperStates_IncludesAll) 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 9127c6e4dd06d7cf79e7658e120e6f6fa4302b87 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Fri, 26 Jun 2020 08:25:20 -0700 Subject: [PATCH 19/36] build: removing test failure ignore PL-629/state-mediator --- debian/rules | 7 ------- 1 file changed, 7 deletions(-) diff --git a/debian/rules b/debian/rules index ae028431..f76ffa31 100755 --- a/debian/rules +++ b/debian/rules @@ -38,13 +38,6 @@ override_dh_auto_build: if [ -f "/opt/ros/melodic/setup.sh" ]; then . "/opt/ros/melodic/setup.sh"; fi && \ dh_auto_build -override_dh_auto_test: - # In case we're installing to a non-standard location, look for a setup.sh - # in the install tree that was dropped by catkin, and source it. It will - # set things like CMAKE_PREFIX_PATH, PKG_CONFIG_PATH, and PYTHONPATH. - echo -- Running tests. Even if one of them fails the build is not canceled. - if [ -f "/opt/ros/melodic/setup.sh" ]; then . "/opt/ros/melodic/setup.sh"; fi && \ - dh_auto_test || true override_dh_shlibdeps: # In case we're installing to a non-standard location, look for a setup.sh From 6395d5872988dd3d6c02d658a27c15f59c1bf8ce Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Fri, 26 Jun 2020 08:36:09 -0700 Subject: [PATCH 20/36] ci: added test override back because it has the same behavior PL-629/state-mediator --- debian/rules | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/debian/rules b/debian/rules index f76ffa31..ae028431 100755 --- a/debian/rules +++ b/debian/rules @@ -38,6 +38,13 @@ override_dh_auto_build: if [ -f "/opt/ros/melodic/setup.sh" ]; then . "/opt/ros/melodic/setup.sh"; fi && \ dh_auto_build +override_dh_auto_test: + # In case we're installing to a non-standard location, look for a setup.sh + # in the install tree that was dropped by catkin, and source it. It will + # set things like CMAKE_PREFIX_PATH, PKG_CONFIG_PATH, and PYTHONPATH. + echo -- Running tests. Even if one of them fails the build is not canceled. + if [ -f "/opt/ros/melodic/setup.sh" ]; then . "/opt/ros/melodic/setup.sh"; fi && \ + dh_auto_test || true override_dh_shlibdeps: # In case we're installing to a non-standard location, look for a setup.sh From 9be5a0ae0c18c2dc238b2bca1ff51b7322caad56 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Fri, 26 Jun 2020 08:58:18 -0700 Subject: [PATCH 21/36] test: avoids seg fault PL-629/state-mediator --- test/state_unit_tests.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 8d3cd7ee..e60aef2f 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -109,16 +109,14 @@ TEST(StateMediator, allowsTransition_UnhandledThrowsException) TEST(StateMediator, allowsTransition_AutoToManyAllowed) { - std::vector allowed{SuperState::READY, SuperState::SEMI_AUTO, - SuperState::HOLD, SuperState::ABORT, - SuperState::MANUAL}; + std::vector allowed{SuperState::READY, SuperState::SEMI_AUTO}; + allowed.push_back(SuperState::HOLD); 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}; ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::SEMI_AUTO,allowed); } From 9540e5cce837e35a0191aceca737e43e9e2f26ee Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Fri, 3 Jul 2020 09:56:45 -0700 Subject: [PATCH 22/36] test: verifying exception is thrown PL-629/state-mediator --- test/state_unit_tests.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index e60aef2f..feb25165 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -103,7 +103,17 @@ TEST(StateMediator, allowsTransition_ShutdownToOffAllowed) TEST(StateMediator, allowsTransition_UnhandledThrowsException) { - FAIL() << "JUst testing"; + try + { + mediator.allowsTransition(SuperState::LAST_STATE,SuperState::OFF); + FAIL() << "Expected expection since last state not handled"; + + } + catch(const std::exception& e) + { + //expected. + } + } @@ -121,10 +131,6 @@ TEST(StateMediator, allowsTransition_SemiAutoToManyAllowed) ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::SEMI_AUTO,allowed); } - - - - TEST(StateMediator, allSuperStates_IncludesAll) { std::vector all = mediator.allSuperStates(); From 29bcc103c967c4e8bb7138fc44e2c2326deb7786 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Fri, 3 Jul 2020 10:47:42 -0700 Subject: [PATCH 23/36] test: added comments PL-629/state-mediator --- test/state_unit_tests.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index feb25165..3076135a 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -8,6 +8,7 @@ using namespace am; StateMediator mediator; +/**Re-usable test method for validating single states*/ void ASSERT_TRANSITION_ALLOWED(SuperState from, SuperState to, bool expected) { @@ -15,6 +16,7 @@ ASSERT_TRANSITION_ALLOWED(SuperState from, SuperState to, bool expected) 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) { @@ -26,7 +28,9 @@ ASSERT_TRANSITIONS_ALLOWED(SuperState from, std::vector states, bool } - +/**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) { @@ -75,8 +79,6 @@ TEST(StateMediator, allowsTransition_ArmedToAutoAndAbortIsAllowed) } - - TEST(StateMediator, allowsTransition_HoldToExceptionStates) { std::vector allowed{SuperState::ABORT, SuperState::MANUAL}; @@ -101,6 +103,8 @@ 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 @@ -131,6 +135,7 @@ TEST(StateMediator, allowsTransition_SemiAutoToManyAllowed) ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::SEMI_AUTO,allowed); } +/**Basic validation of super state enumeration */ TEST(StateMediator, allSuperStates_IncludesAll) { std::vector all = mediator.allSuperStates(); From 383c63014c8f577c4503d650d49c380e87e0d912 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Fri, 3 Jul 2020 11:16:11 -0700 Subject: [PATCH 24/36] test: array initialization avoid seg fault PL-629/state-mediator --- test/state_unit_tests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 3076135a..1d20d459 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -123,8 +123,8 @@ TEST(StateMediator, allowsTransition_UnhandledThrowsException) TEST(StateMediator, allowsTransition_AutoToManyAllowed) { - std::vector allowed{SuperState::READY, SuperState::SEMI_AUTO}; - allowed.push_back(SuperState::HOLD); + std::array allowedArray{SuperState::HOLD,SuperState::ABORT,SuperState::READY,SuperState::SEMI_AUTO}; + std::vector allowed(allowedArray.begin(),allowedArray.end()); ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::AUTO,allowed); } From 12c7c6d86da8cf219dfc3286dc9f1a3687dc5264 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Sat, 4 Jul 2020 08:13:18 -0700 Subject: [PATCH 25/36] test: avoiding seg fault PL-629/state-mediator --- test/state_unit_tests.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 1d20d459..8e2f10f7 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -39,7 +39,7 @@ ASSERT_MULTIPLE_STATES_ALLOWED(SuperState from, std::vector allowedS { SuperState to = allowedStates.at(i); ASSERT_TRANSITION_ALLOWED(from,to,true); - notAllowed.erase(notAllowed.begin() + (int) to); + //notAllowed.erase(notAllowed.begin() + (int) to); } ASSERT_TRANSITIONS_ALLOWED(from,notAllowed,false); } @@ -123,8 +123,7 @@ TEST(StateMediator, allowsTransition_UnhandledThrowsException) TEST(StateMediator, allowsTransition_AutoToManyAllowed) { - std::array allowedArray{SuperState::HOLD,SuperState::ABORT,SuperState::READY,SuperState::SEMI_AUTO}; - std::vector allowed(allowedArray.begin(),allowedArray.end()); + std::vector allowed{SuperState::HOLD,SuperState::ABORT,SuperState::READY,SuperState::SEMI_AUTO, SuperState::MANUAL}; ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::AUTO,allowed); } From 825ca45f45fe5d3d6e46c4b1b0148dee29c1da0d Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Sun, 5 Jul 2020 18:45:31 -0700 Subject: [PATCH 26/36] test: re-enabling failing line now that vscode debugger is working PL-629/state-mediator --- test/state_unit_tests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index 8e2f10f7..d1d06715 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -39,7 +39,7 @@ ASSERT_MULTIPLE_STATES_ALLOWED(SuperState from, std::vector allowedS { SuperState to = allowedStates.at(i); ASSERT_TRANSITION_ALLOWED(from,to,true); - //notAllowed.erase(notAllowed.begin() + (int) to); + notAllowed.erase(notAllowed.begin() + (int) to); } ASSERT_TRANSITIONS_ALLOWED(from,notAllowed,false); } @@ -48,7 +48,7 @@ ASSERT_MULTIPLE_STATES_ALLOWED(SuperState from, std::vector allowedS void ASSERT_SINGLE_STATE_ALLOWED(SuperState from, SuperState to) { - std::vector allowed{to}; + std::vector allowed{to}; ASSERT_MULTIPLE_STATES_ALLOWED(from,allowed); } From 21787d4a5d948c2cf2d49514e4649b9a969479f9 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Sun, 5 Jul 2020 19:05:48 -0700 Subject: [PATCH 27/36] test: fixed seg fault using vector erase incorrectly PL-629/state-mediator --- test/state_unit_tests.cpp | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/test/state_unit_tests.cpp b/test/state_unit_tests.cpp index d1d06715..43b8ea9a 100644 --- a/test/state_unit_tests.cpp +++ b/test/state_unit_tests.cpp @@ -34,13 +34,24 @@ ASSERT_TRANSITIONS_ALLOWED(SuperState from, std::vector states, bool void ASSERT_MULTIPLE_STATES_ALLOWED(SuperState from, std::vector allowedStates) { - std::vector notAllowed = mediator.allSuperStates(); + //verify allowed as expected for(int i = 0; i < allowedStates.size(); i++) { SuperState to = allowedStates.at(i); ASSERT_TRANSITION_ALLOWED(from,to,true); - notAllowed.erase(notAllowed.begin() + (int) to); } + + + //not allowed is all states minus those allowed + std::vector notAllowed; + for(SuperState state: mediator.allSuperStates()) + { + if (std::find(allowedStates.begin(), allowedStates.end(), state) == allowedStates.end()) + { + notAllowed.push_back(state); + } + } + ASSERT_TRANSITIONS_ALLOWED(from,notAllowed,false); } @@ -109,7 +120,8 @@ TEST(StateMediator, allowsTransition_UnhandledThrowsException) { try { - mediator.allowsTransition(SuperState::LAST_STATE,SuperState::OFF); + int someBadNumber=999999; + mediator.allowsTransition((SuperState)someBadNumber,SuperState::OFF); FAIL() << "Expected expection since last state not handled"; } @@ -129,7 +141,7 @@ TEST(StateMediator, allowsTransition_AutoToManyAllowed) } TEST(StateMediator, allowsTransition_SemiAutoToManyAllowed) { - std::vector allowed{SuperState::AUTO, SuperState::HOLD, SuperState::ABORT}; + std::vector allowed{SuperState::AUTO, SuperState::HOLD, SuperState::ABORT, SuperState::MANUAL}; ASSERT_MULTIPLE_STATES_ALLOWED(SuperState::SEMI_AUTO,allowed); } From 5a186ad4afdc86a7d928323f09b2bf68d6b5798e Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Sun, 5 Jul 2020 19:24:07 -0700 Subject: [PATCH 28/36] ci: added code coverage (I hope) PL-629/state-mediator --- .github/workflows/package.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/package.yml b/.github/workflows/package.yml index 91a8cba9..0dffef0e 100644 --- a/.github/workflows/package.yml +++ b/.github/workflows/package.yml @@ -49,6 +49,9 @@ jobs: with: name: ${{ steps.xunit-viewer.outputs.report-name }} path: ${{ steps.xunit-viewer.outputs.report-dir }} + - uses: codecov/codecov-action@v1 + with: + token: ${{ secrets.CODECOV_TOKEN }} # not required for public repos draft-pr: runs-on: ubuntu-18.04 From b628ca268625e89595bf00d9c0a014af6aeebda1 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Sun, 5 Jul 2020 21:13:56 -0700 Subject: [PATCH 29/36] ci: added code coverage, but it is not working PL-629/state-mediator --- CMakeLists.txt | 32 ++++++++++++++++++++++++++------ package.xml | 2 +- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c51bb246..bf02590c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -181,13 +181,33 @@ install(DIRECTORY include/super_lib ## Testing ## ############# +if(CATKIN_ENABLE_TESTING AND ENABLE_COVERAGE_TESTING) + find_package(code_coverage REQUIRED) + # Add compiler flags for coverage instrumentation before defining any targets + APPEND_COVERAGE_COMPILER_FLAGS() +endif() + file(GLOB TEST_FILES test/*_tests.cpp ) -## Add gtest based cpp test target and link libraries -list(REMOVE_ITEM am_super_cpp_files src/am_super/am_super.cpp ) -catkin_add_gtest(${PROJECT_NAME}_test ${TEST_FILES} src/am_super/state_mediator.cpp) -if(TARGET ${PROJECT_NAME}_test) - target_link_libraries(${PROJECT_NAME}_test ${catkin_LIBRARIES}) -endif() + +# code coverage setup: https://github.com/mikeferguson/code_coverage +if (CATKIN_ENABLE_TESTING) + + ## Add gtest based cpp test target and link libraries + list(REMOVE_ITEM am_super_cpp_files src/am_super/am_super.cpp ) + catkin_add_gtest(${PROJECT_NAME}_test ${TEST_FILES} src/am_super/state_mediator.cpp) + if(TARGET ${PROJECT_NAME}_test) + target_link_libraries(${PROJECT_NAME}_test ${catkin_LIBRARIES}) + endif() + + # Create a target ${PROJECT_NAME}_coverage_report + if(ENABLE_COVERAGE_TESTING) + set(COVERAGE_EXCLUDES "*/${PROJECT_NAME}/test*" ) + add_code_coverage( + NAME ${PROJECT_NAME}_coverage_report + DEPENDENCIES tests + ) + endif() +endif() \ No newline at end of file diff --git a/package.xml b/package.xml index f258c661..b05b8d33 100644 --- a/package.xml +++ b/package.xml @@ -21,5 +21,5 @@ std_msgs std_srvs rosunit - + code_coverage From fec1464802b19d182787624e348871c6cd91fa5f Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Mon, 6 Jul 2020 07:57:58 -0700 Subject: [PATCH 30/36] cI: code format not triggered due to misnaming PL-629/state-mediator --- .github/workflows/format.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/format.yml b/.github/workflows/format.yml index 0df38e2f..d3daf498 100644 --- a/.github/workflows/format.yml +++ b/.github/workflows/format.yml @@ -1,7 +1,8 @@ name: Code Format on: + # see ready.yml repository_dispatch: - types: [ready-for-review] + types: [ready-for-release] jobs: code-format: runs-on: ubuntu-18.04 From 436a914922e1d998dcd7e549d5fd74e6720cb6eb Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Mon, 6 Jul 2020 08:01:06 -0700 Subject: [PATCH 31/36] ci: code format on push to master for simplicity github actions is limited with pull requests in the ideal workflow PL-629/state-mediator --- .github/workflows/format.yml | 14 -------------- .github/workflows/ready.yml | 15 --------------- .github/workflows/release.yml | 8 ++++++++ 3 files changed, 8 insertions(+), 29 deletions(-) delete mode 100644 .github/workflows/format.yml delete mode 100644 .github/workflows/ready.yml diff --git a/.github/workflows/format.yml b/.github/workflows/format.yml deleted file mode 100644 index d3daf498..00000000 --- a/.github/workflows/format.yml +++ /dev/null @@ -1,14 +0,0 @@ -name: Code Format -on: - # see ready.yml - repository_dispatch: - types: [ready-for-release] -jobs: - code-format: - runs-on: ubuntu-18.04 - steps: - - uses: actions/checkout@v2 - - name: Format Code - uses: AutoModality/action-ros-clang-format@master - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/ready.yml b/.github/workflows/ready.yml deleted file mode 100644 index 2ef568b6..00000000 --- a/.github/workflows/ready.yml +++ /dev/null @@ -1,15 +0,0 @@ -name: Ready for Review -on: - pull_request: - types: - - ready_for_review -jobs: - notify-ready: - runs-on: ubuntu-18.04 - steps: - - name: Notify Code Format - uses: peter-evans/repository-dispatch@v1 - with: - token: ${{ secrets.AMGITBOT_PAT }} - repository: ${{ github.repository }} - event-type: ready-for-release \ No newline at end of file diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 66e7a8c9..1a14ce2c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -8,6 +8,14 @@ env: RELEASE_PROPERTIES_FILE: release/release.properties RELEASE_PAYLOAD_FILE: release/payload.json jobs: + code-format: + runs-on: ubuntu-18.04 + steps: + - uses: actions/checkout@v2 + - name: Format Code + uses: AutoModality/action-ros-clang-format@master + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} release: runs-on: ${{ matrix.os }} strategy: From f83bb4686abffc1bead189e09f5acb318a663b34 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Mon, 6 Jul 2020 08:06:27 -0700 Subject: [PATCH 32/36] ci: code format after release to avoid new commit semantic release will skip releasing in such cases PL-629/state-mediator --- .github/workflows/release.yml | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1a14ce2c..39feb59b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -8,14 +8,6 @@ env: RELEASE_PROPERTIES_FILE: release/release.properties RELEASE_PAYLOAD_FILE: release/payload.json jobs: - code-format: - runs-on: ubuntu-18.04 - steps: - - uses: actions/checkout@v2 - - name: Format Code - uses: AutoModality/action-ros-clang-format@master - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} release: runs-on: ${{ matrix.os }} strategy: @@ -134,6 +126,16 @@ jobs: distro: 'ubuntu' release: 'bionic' file: '${{ steps.package.outputs.artifact-path }}' + code-format: + runs-on: ubuntu-18.04 + # placing after release to avoid commit to master that will cause semantic-release to skip + needs: release-package + steps: + - uses: actions/checkout@v2 + - name: Format Code + uses: AutoModality/action-ros-clang-format@master + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} notify-amros: needs: release-package runs-on: ubuntu-latest From efe6f7f21ab65f7fc9f6acb76b571f7c230bb4c4 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Mon, 6 Jul 2020 08:29:12 -0700 Subject: [PATCH 33/36] ci: using gitbot to allow push to master for format master --- .github/workflows/release.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 39feb59b..df4b1abe 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -135,7 +135,7 @@ jobs: - name: Format Code uses: AutoModality/action-ros-clang-format@master env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_TOKEN: ${{ secrets.AMGITBOT_PAT }} notify-amros: needs: release-package runs-on: ubuntu-latest From c9f04eca51bd3c1a226c5866552f9c1d43bae356 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Mon, 6 Jul 2020 09:07:15 -0700 Subject: [PATCH 34/36] ci: removing format code since push is denied on master master --- .github/workflows/release.yml | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index df4b1abe..c12e1fdc 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -125,17 +125,7 @@ jobs: repo: 'release' distro: 'ubuntu' release: 'bionic' - file: '${{ steps.package.outputs.artifact-path }}' - code-format: - runs-on: ubuntu-18.04 - # placing after release to avoid commit to master that will cause semantic-release to skip - needs: release-package - steps: - - uses: actions/checkout@v2 - - name: Format Code - uses: AutoModality/action-ros-clang-format@master - env: - GITHUB_TOKEN: ${{ secrets.AMGITBOT_PAT }} + file: '${{ steps.package.outputs.artifact-path }}' notify-amros: needs: release-package runs-on: ubuntu-latest From 0b98d52695ae6c67afa38096363f9179e03f55da Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Mon, 6 Jul 2020 13:19:54 -0700 Subject: [PATCH 35/36] perf: refactor state transition to use tested code PL-629/apply-state-mediator --- src/am_super/am_super.cpp | 124 +++++++++----------------------------- 1 file changed, 28 insertions(+), 96 deletions(-) diff --git a/src/am_super/am_super.cpp b/src/am_super/am_super.cpp index 29b92e18..32c0ad88 100644 --- a/src/am_super/am_super.cpp +++ b/src/am_super/am_super.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -22,7 +23,6 @@ #include #include #include - #if CUDA_FLAG #include #endif @@ -104,6 +104,9 @@ class AMSuper */ SuperState system_state_; + /** manage logic for SuperState transitions */ + StateMediator state_mediator_; + /** * flight controller state */ @@ -761,107 +764,36 @@ class AMSuper { ROS_INFO_STREAM("request change system state from: " << stateToString(system_state_) << " to: " << stateToString(state)); - bool legal = false; - switch (system_state_) - { - case SuperState::OFF: - if (state == SuperState::BOOTING) - { - legal = true; - system_state_ = state; - } - break; - case SuperState::BOOTING: - if (state == SuperState::READY) - { - ROS_INFO_STREAM("sending CONFIGURE to all nodes"); - legal = true; - sendLifeCycleCommand(AMLifeCycle::BROADCAST_NODE_NAME, LifeCycleCommand::CONFIGURE); - system_state_ = state; - } - break; - case SuperState::READY: - if (state == SuperState::ARMING) - { - ROS_INFO_STREAM("sending ACTIVATE to all nodes"); - legal = true; - sendLifeCycleCommand(AMLifeCycle::BROADCAST_NODE_NAME, LifeCycleCommand::ACTIVATE); - system_state_ = state; - } - else if (state == SuperState::ARMING) - { - legal = true; - system_state_ = state; - } - break; - case SuperState::ARMING: - if (state == SuperState::ARMED) - { - legal = true; - system_state_ = state; - } - break; - case SuperState::ARMED: - // TODO: remove ABORT state here once we know how to deal with arming errors (should go back to READY). - if (state == SuperState::AUTO || state == SuperState::ABORT) - { - legal = true; - system_state_ = state; - } - break; - case SuperState::AUTO: - if (state == SuperState::READY || state == SuperState::SEMI_AUTO || state == SuperState::HOLD || - state == SuperState::ABORT || state == SuperState::MANUAL) - { - legal = true; - system_state_ = state; - } - break; - case SuperState::SEMI_AUTO: - if (state == SuperState::AUTO || state == SuperState::HOLD || state == SuperState::ABORT || - state == SuperState::MANUAL) - { - legal = true; - system_state_ = state; - } - break; - case SuperState::HOLD: - if (state == SuperState::ABORT || state == SuperState::MANUAL) - { - legal = true; - system_state_ = state; - } - break; - case SuperState::ABORT: - if (state == SuperState::READY || state == SuperState::MANUAL) - { - legal = true; - system_state_ = state; - } - break; - case SuperState::MANUAL: - if (state == SuperState::READY) - { - legal = true; - system_state_ = state; - } - system_state_ = state; - break; - case SuperState::SHUTDOWN: - if (state == SuperState::OFF) - { - legal = true; - system_state_ = state; - } - break; - } + bool legal = state_mediator_.allowsTransition(system_state_,state); if (!legal) { - ROS_ERROR_STREAM("illegal state transition"); + ROS_ERROR_STREAM("illegal state transition from " + << stateToString(system_state_) << " to " << stateToString(state)); } else { + //send lifecycle updates for limited state transition + switch(system_state_){ + case SuperState::BOOTING: + if (state == SuperState::READY) + { + ROS_INFO_STREAM("System Ready: sending CONFIGURE to all nodes"); + sendLifeCycleCommand(AMLifeCycle::BROADCAST_NODE_NAME, LifeCycleCommand::CONFIGURE); + } + break; + case SuperState::READY: + if (state == SuperState::ARMING) + { + ROS_INFO_STREAM("sending ACTIVATE to all nodes"); + sendLifeCycleCommand(AMLifeCycle::BROADCAST_NODE_NAME, LifeCycleCommand::ACTIVATE); + } + break; + } + + //persist given state as the new current state + system_state_ = state; + reportSystemState(); sendLEDMessage(); From 89fa839583b26db0d0b53582f122554e132cb443 Mon Sep 17 00:00:00 2001 From: Aaron Roller Date: Mon, 6 Jul 2020 13:25:37 -0700 Subject: [PATCH 36/36] refactor: sending lifecycle when state reached removing duplicate logic of allowed transition PL-629/apply-state-mediator --- src/am_super/am_super.cpp | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/am_super/am_super.cpp b/src/am_super/am_super.cpp index 32c0ad88..9001aced 100644 --- a/src/am_super/am_super.cpp +++ b/src/am_super/am_super.cpp @@ -773,21 +773,15 @@ class AMSuper } else { - //send lifecycle updates for limited state transition - switch(system_state_){ - case SuperState::BOOTING: - if (state == SuperState::READY) - { - ROS_INFO_STREAM("System Ready: sending CONFIGURE to all nodes"); + //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); - } break; - case SuperState::READY: - if (state == SuperState::ARMING) - { + case SuperState::ARMING: ROS_INFO_STREAM("sending ACTIVATE to all nodes"); sendLifeCycleCommand(AMLifeCycle::BROADCAST_NODE_NAME, LifeCycleCommand::ACTIVATE); - } break; }