search

RobotLocomotion / robot-plan-runner

10 stars 4 forks source link

state machine #4

Closed pangtao22 closed 3 years ago

pangtao22 commented 3 years ago

This change is Reviewable

pangtao22 commented 3 years ago
  1. I think we need way more explicit documentation / commenting on things like the class definition. If you want I can take a stab at documenting everything on our next round of PRs.
  2. nit: I wonder if @mpetersen94 agrees, but the coding style was a bit difficult to parse coming from reading Drake code. (not a lot of blank lines between methods, etc.) - we can also reformat later if we decide to change the style.
  3. We might put all the files related to state_manager in a separate folder to make things cleaner :) (plan_manager_state_manager and all the state_error, state_idle, etc files)
  4. Could we also include the client script in this PR?
  1. Agreed. I've tried to document the relationship between classes in the google docs and the creately diagram, but we also need per-function and per-class documentation.
  2. The formatting is done with clang-format. There might be parameters that allow additional spaces between methods, etc. It would be nice to include lint as part of CI. Update: clang-format doesn't seem to adjust white spaces between function declarations. I added white spaces and to documentations to some of the .h files.
  3. Done.
  4. Done.
pangtao22 commented 3 years ago

src/plan_runner/state_running.cc, line 16 at r1 (raw file):

Previously, hjsuh94 (Hyung Ju Suh (Terry)) wrote…
I think the right thing to do is to separate out the part that is starting and stopping the plan into a different method, to avoid it being overloaded with a method that returns the plan. But we can just merge and fix later :) I think it is a nit.

Added a TODO.

pangtao22 commented 3 years ago

Hi Mark, just saw your comments.

I don't love the state machine being split up as a series of classes with one for each state. I'm not sure what this gets us. I think the thought is that we can extend this with more class types but the logic of switching between the states it tied up between them all and as a result the logic of the state machine feels hard to follow with it split up among 11 different files. Am I missing the advantage?

Compared with a centralized blob of if-else statements, delegating the state machine's functions to different State objects allows us to unambiguously specify the behavior of each function in each state. Having state transitions defined in states instead of a centralized location also allows adding new states without altering existing ones. In fact this way of coding up a state machine is what software engineers call the "state design pattern", which is a new trick I learned from this book.

I'm not sure I love locking the plan receiver such that it can only receive new plans when it is not running a plan. I feel like that is more restrictive than necessary and means the only way to make the full stack reactive is to write a bespoke plan that is reactive (and deal with all the plumbing that is required) or use a streaming plan. You can't send a new plan mid-execution to switch to.

We can totally change this behavior if the use case for mid-execution switch arises. What's been implemented is just something simple that makes sense for the basic use cases I can think of.

Parsing the Cmake is a lot easier than most projects so I'm on board with the decision we've made. Glad that it's been simple so far :D