Px4-autopilot: commander rewrite

[dagar]

Commander has become unmanageable. After the next stable release goes out let's decide on a path forward for a rewrite or major restructuring.

@LorenzMeier @bkueng

I believe https://github.com/PX4/Firmware/pull/6750 is the only major commander work in progress.

Relevant - https://github.com/PX4/Firmware/pull/6955

Notes and Ideas

• do a pass and remove everything that doesn't belong in commander
  
  
  
  • define what commander should and shouldn't be responsible for
  
  
• create new commander c++ class using blocklib for params and subscriptions, publications
• subsystem_info still used?
• Find a better home for ROI - see VEHICLE_CMD_DO_SET_ROI
• Calibration switch to matrix and move out of commander - https://github.com/PX4/Firmware/pull/6513
• FW engine failure new home - https://github.com/PX4/Firmware/issues/6958
• Consider generating or verifying the various state machines? https://github.com/PX4/Firmware/issues/2463
• move all handling of manual_control to the stick_mapper. Stick mapper can publish vehicle commands to change modes or arm/disarm.
• write a simple vehicle_command command line app to publish vehicle commands. This replaces commander arm, takeoff, etc
• px4_custom_mode.h and mavlink VEHICLE_MODE_FLAG - only use in mavlink module, which can then publish a vehicle command
• how to handle low priority loop?
• sanitize test commands (gps_failure_cmd, rc_signal_lost_cmd, etc). These should be forcing the regular flag, and not independent
• how can we write meaningful testing?
• kill vtol_fw_permanent_stab
• kill old style switches? https://github.com/PX4/Firmware/issues/6611
• should vehicle_control_mode_s flags be an array?
• commander should be the definitive source of supported commands
• bring all preflight checking together encapsulated in a new class. The commander main loop shouldn't be looking at individual sensors, etc
• use event interface - http://discuss.px4.io/t/event-interface/2318
• does commander need to publish the first mission_state?
• handle led and buzzer in events?
• review hotplug
• significantly better error reporting for denied transitions. This is a huge source of confusion for new users. Messages need to be actionable.
• properly track previous main_state. https://github.com/PX4/Firmware/issues/7947

Issue Tracker

• [ ] move calibration out of commander
• [ ] move all stick mode mapping to stick_mapper (use vehicle_commands to change mode or arm)
• [ ] geofence handle in navigator (publish mode changes for violation)
• [x] create new commander c++ class using blocklib for params and subscriptions, publications @dagar #7517
• [ ] expand preflight_sensor and remove direct sensor subscription from commander
• [x] Find a better home for ROI - see VEHICLE_CMD_DO_SET_ROI
• [ ] preflight refactor, this alone is a massive task
• [ ] review all status_flags_s (some are filled improperly, some aren't used, some should be used)
• [ ] move FW engine failure detection to the FW attitude controller
• [ ] move px4_custom_mode.h and mavlink VEHICLE_MODE_FLAG to mavlink module
• [ ] explore state machine generation options @dagar (https://github.com/PX4/Firmware/issues/10584)
• [ ] centralize and simplify system_power and battery status for preflight checks, arming, failsafes, etc. Commander should only have a few overall status flags to check @dagar @davids5
• [ ] land detector can print it's own status

[priseborough]

In addition to the above notes and ideas:

• A specification for the intended state machine logic is created and reviewed before we start coding.
• Documentation is significantly improved on current practice. All class variables should be documented. The definitions of variables used for navigation states and transition events are particularly important.
• Transition events are represented at the top level using a single struct. This can be comprised of smaller structs to separate different types of events - RC, failsafe, etc. The use of a unions with flags and integer bitmask will be efficient with storage.

[mcharleb]

+1 for "A specification for the intended state machine logic is created and reviewed before we start coding"

[bkueng]

It's nice to see that you're pushing this forward @dagar

Let me add some additional points/remarks:

• abstract the gps position to just has_position. It should not matter what the position source is (gps, vision, ...), as much as that is possible.
• get rid of the low prio thread entirely: move sensor calibration into the events module.
• as already mentioned, use something formal for the state machine (#2463). I know that @lovettchris has experimented with the P language (and published an interesting video), so you're input here would be valuable.
  It would be cool to have an automated way to generate state transition diagrams from source (or the other way around). This would ensure the docs always reflects the state of the code.

[ChristophTobler]

• abstract the gps position to just has_position. It should not matter what the position source is (gps, vision, ...), as much as that is possible.

this is very important for the future, I think.

[LorenzMeier]

State machine testing: https://github.com/PX4/Firmware/tree/master/src/modules/commander/commander_tests

[dagar]

Update for anyone interested in this.

There's likely never going to be a full "commander rewrite", but gradually we are getting the complexity under control. The biggest change I want to push for this year is finding some kind of framework or library for handling the state machine (https://github.com/PX4/Firmware/issues/10584).

[davids5]

@dagar how about ESTELLE https://books.google.com/books?id=-OJ6CwAAQBAJ&pg=PA313&lpg=PA313&dq=estel+state+transition&source=bl&ots=-m7VzTVeGG&sig=ACfU3U3UsKzhqPvd6-J213YKpctpNGNpAA&hl=en&sa=X&ved=2ahUKEwjV--nmgrngAhUUJDQIHThyDUAQ6AEwAHoECAIQAQ#v=onepage&q=estel%20state%20transition&f=false

Or is that too old-school?

[davids5]

Here is a more complete reference http://www-lor.int-evry.fr/idemcop/uk/est-lang/download/estelle-tutorial-slides.pdf