Skip to content
This repository was archived by the owner on Aug 1, 2024. It is now read-only.

Finite State Machine library #418

Merged
merged 24 commits into from
Jul 3, 2019
Merged

Finite State Machine library #418

merged 24 commits into from
Jul 3, 2019

Conversation

jkflying
Copy link
Contributor

@jkflying jkflying commented Jun 25, 2019

This PR introduces an example implementation of a Finite State Machine library. It has a few desirable features:

  1. All state transitions are decided at compile time, and compile down to nested switches. No dynamic memory allocations, RTTI or anything else with undefined performance or unsuitable for embedded usage.
  2. Allows both explicit and implicit error handling.
  3. All of the definitions for the state transitions are stored in a single place. The code just chooses which of the available transitions to use.
  4. We can easily generate a .dot file for each FSM with the help of a simple Python script.

This is generated from the FSM library's own unit tests:
unit_tests_graph
high res version

The second part of the PR was porting the waypoint_generator_node in the safe_landing_planner to use this, as a real-world example.
waypoint_generator_node_graph
high res version

This still needs further SITL testing to make sure nothing has broken, and then additional unit tests for the safe_landing_planner waypoint_generator_node, which wasn't tested until now.

FYI @dagar @Jaeyoung-Lim

TODO:

  • Unit tests for waypoint_generator
  • SITL test safe_landing_planner
  • Flight test

@jkflying jkflying changed the title FSM library Finite State Machine library Jun 25, 2019
@dagar dagar self-requested a review June 25, 2019 12:43
@jkflying
Copy link
Contributor Author

@julianoes I forgot to tag you earlier, please take a look around (particularly the test_usm.cpp) and see if there is something here that would be useful for you.

@dagar
Copy link
Member

dagar commented Jul 1, 2019

Let me know if/when you need more testing capacity and we can talk about moving from Travis-ci to Jenkins (http://ci.px4.io:8080/blue/organizations/jenkins/PX4%2FFirmware/detail/master/1082/pipeline).

At times it can be more annoying than travis, but already has drastically more resources (50+ slaves) and can easily dynamically provision anything else from ec2 as needed.

Copy link
Collaborator

@nicovanduijn nicovanduijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty big PR, I just looked at the first half for now

if(TARGET ${PROJECT_NAME}-test-roscore)
target_link_libraries(${PROJECT_NAME}-test-roscore ${PROJECT_NAME}
if(TARGET ${PROJECT_NAME}-test-roscore)
target_link_libraries(${PROJECT_NAME}-test-roscore ${PROJECT_NAME}
${catkin_LIBRARIES}
${YAML_CPP_LIBRARIES})
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did these whitespace changes get in through autoformat? My clang-format doesn't check cmake files, which is why I'm curious. In terms of keeping this huge PR somewhat reviewable, consider getting this fluff out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just annoying me when I was looking into adding roscore-style tests to the safe_landing_planner. They didn't end up being added, but I felt that the reformatting I did would still be useful.


protected:
// config
float yaw_setpoint_ = NAN;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I just spend an enormous amount of time debugging different conventions for yaw, can we specify (at least in comments here, preferably in the variable name) the frame and the units? (I'm guessing here it's fcu frame and degrees, so maybe yaw_setpoint_fcu_deg_?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in the local frame in radians, it seems we don't have the same complications here that we would in the local_planner because we don't have a histogram.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, but units might make sense still

Eigen::Vector3f velocity_setpoint_ = Eigen::Vector3f(NAN, NAN, NAN);
Eigen::Vector3f loiter_position_ = Eigen::Vector3f(NAN, NAN, NAN);
Eigen::Vector3f exploration_anchor_ = Eigen::Vector3f(NAN, NAN, NAN);
Eigen::Vector3f vel_sp = Eigen::Vector3f(NAN, NAN, NAN);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a vel_sp and a velocity_setpoint_... Smells like debug code that wasn't properly cleaned up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkflying jkflying merged commit b18c090 into master Jul 3, 2019
@jkflying jkflying deleted the fsm-library branch July 3, 2019 10:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants