Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PX4 Flight Stack Structure Proposal #7318

Closed
Stifael opened this issue May 30, 2017 · 34 comments
Closed

PX4 Flight Stack Structure Proposal #7318

Stifael opened this issue May 30, 2017 · 34 comments

Comments

@Stifael
Copy link
Contributor

Stifael commented May 30, 2017

As in any large project, it is important to not only add new features and fix legacy code, but also to have a common goal in mind in terms of software architecture to make better judgments about future improvements. In addition, it is more than overdue to work on a software diagram of the PX4 project that represents the current and future architecture.

The link below shows a first a example of a software architecture that would simplify future enhancement greatly. It is to note that the diagram given below is a proposal that is not considered to be finished nor static, but rather the PX4 community is encouraged to actively participate in finding a common goal.
Proposal:
https://docs.google.com/drawings/d/1Uw1PmU3hqEfWR3et9KT7zWggDX9wqoAiCt5suiWdnlw/edit

Current:
https://docs.google.com/drawings/d/1oJ6x8HaqiRiW7cl-WFoC24Gn1lOkpjIPyGak5cfryPw/edit?usp=sharing.

I shortly try to describe the difference to the current architecture:

  • free modules from module states. i.e. the position controller does not need to know about commander states nor navigation states.
  • keep the interfaces between modules slim. i.e. the position controller, attitude controller, rate controller and so on only need to have one input, and they all should not care about where the input comes from. the same is true for the navigator: its only purpose is to send out way-points. It should not care about being a fixedwing or mc.
  • all computation should be done in local frame. the navigator should convert all global setpoints first into local frame before doing any mission checks
  • introduce a new module Stick mapper. it basically deals with the stick inputs
  • introduce a new module Trajectory (or whatever name makes sense). This modules fuses different inputs together (from an obstacle avoidance module, navigator module, stick mapper module or from offboard) and has ONLY one set of setpoints as output. The setpoints can either be position/velocity/acceleration setpoints, attitude setpoints or rate setpoints.
  • rate controller should run faster than attitude controller

In addition to this diagram, each module itself should have a similar diagram. For instance, in the current proposal the Trajectory module takes over some of the work of the navigator and position controller. Consequently, the Trajectory module could possibly become large.

Open questions and suggestions:

1.) race condition between two modules which publish the same message:

This can occur for instance if the vehicle is in position control and the attitude controller generates rate controller setpoints, but then the user switches to rate control mode and the the setpoints are directly generated from the stick inputs. During the switch, it can happen that attitude controller module and the trajectory module (or possibly the stick_mapper) send the same setpoints
Solutions:

  • uorb priority

2.) hybrid control

In altitude control the z-position is controlled by the position controller, but roll and pitch setpoints come from the sticks.

3.) separate uorb from modules

Is there a good way to keep the modules separated from uorb such that the interface could possibly be exchanged as well?

4.) trajectory module pipeline

Should geofence and obstacle avoidance be after flight-task (where trajectories are computed) or before flight-task such that no-fly-zones can be incorporated directly into the trajectory generation. The first approach requires some sort of repeated loop (not really desired)

@Stifael
Copy link
Contributor Author

Stifael commented May 30, 2017

commander refactor: #7055
stick_mapper PR: #6439 Issue: #2822
global to local: https://github.com/PX4/Firmware/tree/global_to_local
vtol structure proposal: #4845

@dagar
Copy link
Member

dagar commented May 30, 2017

It's really great to see this written down!

We'll need to review OFFBOARD and the complexity of position_setpoint (https://github.com/PX4/Firmware/blob/master/msg/position_setpoint.msg).

@mcharleb
Copy link
Contributor

This is a great step forward in making the code a bit more approachable for developers.

@Stifael
Copy link
Contributor Author

Stifael commented May 30, 2017

as far as i know, the offboard needs some rework for sure. right now I think the triplets are exploited for offboard...
We were not entirely sure where the offboard setpoints should be handled. this definitely needs farther discussion

@mcharleb
Copy link
Contributor

@Stifael What do you think about platform support layering? Would it help to factor out Linux, QuRT and NuttX support into separate git repos? One of the concerns that I have heard about PX4 is the size of the codebase, but a lot of that is platform support and drivers. Using git submodules makes it difficult to use different collections of layers (NuttX, DriverFramework, etc). Perhaps something like repo would provide a cleaner way to aggregate layers for a particular platform build.

@Stifael
Copy link
Contributor Author

Stifael commented May 31, 2017

@mcharleb:
I am not sure If I understood your suggestion correctly: do you suggest to have everything related to a specific platform in one repo? Basically that would mean that DriverFramework would be part of the Posix repo?
Anyway I think @bkueng or @julianoes can better answer that question

@Stifael
Copy link
Contributor Author

Stifael commented May 31, 2017

Since this "issue" could become large, I will add suggestion/questions to the top comment.

@bkueng
Copy link
Member

bkueng commented May 31, 2017

@mcharleb

Would it help to factor out Linux, QuRT and NuttX support into separate git repos? One of the concerns that I have heard about PX4 is the size of the codebase, but a lot of that is platform support and drivers

Factoring that out would not reduce the code base. And personally I find it a bit of a hassle coping with submodules - having more makes things worse. Especially if the changes affect multiple repos. repo might help. However I think the more important thing that people can easier cope with the code base is having a good directory structure with clear platform separation & API's (which we more or less have), avoid platform-specific ifdefs in general modules (@davids5 helps pushing that), and documentation. This includes some high-level overview graph like the google doc above, module documentation (#7145), and arch-specifics for each HW we support (which could also use some improvements, especially for Snapdragon, since it's the most exotic architecture).

@dagar
Copy link
Member

dagar commented May 31, 2017

@bkueng @mcharleb one minor thing on that note is if we confine NuttX to a single directory it doesn't even need to be cloned by default. The build system can handle that on demand as a dependency only if building a nuttx config.

I've personally experienced a lot of pain with our submodules (especially the submodules within submodules), but still find them to be the least bad option.

@julianoes
Copy link
Contributor

The build system can handle that on demand as a dependency only if building a nuttx config.

I would like that but I'm not sure how to do it technically. Just script git cloning?

@MaEtUgR
Copy link
Member

MaEtUgR commented May 31, 2017

Thanks @Stifael, it was a very good idea to create this issue. Finally the graph I started and tried to get spread finds attention and can serve as Request For Comments 😃

@davids5
Copy link
Member

davids5 commented May 31, 2017

@dagar - whatever we do here for NuttX please ensure to keep the development mode and not auto update it. GIT_SUBMODULES_ARE_EVIL=true

@davids5
Copy link
Member

davids5 commented May 31, 2017

@dagar - I have had issues with some DF submodules. I have had to rm -fr directories that I do not ever do any work in. I then use my goto command gitsync

#!/bin/bash
git submodule sync --recursive && git submodule update --init --recursive

I also find this command a big help gitlast

#/bin/bash
git for-each-ref --count=30 --sort=-committerdate refs/heads/ --format='%(refname:short)'

@jgoppert
Copy link
Member

jgoppert commented May 31, 2017

I support @mcharleb 's suggestions. @bkueng This type of layout should actually reduce the number of submodules you have to deal with for a given build.

My dream repo structure:

PX4 Library

My concept of this library is basically like ECL but using uORB and defining entire PX4 modules, or just call it ECL if we want to stick with that name. The benefit would be that these modules would be reusable and easily interchangeable, only dependent on uORB for internal message passing and an OS interface

  • Submodules:
    • mavlink
    • math library
    • ecl (or just incorporate the source into the PX4 library)
  • OS abstraction layer (think we already have this with PX4_...)
  • uORB Message definitions (internal messaging) -> prepare for possible switch to ROS2
  • MAVLink submodule (external messaging)
  • C++ modules that use those message definitions and OS abstraction layer
    • Estimators
    • Controllers
    • Message Handling
  • Include intense code verification and inspection here, abstract the low level driver data when testing
  • External interface to APM: Currently ECL is also used by APM. The question would be how hard it would be used use this new type of library directly for APM. I think it would be possible since they would only need to support the OS API calls and uORB.
  • CMake Output: Ideally a binary library depending on the cmake toolchain provided

PX4 Gazebo

  • submodules or CMake External
    • PX4 Library
  • gazebo models
  • gazebo plugins
  • CMake Output: plugin libraries for gazebo, just make flight control a plugin as well? might simplify multi-vehicle use-case, alternatively, stick to udp style and create px4 posix executable

PX4 OS Posix

  • submodules or CMake External
    • PX4 Library
      • mavlink
      • math library
  • Posix drivers
  • CMake Output: posix executable

PX4 OS Nuttx

  • submodules or CMake External
    • PX4 Library
      • mavlink
      • math library
    • NuttX
  • CMake Output: nuttx image

Downsides of this

  • Each PX4 OS repository would have it's own commit version of the PX4 Library that it was using. This could be somewhat mitigated by have the CI for the core repo try to build a set of OS repos, and if they fail then report a failure, if they succeed, then automatically push the updated version. Or as @bkueng suggested use a tool like repo, that just uses a branch name instead of an actual commit, but this is hard for unit testing, since you have no guarantee that a commit that passed CI will still pass CI when someone updates a branch on a dependent repo a week later..

@jgoppert jgoppert self-assigned this May 31, 2017
@mhkabir mhkabir self-assigned this Jun 1, 2017
@bkueng
Copy link
Member

bkueng commented Jun 1, 2017

@jgoppert ok if you put it that way it does look nice! But one more downside that I see: I often test things in SITL and then directly on an NuttX board. Switching repos in between would make that workflow much harder.
Also what about mixed architectures like Snapdragon? It has 2 OS'es. I could also imagine an architecture that has NuttX & Linux, for example when we want to run some modules on the Linux side on the Aero. Cutting these into separate repos makes things more difficult.
My take-away from this is that we need a good interface/abstraction towards the OS.

@bkueng
Copy link
Member

bkueng commented Jun 1, 2017

I created another doc with the current state & changes in red: https://docs.google.com/drawings/d/1oJ6x8HaqiRiW7cl-WFoC24Gn1lOkpjIPyGak5cfryPw/edit?usp=sharing. This should help us see which changes are required and which ones are independent from others.
Feel free to correct, modify, and/or extend.

We should also take FW, VTOL, Rover, Boats, Rockets, ... into account (There was some discussion about VTOL in #4845). @AndreasAntener @Tumbili @sanderux

@jgoppert
Copy link
Member

jgoppert commented Jun 1, 2017

@ bkeung Some other take-aways for me:

  1. PX4 should evolve to become a flight control library
  2. There should be no driver code in the PX4 library
  3. The PX4 library should provide a set of nodes all using a common pub/sub framework with message definitions

For pub/sub we can stick with uORB for now with a ROS adapter until ROS2 is ready. When ROS2 is ready, I think uORB should be gone. The PX4 OS drivers can all live in one repo or separate repos, whatever we find more convenient.

@Stifael
Copy link
Contributor Author

Stifael commented Jun 1, 2017

@jgoppert
Do you suggest a wrapper around uorb? How "common" are all the different pub/sub methods? Im asking because there was the discussion to incorporate priorities into uorb, or even fault detection for some of the messages. I guess that would then break the pub/sub methology?

@jgoppert
Copy link
Member

jgoppert commented Jun 1, 2017

Well uORB currently works in practice while ROS2 doesn't. Whatever we need extra to do our job we should make sure it makes it into ROS2 whille we still have input. I watched a talk on ros2 and they are definetly interested in supporting our use case. They are doing a lot of work on latency and real time. Currently they are very similar but ros 1 doesn't have priority and uorb doesn't have queue size etc. In my iekf branch I already wrote a ros api port using uorb as the backend, and there has been some other work on a middle layer as well, so definitely doable.

@eyeam3 eyeam3 self-assigned this Jun 2, 2017
@stale
Copy link

stale bot commented Jan 30, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@julianoes
Copy link
Contributor

I think the discussion will come up again.

@MaEtUgR
Copy link
Member

MaEtUgR commented Jan 31, 2019

@julianoes I think it's a very important topic to make sure we're all pulling on the same rope. Can we discuss some high-level possibilities in the next dev call?

@dagar dagar removed the status/STALE label Feb 5, 2019
@dagar dagar removed the devcall label Apr 16, 2019
@stale stale bot unassigned simonegu Jul 15, 2019
@PX4 PX4 deleted a comment from stale bot Jul 15, 2019
@stale stale bot removed the Admin: Wont fix label Jul 15, 2019
@stale
Copy link

stale bot commented Oct 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Oct 13, 2019
@MaEtUgR
Copy link
Member

MaEtUgR commented Feb 19, 2020

We need to reflect on the topic and follow-up with adapted plans otherwise this issue is useless.

@stale stale bot removed the stale label Feb 19, 2020
@LorenzMeier
Copy link
Member

I'm closing this because this is just stale. We need to error on the side of having relevant issues open and actually following up and have not as much fear of loosing information.

Open source projects live and die around active contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment