-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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
Plane: Onion part 1b #10162
Plane: Onion part 1b #10162
Conversation
#include "mode.h" | ||
#include "Plane.h" | ||
|
||
bool ModeAcro::_enter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool
is interesting.
Traditionally on Plane we never fail to enter a mode....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... not when we're done with the onion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do fail to enter Q modes actually, though it works by switching back to previous_mode
arduplane.zip |
need to fix commit msg on the major commit |
For reference, the Flight modes are defined in Mavlink enums: |
9a21ef5
to
4bd5eeb
Compare
@amilcarlucas For Plane modes, we have this. Just like the other vehicles. |
rebased on master and updated commit msg |
I whould like to see that re-written as That way it is clear that they need to be in sync with mavlink AND still allows one to add a new mode by appending it to the bottom of the list without the = sign. |
@amilcarlucas fair request. However, all vehicles need that change. Let's leave that for another PR. |
@magicrub OK, I'll do a PR once this one goes in. |
@magicrub @amilcarlucas We've been craving for an AP_Telemetry library forever, tying things more into MAVLink only makes that job harder, not easier... The flight mode numbers have its origin in ArduPilot, not in MAVLink. |
4bd5eeb
to
c1069a3
Compare
rebased again. I looked at adding comments on mode.h but I just don't see where adding any would help. All comments also match rover/copter. Please don't hold this PR to a higher standard than rover/copter mode system. |
On Tue, 15 Jan 2019, Amilcar Lucas wrote:
@magicrub OK, I'll do a PR once this one goes in.
You could make an equivalent PR against Copter or Rover so we can consider
:-)
|
@magicrub, I should just stay quiet but I'm excited about this PR.. I guess you said on the dev call that you wanted to do some more testing or are you happy for this to go in now? I see the WIP label is gone.. |
@OXINARF we can cross that bridge once we get there :) |
c1069a3
to
7341af3
Compare
Needs a rebase |
7341af3
to
18350b6
Compare
rebased |
437d982
to
2fa5cdd
Compare
During the lat devcall @tridge offered to help test, although he'll be unavailable for the next week so it will have to wait until he gets back. Meanwhile, I've been rebasing it every few days. |
6195297
to
be031c5
Compare
Hey @magicrub, what's the status of this? |
be031c5
to
f228452
Compare
I just rebased this. @peterbarker and I will be flying it here in Canberra dev conference. Plan is to pound on it and get it merged this weekend |
9a4e816
to
e78eda4
Compare
This was just flown on two aircraft at CMAC, owned by @peterbarker and @tridge . So, things are looking promising for a merge very soon! |
e78eda4
to
c771eeb
Compare
rebased. I've also added the DevTopic tag. Let's leave that tag on until this merges |
This needs some eyeballs on it. Can a few other devs PLEASE take a look at this? |
} | ||
break; | ||
// we failed entering new mode, roll back to old | ||
previous_mode = &old_previous_mode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you need to re-enter the old mode, as enter a mode has significant side effects? (IE navigation target etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, quadplane is the black duck here. It's the only thing that can fail a set_mode. I don't have a quadplane to test thoroughly. Can you test it for me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@magicrub, I'd vote for testing it in SITL..
c771eeb
to
732bc0d
Compare
I think @tridge 's baby hit the merge button on accident! |
This replaces #10107 by rolling back a couple of the changes that held mode properties. It should be more "obviously correct" with structure-only changes. Next part will be where we are doing more fancy things with the mode classes. Let's get the structure in place and merged into master so we can then more easily discuss how to do all the optimization stuff with using the mode classes.
I marked this WIP because I plan to break the commits up a little better and add some comments. No source code changes are planned. This PR is here to get more eyes on this and get some testing happening. It would be a HUGE help if @Naterater could do some flight tests with this in all the flight modes. Ping us if you need a binary!