diff --git a/.github/workflows/package.yml b/.github/workflows/package.yml index a849f138..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 @@ -60,12 +63,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 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 66e7a8c9..c12e1fdc 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -125,7 +125,7 @@ jobs: repo: 'release' distro: 'ubuntu' release: 'bionic' - file: '${{ steps.package.outputs.artifact-path }}' + file: '${{ steps.package.outputs.artifact-path }}' notify-amros: needs: release-package runs-on: ubuntu-latest diff --git a/CMakeLists.txt b/CMakeLists.txt index 6a6c4de7..bf02590c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -109,18 +109,32 @@ include_directories( ${catkin_INCLUDE_DIRS} ) + +file(GLOB super_lib_cpp_files + src/super_lib/*.cpp +) + +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 - 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}) if(CUDA_FOUND) - add_executable(am_super src/am_super/am_super.cpp src/cuda/cuda_utility_class.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) + 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}) @@ -167,8 +181,33 @@ install(DIRECTORY include/super_lib ## Testing ## ############# -## Add gtest based cpp test target and link libraries -catkin_add_gtest(${PROJECT_NAME}_test test/sample_tests.cpp) -if(TARGET ${PROJECT_NAME}_test) - target_link_libraries(${PROJECT_NAME}_test ${catkin_LIBRARIES}) +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 +) + + +# 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/README.md b/README.md index 3cdfe997..ee5c18d0 100644 --- a/README.md +++ b/README.md @@ -5,3 +5,19 @@ 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/) + +# 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 + +``` +catkin_make run_tests +``` + +See [test](test) for more. \ No newline at end of file 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 new file mode 100644 index 00000000..d76af8f7 --- /dev/null +++ b/include/am_super/state_mediator.h @@ -0,0 +1,39 @@ + + +#ifndef AM_SUPER_INCLUDE_AM_SUPER_SERVICE_H_ +#define AM_SUPER_INCLUDE_AM_SUPER_SERVICE_H_ + +#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. + */ +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); + + /** + * @return a vector of all states in order of declaration, excluding LastState which is used for enum iteration + */ + std::vector allSuperStates(); + +private: +}; +} + +#endif /* AM_SUPER_INCLUDE_AM_SUPER_SERVICE_H_ */ 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 diff --git a/src/am_super/am_super.cpp b/src/am_super/am_super.cpp index 29b92e18..9001aced 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,30 @@ 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 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::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(); diff --git a/src/am_super/state_mediator.cpp b/src/am_super/state_mediator.cpp new file mode 100644 index 00000000..5bc5a677 --- /dev/null +++ b/src/am_super/state_mediator.cpp @@ -0,0 +1,104 @@ +#include + +namespace am +{ + + 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"); + } + } + 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/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..f574220f --- /dev/null +++ b/test/all_tests.cpp @@ -0,0 +1,7 @@ +#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 deleted file mode 100644 index 67965064..00000000 --- a/test/sample_tests.cpp +++ /dev/null @@ -1,19 +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); -} - -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_unit_tests.cpp b/test/state_unit_tests.cpp new file mode 100644 index 00000000..43b8ea9a --- /dev/null +++ b/test/state_unit_tests.cpp @@ -0,0 +1,166 @@ +#include // googletest header file +#include +#include + +#include +using std::string; +using namespace am; + +StateMediator mediator; + +/**Re-usable test method for validating single states*/ +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); +} + +/**Re-usable test method for validating multiple states behave as expected*/ +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); + } +} + + +/**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) +{ + //verify allowed as expected + for(int i = 0; i < allowedStates.size(); i++) + { + SuperState to = allowedStates.at(i); + ASSERT_TRANSITION_ALLOWED(from,to,true); + } + + + //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); +} + +/**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_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_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); +} + +/** 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_AutoToManyAllowed) +{ + std::vector allowed{SuperState::HOLD,SuperState::ABORT,SuperState::READY,SuperState::SEMI_AUTO, 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); +} + +/**Basic validation of super state enumeration */ +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); + ASSERT_EQ(all.back(),SuperState::LAST_STATE); + +}